Skip to content

Commit

Permalink
Warn if using React.unmountComponentAtNode on a different React insta…
Browse files Browse the repository at this point in the history
…nce's tree. (#7456)

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

#3787

* Adding tests.

* Implementing recommended changes.

#3787
  • Loading branch information
ventuno authored and sophiebits committed Aug 18, 2016
1 parent 921d8c1 commit a9e681a
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 15 deletions.
65 changes: 50 additions & 15 deletions src/renderers/dom/client/ReactMount.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down Expand Up @@ -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.'
);

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.'
);

Expand Down
28 changes: 28 additions & 0 deletions src/renderers/dom/client/__tests__/ReactMount-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 <div><div /></div>;
}
}

ReactDOM.render(<Component />, 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;
Expand Down

0 comments on commit a9e681a

Please sign in to comment.