From 21b5aad51e469bf6a26b79e95e1a157ffe873728 Mon Sep 17 00:00:00 2001 From: ventuno Date: Tue, 9 Aug 2016 21:28:31 -0700 Subject: [PATCH 1/3] Warn when using React.unmountComponentAtNode on a different React instance's tree https://github.com/facebook/react/issues/3787 --- src/renderers/dom/client/ReactMount.js | 61 +++++++++++++++++++------- 1 file changed, 46 insertions(+), 15 deletions(-) diff --git a/src/renderers/dom/client/ReactMount.js b/src/renderers/dom/client/ReactMount.js index c33dee098ca7a..7e2c359e811ba 100644 --- a/src/renderers/dom/client/ReactMount.js +++ b/src/renderers/dom/client/ReactMount.js @@ -206,6 +206,41 @@ function hasNonRootReactChild(container) { } } +/** + * True if the supplied DOM node is a React DOM element and + * it has been rendered by a different React instance. + * + * @param {?DOMElement} node The candidate DOM node. + * @return {boolean} True if the DOM has been rendered by a different React instance + * @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 isNodeElement(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 isNodeElement(node) && (node.hasAttribute(ROOT_ATTR_NAME) || node.hasAttribute(ATTR_NAME)); +} + function getHostRootInstanceInContainer(container) { var rootEl = getReactRootElementInContainer(container); var prevHostInstance = @@ -329,11 +364,7 @@ var ReactMount = { ); invariant( - container && ( - container.nodeType === ELEMENT_NODE_TYPE || - container.nodeType === DOC_NODE_TYPE || - container.nodeType === DOCUMENT_FRAGMENT_NODE_TYPE - ), + isNodeElement(container), '_registerComponent(...): Target container is not a DOM element.' ); @@ -546,14 +577,18 @@ var ReactMount = { ); invariant( - container && ( - container.nodeType === ELEMENT_NODE_TYPE || - container.nodeType === DOC_NODE_TYPE || - container.nodeType === DOCUMENT_FRAGMENT_NODE_TYPE - ), + isNodeElement(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 React instance.' + ); + } + var prevComponent = getTopLevelWrapperInContainer(container); if (!prevComponent) { // Check if the node being unmounted was rendered by React, but isn't a @@ -599,11 +634,7 @@ var ReactMount = { transaction ) { invariant( - container && ( - container.nodeType === ELEMENT_NODE_TYPE || - container.nodeType === DOC_NODE_TYPE || - container.nodeType === DOCUMENT_FRAGMENT_NODE_TYPE - ), + isNodeElement(container), 'mountComponentIntoNode(...): Target container is not valid.' ); From 7d643fedbc762addd00cde1b4e9d31cc5abbde30 Mon Sep 17 00:00:00 2001 From: ventuno Date: Mon, 15 Aug 2016 21:16:12 -0700 Subject: [PATCH 2/3] Adding tests. --- src/renderers/dom/client/ReactMount.js | 6 +++- .../dom/client/__tests__/ReactMount-test.js | 28 +++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/renderers/dom/client/ReactMount.js b/src/renderers/dom/client/ReactMount.js index 7e2c359e811ba..8da5bdffedfd3 100644 --- a/src/renderers/dom/client/ReactMount.js +++ b/src/renderers/dom/client/ReactMount.js @@ -227,7 +227,11 @@ function nodeIsRenderedByOtherInstance(container) { * @internal */ function isNodeElement(node) { - return !!(node && (node.nodeType === ELEMENT_NODE_TYPE || node.nodeType === DOC_NODE_TYPE || node.nodeType === DOCUMENT_FRAGMENT_NODE_TYPE)); + return !!(node && ( + node.nodeType === ELEMENT_NODE_TYPE || + node.nodeType === DOC_NODE_TYPE || + node.nodeType === DOCUMENT_FRAGMENT_NODE_TYPE + )); } /** diff --git a/src/renderers/dom/client/__tests__/ReactMount-test.js b/src/renderers/dom/client/__tests__/ReactMount-test.js index 3b79e8cc71ed2..47189fb6d879d 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 instance', function() { + jest.resetModuleRegistry(); + var ReactDOMOther = require('ReactDOM'); + var container = document.createElement('container'); + + class Component extends React.Component { + render() { + return
; + } + } + + ReactDOM.render(, container); + // Make sure ReactDOM and ReactDOMOther are different instances + 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 React instance.' + ); + + // Don't throw a warning if the correct React instance 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; From 0287f4f82a90882dc9d9ad189e5b1e33e3675cab Mon Sep 17 00:00:00 2001 From: ventuno Date: Wed, 17 Aug 2016 16:55:55 -0700 Subject: [PATCH 3/3] Implementing recommended changes. https://github.com/facebook/react/issues/3787 --- src/renderers/dom/client/ReactMount.js | 16 ++++++++-------- .../dom/client/__tests__/ReactMount-test.js | 10 +++++----- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/renderers/dom/client/ReactMount.js b/src/renderers/dom/client/ReactMount.js index 8da5bdffedfd3..e8d7e3d5ae574 100644 --- a/src/renderers/dom/client/ReactMount.js +++ b/src/renderers/dom/client/ReactMount.js @@ -208,10 +208,10 @@ function hasNonRootReactChild(container) { /** * True if the supplied DOM node is a React DOM element and - * it has been rendered by a different React instance. + * 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 a different React instance + * @return {boolean} True if the DOM has been rendered by another copy of React * @internal */ function nodeIsRenderedByOtherInstance(container) { @@ -226,7 +226,7 @@ function nodeIsRenderedByOtherInstance(container) { * @return {boolean} True if the DOM is a valid DOM node. * @internal */ -function isNodeElement(node) { +function isValidContainer(node) { return !!(node && ( node.nodeType === ELEMENT_NODE_TYPE || node.nodeType === DOC_NODE_TYPE || @@ -242,7 +242,7 @@ function isNodeElement(node) { * @internal */ function isReactNode(node) { - return isNodeElement(node) && (node.hasAttribute(ROOT_ATTR_NAME) || node.hasAttribute(ATTR_NAME)); + return isValidContainer(node) && (node.hasAttribute(ROOT_ATTR_NAME) || node.hasAttribute(ATTR_NAME)); } function getHostRootInstanceInContainer(container) { @@ -368,7 +368,7 @@ var ReactMount = { ); invariant( - isNodeElement(container), + isValidContainer(container), '_registerComponent(...): Target container is not a DOM element.' ); @@ -581,7 +581,7 @@ var ReactMount = { ); invariant( - isNodeElement(container), + isValidContainer(container), 'unmountComponentAtNode(...): Target container is not a DOM element.' ); @@ -589,7 +589,7 @@ var ReactMount = { warning( !nodeIsRenderedByOtherInstance(container), 'unmountComponentAtNode(): The node you\'re attempting to unmount ' + - 'was rendered by another React instance.' + 'was rendered by another copy of React.' ); } @@ -638,7 +638,7 @@ var ReactMount = { transaction ) { invariant( - isNodeElement(container), + 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 47189fb6d879d..ca7453d8d9439 100644 --- a/src/renderers/dom/client/__tests__/ReactMount-test.js +++ b/src/renderers/dom/client/__tests__/ReactMount-test.js @@ -229,10 +229,10 @@ describe('ReactMount', function() { ); }); - it('should warn if the unmounted node was rendered by another instance', 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('container'); + var container = document.createElement('div'); class Component extends React.Component { render() { @@ -241,7 +241,7 @@ describe('ReactMount', function() { } ReactDOM.render(, container); - // Make sure ReactDOM and ReactDOMOther are different instances + // Make sure ReactDOM and ReactDOMOther are different copies expect(ReactDOM).not.toEqual(ReactDOMOther); spyOn(console, 'error'); @@ -249,10 +249,10 @@ describe('ReactMount', function() { 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 React instance.' + 'was rendered by another copy of React.' ); - // Don't throw a warning if the correct React instance unmounts the node + // Don't throw a warning if the correct React copy unmounts the node ReactDOM.unmountComponentAtNode(container); expect(console.error.calls.count()).toBe(1); });