Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fiber cannot render to DocumentFragment/ShadowRoot #11017

Closed
Hotell opened this issue Oct 1, 2017 · 12 comments · Fixed by #11037
Closed

Fiber cannot render to DocumentFragment/ShadowRoot #11017

Hotell opened this issue Oct 1, 2017 · 12 comments · Fixed by #11037

Comments

@Hotell
Copy link
Contributor

Hotell commented Oct 1, 2017

Do you want to request a feature or report a bug?
bug

What is the current behavior?
React 15.x was able to render within a DocumentFragment/shadowRoot, not possible with Fiber anymore

DEMO

https://www.webpackbin.com/bins/-KvPFG7-HGfQ34IgUxQt

import React, {Component} from 'react'
import {render} from 'react-dom'


class MyComponent extends Component{
  render(){
    return <div>Hello From React !</div>
  }
}

class MyElement extends HTMLElement {
  static is = 'my-element'
  constructor(){
    super()
    const shadowRoot = this.attachShadow({mode:'open'})
    try {
      // this fails 
      render(<MyComponent/>, shadowRoot)
    } catch (e){
      console.error(e)
      shadowRoot.innerHTML = `
        <div style="color:red;">${e}</div>
        <div>see console log for stack trace</div>
       `
    }
  }
  connectedCallback(){
    console.log('MyElement mounted!')
  }
}

customElements.define(MyElement.is, MyElement)

related -> skatejs/renderer-react#3

What is the expected behavior?
be able to render to shadow root like previous versions of React

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
React 16
Chrome

@gurkerl83
Copy link

Hi, I have determined the same issue a few days ago.

This might be related to

Wildhoney/Keo#37

The second note describes a fix to this problem. The method is getRootHostContext(rootContainerInstance) in the ReactFiberReconciler. If you apply this fix in the source and recompile, you will get another warning which says

Warning: validateDOMNesting(...):

cannot appear as a child of <#document>.

This issue needs more investigation, but the application itself should work on shadow-dom.

Thanks,
Markus

@gaearon
Copy link
Collaborator

gaearon commented Oct 2, 2017

I have a fix in #11037.

@gaearon
Copy link
Collaborator

gaearon commented Oct 2, 2017

Note: there are more places in the source where we compare to DOCUMENT_NODE. Should we compare to DOCUMENT_FRAGMENT_NODE for them too? Do you want to do an audit and check all these cases?

@Hotell
Copy link
Contributor Author

Hotell commented Oct 2, 2017

Fix looks good @gaearon ! thx for super fast response :)

Note: there are more places in the source where we compare to DOCUMENT_NODE. Should we compare to DOCUMENT_FRAGMENT_NODE for them too? Do you want to do an audit and check all these cases?

I think this check within getRootHostContext should be enough for WebComponents

@Hotell
Copy link
Contributor Author

Hotell commented Oct 2, 2017

maybe also adding addition test for <template/> tag ?

const template = document.createElement('template')
template.innerHTML = `<div>Render here</div>`

const templateInstance = template.content.cloneNode(true)
ReactDOM.render(React.createElement(HelloWorld), templateInstance);

@gaearon
Copy link
Collaborator

gaearon commented Oct 2, 2017

Would you like to contribute to the fixture I added? You can send a PR against my fork branch. You know more about web components than I do :-)

@gurkerl83
Copy link

Hi, the mentioned warning validateDOMNesting did only occur with the development bundle. I did forget to activate NODE_ENV = 'production'.

Which tags to cover in a test might be a discussion for a dedicated thread because not only the tag but also the tag was introduced in the shadow DOM with V1.

@gaearon
Copy link
Collaborator

gaearon commented Oct 2, 2017

I fixed the warning too.

@Hotell
Copy link
Contributor Author

Hotell commented Oct 2, 2017

Would you like to contribute to the fixture I added? You can send a PR against my fork branch. You know more about web components than I do :-)

huh I was AFK, next time sure ! thanks again @gaearon ! much appreciated 👍

@gaearon gaearon mentioned this issue Oct 3, 2017
19 tasks
@gaearon
Copy link
Collaborator

gaearon commented Nov 3, 2017

React 16.1.0-beta has been released. Please update react, react-dom, and react-test-renderer (if you use it) to this version and let us know if it solved the issue! We’d appreciate if you could test before Monday when we plan to get 16.1.0 out.

@guillaumesoldera
Copy link

With 16.1.0-beta, render into document fragment works for me! Thanks!

@Hotell
Copy link
Contributor Author

Hotell commented Nov 7, 2017

I am more than happy to confirm that with 16.1.0-beta everything works! thanks a lot @gaearon much appreciated. Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants