Warn when passing invalid containers to render and unmountComponentAtNode

[charliermarsh]

Resolves #1858 and #2045.

The hasNonRootReactChild method checks if the container has a React-rendered child (i.e., is non-empty) with a non-root ID, which is used as the criteria for warning in both renderComponent and unmountComponentAtNode. Note that there's also a specific warning for when you pass in a root node, rather than a container.

For example, if your markup is:

<div id="root" data-reactid=".0">
  <div id="non-root" data-reactid=".0.0">
    <div data-reactid=".0.0.0"></div>
  </div>
</div>

Then you see the following warnings (prepended with method name, warning, etc.):

var nonRoot = document.getElementById('non-root');
React.unmountComponentAtNode(nonRoot);
>>> "The node you're attempting to unmount is not a valid React root node, and thus cannot be unmounted."

Or:

var root = document.getElementById('root');
React.unmountComponentAtNode(root);
>>> "The node you're attempting to unmount... You may have passed in a React root node as argument, rather than its container."

Or:

var root = document.getElementById('root');
React.renderComponent(<div />, root);
>>> "Replacing React-rendered children with a new root component."

The naming here got a little verbose, so I'm open to suggestions. I think the specific warning for passing in a root node as a container is also worth discussing, but I left it in for now as it's easy to remove.

[syranide]

(Kind of related #1912)

[zpao]

Style nit - break function call onto its own line, splitting the arg like this is awkward.

    var containerHasNonRootReactChild =
      ReactMount.hasNonRootReactChild(container);

[zpao]

A bunch of nits without a proper review! I think it mostly makes sense though. I'll give it a closer look soon.

[charliermarsh]

@zpao Addressed, thanks. (Was curious about the use of parens in ReactMountDestruction-test.js. What's an example where ASI would behave badly? Does it have to do with }? Seems like the first test in that file works without parens around the JSX as well.)

[zpao]

http://inimino.org/~inimino/blog/javascript_semicolons is probably a good thing to read.

In the case of returns, a semi colon is inserted at the end of the line, so

return
  4;

actually returns undefined, and then 4 is never reached. But this

return (
  4
);

ensures that an expression is started (via the opening paren) and gets evaluated. So that would return 4.

return 4;

of course returns 4 because it's on the same line. The other example in the file has everything on the same line, so that works.

Other cases in JSX work as long as you start it on the return line, but we don't like the way that looks so we prefer the explicit opening paren.

return <div>
  4
</div>;
// becomes
return React.DOM.div(null,
  "4"
);

You can see that the opening paren on the return line means that this works.

[sophiebits]

@crm416 I agree that the parens in ReactMountDestruction-test look unnecessary.

[zpao]

Ah, I was looking at the wrong file. Yea, the ones in the preceding test in ReactMountDestruction-test are unnecessary.

[charliermarsh]

@zpao Thanks for the explanation--really helpful. Rather than just remove the parens, I put the components in the preceding tests on a single line. Let me know if you prefer a different style.

[sophiebits]

Can we add a "If you intended to update the children of this node, you should instead have the component that rendered the existing children update its state and render the new components instead of calling the top-level React.render to update a subtree."

Also, s/renderComponent/render/. :)

[sophiebits]

Sorry for the long delay on this, @crm416. A few last inline comments, and then I think we'll be good to go. You'll also need to rebase for a clean merge – you'll at least need to change renderComponent to render; not sure if there's anything else.

[sophiebits]

Merged in 10ab0c8.