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

Warn if using React.unmountComponentAtNode on a different React instance's tree. #7456

Merged
merged 3 commits into from
Aug 18, 2016

Conversation

ventuno
Copy link
Contributor

@ventuno ventuno commented Aug 10, 2016

As #3819 has been closed, I tried to provide a fix for #3787.

  1. If the node is a valid DOM node (e.g.: is of type Node.ELEMENT_NODE, Node.DOCUMENT_NODE or Node.DOCUMENT_FRAGMENT_NODE, see [1]);
  2. and if the node has been rendered by React (e.g.: has the data-reactid or data-reactroot attribute, see Warn when using React.unmountComponentAtNode on a different React instance's tree #3787 (comment));
  3. and if ReactDOMComponentTree.getInstanceFromNode returns null;
  4. then, this element has been rendered by a different React instance and we show a warning.

[1] https://developer.mozilla.org/en-US/docs/Web/API/Node/nodeType

@ghost
Copy link

ghost commented Aug 10, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@ghost ghost added the CLA Signed label Aug 10, 2016
@ghost
Copy link

ghost commented Aug 10, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@aweary
Copy link
Contributor

aweary commented Aug 16, 2016

@ventuno could you add tests for this as well to verify it works? Thanks!

@syranide
Copy link
Contributor

Am I misunderstanding this or is the warning really confusing? It's not about unmounting something that "was rendered by another React instance", but trying to unmount something that isn't a React root (in this case, a React sub-tree). Right?

@aweary
Copy link
Contributor

aweary commented Aug 16, 2016

It's not about unmounting something that "was rendered by another React instance"

@syranide looking at #3787 that does seem to be the goal. From @spicyj in the google groups thread:

Due to our approach of storing internal state in React internals so that it's not accessible outside of React, we'll probably never support your use case of using React.unmountComponentAtNode with a different copy of React than the one that rendered it initially. We can add a warning for this though – opened #3787 to track.

@ghost ghost added the CLA Signed label Aug 16, 2016
@ventuno
Copy link
Contributor Author

ventuno commented Aug 16, 2016

Indeed, the goal of this PR is to show a warning whenever a react node is being unmounted by a different React instance than the one that has originally rendered it.
I agree it's not a very common scenario, but it probably still makes sense to warn the user if such scenario is detectable as it generated confusion in the past (e.g.: the google groups thread mentioned by @aweary in #7456 (comment)).

@ghost ghost added the CLA Signed label Aug 16, 2016
* @return {boolean} True if the DOM is a valid DOM node.
* @internal
*/
function isNodeElement(node) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we call this isValidContainer?

@sophiebits
Copy link
Collaborator

Thanks for sending this in! Looks good except a couple nits inline. Mind updating?

@ghost ghost added the CLA Signed label Aug 17, 2016
@ghost ghost added the CLA Signed label Aug 17, 2016
@sophiebits sophiebits merged commit a9e681a into facebook:master Aug 18, 2016
@sophiebits
Copy link
Collaborator

Thanks!

@ventuno ventuno deleted the bug-3787 branch August 18, 2016 00:23
@zpao zpao modified the milestones: 15-next, 15.3.1 Aug 19, 2016
zpao pushed a commit that referenced this pull request Aug 19, 2016
…nce's tree. (#7456)

* Warn when using React.unmountComponentAtNode on a different React instance's tree

#3787

* Adding tests.

* Implementing recommended changes.

#3787

(cherry picked from commit a9e681a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants