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

Rendering components with identical children into the same parent container breaks #126

Closed
hatched opened this issue Apr 16, 2016 · 7 comments
Assignees

Comments

@hatched
Copy link

hatched commented Apr 16, 2016

If you try to render two different components into the same parent container multiple times with shared child components the browser will throw an error:

Uncaught HierarchyRequestError: Failed to execute 'replaceChild' on 'Node': The new child element contains the parent.

"preact": "4.6.2",
"preact-render-to-string": "2.4.0"

Chrome Version 50.0.2661.75 (64-bit)

This is easier to explain with some code:

class Intro extends Component {
  render(props) {
    return (<span>Hello</span>)
  }
}
class Index extends Component {
  render(props) {
    return (
      <div>
        <Intro />
      </div>)
  }
}
class Join extends Component {
  render(props) {
    return (<Intro />)
  }
}
  let components = [Index, Join, Index, Join, Index]
  // This loop simulates the user navigating forward and backwards over two components.
  components.forEach(function(Cmp) {
    render(
      <Cmp data={window.preactProps} />,
      document.body,
      document.body.firstChild)
  })
@developit
Copy link
Member

Ah - looks like this has to do with one of the components being high-order and the other not.

@developit developit self-assigned this Apr 16, 2016
developit added a commit that referenced this issue Apr 16, 2016
@developit
Copy link
Member

developit commented Apr 16, 2016

@hatched Quick fix for this (at least for your example) would be to not rely on document.body.firstChild. (interesting to note: using document.body.lastChild seems to fix the issue above).

Instead, I'd recommend using the returned root node from render():

  let root;
  let components = [Index, Join, Index, Join, Index]
  // This loop simulates the user navigating forward and backwards over two components.
  components.forEach(function(Cmp) {
    root = render(
      <Cmp data={window.preactProps} />,
      document.body,
      root)
  })

This accounts for the case where render() destroys the previous rendered root.

I'm working on a lower-level fix for this, in case the explanation above only solves your reproduction and not the bug as you ran into it (let me know).

@hatched
Copy link
Author

hatched commented Apr 16, 2016

Thanks for the quick reply!

Because this is being first rendered on the server I had to initialize root with document.body.firstChild so that it wouldn't get a duplicate render on load. With that change it appears to have resolved the issue.

function renderParent(Component) {
  let root = document.body.firstChild
  root = render(
    <Component data={window.preactProps} />,
    document.body,
    root)
}

@developit
Copy link
Member

Perfect. Now I'm a little stuck: should I keep the fix I just added that checks before invoking replaceChild(), or drop it since it still doesn't fix the firstChild issue?

@hatched
Copy link
Author

hatched commented Apr 16, 2016

I think that it is always a good idea to code defensively and it is a quite small change so unless there are some unintended consequences that I'm not aware of I'd vote to keep it in.

@developit
Copy link
Member

True, it's literally 2 bytes after gzip. I shouldn't agonize over these things so much, haha.

@developit
Copy link
Member

developit commented Apr 16, 2016

I'm going to close this issue out for now since we've got a solution, though I'll likely re-reference it when I address making render() automatically detect and attach to existing rendered components.

edit: for reference, reproduction I was using to test: http://jsfiddle.net/developit/t8p15pps/

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

No branches or pull requests

2 participants