From a9e681a82818bc92a512383caf060b4114a9eb97 Mon Sep 17 00:00:00 2001 From: ventuno Date: Wed, 17 Aug 2016 17:20:36 -0700 Subject: [PATCH] Warn if using React.unmountComponentAtNode on a different React instance's tree. (#7456) * Warn when using React.unmountComponentAtNode on a different React instance's tree https://github.com/facebook/react/issues/3787 * Adding tests. * Implementing recommended changes. https://github.com/facebook/react/issues/3787 --- src/renderers/dom/client/ReactMount.js | 65 ++++++++++++++----- .../dom/client/__tests__/ReactMount-test.js | 28 ++++++++ 2 files changed, 78 insertions(+), 15 deletions(-) diff --git a/src/renderers/dom/client/ReactMount.js b/src/renderers/dom/client/ReactMount.js index 5e1b2d8fabfdc..d7f24aaf386d0 100644 --- a/src/renderers/dom/client/ReactMount.js +++ b/src/renderers/dom/client/ReactMount.js @@ -207,6 +207,45 @@ function hasNonRootReactChild(container) { } } +/** + * True if the supplied DOM node is a React DOM element and + * it has been rendered by another copy of React. + * + * @param {?DOMElement} node The candidate DOM node. + * @return {boolean} True if the DOM has been rendered by another copy of React + * @internal + */ +function nodeIsRenderedByOtherInstance(container) { + var rootEl = getReactRootElementInContainer(container); + return !!(rootEl && isReactNode(rootEl) && !ReactDOMComponentTree.getInstanceFromNode(rootEl)); +} + +/** + * True if the supplied DOM node is a valid node element. + * + * @param {?DOMElement} node The candidate DOM node. + * @return {boolean} True if the DOM is a valid DOM node. + * @internal + */ +function isValidContainer(node) { + return !!(node && ( + node.nodeType === ELEMENT_NODE_TYPE || + node.nodeType === DOC_NODE_TYPE || + node.nodeType === DOCUMENT_FRAGMENT_NODE_TYPE + )); +} + +/** + * True if the supplied DOM node is a valid React node element. + * + * @param {?DOMElement} node The candidate DOM node. + * @return {boolean} True if the DOM is a valid React DOM node. + * @internal + */ +function isReactNode(node) { + return isValidContainer(node) && (node.hasAttribute(ROOT_ATTR_NAME) || node.hasAttribute(ATTR_NAME)); +} + function getHostRootInstanceInContainer(container) { var rootEl = getReactRootElementInContainer(container); var prevHostInstance = @@ -330,11 +369,7 @@ var ReactMount = { ); invariant( - container && ( - container.nodeType === ELEMENT_NODE_TYPE || - container.nodeType === DOC_NODE_TYPE || - container.nodeType === DOCUMENT_FRAGMENT_NODE_TYPE - ), + isValidContainer(container), '_registerComponent(...): Target container is not a DOM element.' ); @@ -540,14 +575,18 @@ var ReactMount = { ); invariant( - container && ( - container.nodeType === ELEMENT_NODE_TYPE || - container.nodeType === DOC_NODE_TYPE || - container.nodeType === DOCUMENT_FRAGMENT_NODE_TYPE - ), + isValidContainer(container), 'unmountComponentAtNode(...): Target container is not a DOM element.' ); + if (__DEV__) { + warning( + !nodeIsRenderedByOtherInstance(container), + 'unmountComponentAtNode(): The node you\'re attempting to unmount ' + + 'was rendered by another copy of React.' + ); + } + var prevComponent = getTopLevelWrapperInContainer(container); if (!prevComponent) { // Check if the node being unmounted was rendered by React, but isn't a @@ -593,11 +632,7 @@ var ReactMount = { transaction ) { invariant( - container && ( - container.nodeType === ELEMENT_NODE_TYPE || - container.nodeType === DOC_NODE_TYPE || - container.nodeType === DOCUMENT_FRAGMENT_NODE_TYPE - ), + isValidContainer(container), 'mountComponentIntoNode(...): Target container is not valid.' ); diff --git a/src/renderers/dom/client/__tests__/ReactMount-test.js b/src/renderers/dom/client/__tests__/ReactMount-test.js index 3b79e8cc71ed2..ca7453d8d9439 100644 --- a/src/renderers/dom/client/__tests__/ReactMount-test.js +++ b/src/renderers/dom/client/__tests__/ReactMount-test.js @@ -229,6 +229,34 @@ describe('ReactMount', function() { ); }); + it('should warn if the unmounted node was rendered by another copy of React', function() { + jest.resetModuleRegistry(); + var ReactDOMOther = require('ReactDOM'); + var container = document.createElement('div'); + + class Component extends React.Component { + render() { + return
; + } + } + + ReactDOM.render(, container); + // Make sure ReactDOM and ReactDOMOther are different copies + expect(ReactDOM).not.toEqual(ReactDOMOther); + + spyOn(console, 'error'); + ReactDOMOther.unmountComponentAtNode(container); + expect(console.error.calls.count()).toBe(1); + expect(console.error.calls.argsFor(0)[0]).toBe( + 'Warning: unmountComponentAtNode(): The node you\'re attempting to unmount ' + + 'was rendered by another copy of React.' + ); + + // Don't throw a warning if the correct React copy unmounts the node + ReactDOM.unmountComponentAtNode(container); + expect(console.error.calls.count()).toBe(1); + }); + it('passes the correct callback context', function() { var container = document.createElement('div'); var calls = 0;