From 822587ea06b1578d41f8ffdd4f1ed782c222d3b1 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 1 Feb 2017 17:45:28 -0800 Subject: [PATCH 1/7] Add extra invalid element info to invariant message --- src/renderers/dom/fiber/ReactDOMFiber.js | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/renderers/dom/fiber/ReactDOMFiber.js b/src/renderers/dom/fiber/ReactDOMFiber.js index 8708b20a0e4d5..278e4e62e2edc 100644 --- a/src/renderers/dom/fiber/ReactDOMFiber.js +++ b/src/renderers/dom/fiber/ReactDOMFiber.js @@ -368,7 +368,20 @@ var ReactDOM = { // allows arrays. invariant( isValidElement(element), - 'render(): Invalid component element.' + 'ReactDOM.render(): Invalid component element.%s', + ( + typeof element === 'string' ? + ' Instead of passing a string like \'div\', pass ' + + 'React.createElement(\'div\') or
.' : + typeof element === 'function' ? + ' Instead of passing a class like Foo, pass ' + + 'React.createElement(Foo) or .' : + // Check if it quacks like an element + element != null && element.props !== undefined ? + ' This may be caused by unintentionally loading two independent ' + + 'copies of React.' : + '' + ) ); } From d47c21589b1bf63cc93866aaba644dbc245904a5 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 1 Feb 2017 17:56:19 -0800 Subject: [PATCH 2/7] Warn when mounting into document.body --- src/renderers/dom/fiber/ReactDOMFiber.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/renderers/dom/fiber/ReactDOMFiber.js b/src/renderers/dom/fiber/ReactDOMFiber.js index 278e4e62e2edc..803054e4dc953 100644 --- a/src/renderers/dom/fiber/ReactDOMFiber.js +++ b/src/renderers/dom/fiber/ReactDOMFiber.js @@ -385,6 +385,17 @@ var ReactDOM = { ); } + if (__DEV__) { + warning( + !container.tagName || container.tagName.toUpperCase() !== 'BODY', + 'render(): Rendering components directly into document.body is ' + + 'discouraged, since its children are often manipulated by third-party ' + + 'scripts and browser extensions. This may lead to subtle ' + + 'reconciliation issues. Try rendering into a container element created ' + + 'for your app.' + ); + } + return renderSubtreeIntoContainer(null, element, container, callback); }, From f4823295af9792c2ef6e0e7da6a4628009dd227d Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 1 Feb 2017 18:18:52 -0800 Subject: [PATCH 3/7] Warn when rendering into React-rendered child that isn't a root --- src/renderers/dom/fiber/ReactDOMFiber.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/renderers/dom/fiber/ReactDOMFiber.js b/src/renderers/dom/fiber/ReactDOMFiber.js index 803054e4dc953..5a8ba571f7864 100644 --- a/src/renderers/dom/fiber/ReactDOMFiber.js +++ b/src/renderers/dom/fiber/ReactDOMFiber.js @@ -386,6 +386,15 @@ var ReactDOM = { } if (__DEV__) { + warning( + !ReactDOMComponentTree.getInstanceFromNode(container) || + container._reactRootContainer, + 'render(...): Replacing React-rendered children with a new root ' + + 'component. If you intended to update the children of this node, ' + + 'you should instead have the existing children update their state ' + + 'and render the new components instead of calling ReactDOM.render.' + ); + warning( !container.tagName || container.tagName.toUpperCase() !== 'BODY', 'render(): Rendering components directly into document.body is ' + From 87bc7cef2c082b971a89cd68878d2b1801bfa14a Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 1 Feb 2017 18:46:06 -0800 Subject: [PATCH 4/7] Warn if unmounting a tree that was rendered by a different copy of React --- src/renderers/dom/fiber/ReactDOMFiber.js | 30 ++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/src/renderers/dom/fiber/ReactDOMFiber.js b/src/renderers/dom/fiber/ReactDOMFiber.js index 5a8ba571f7864..65b547fb5a30a 100644 --- a/src/renderers/dom/fiber/ReactDOMFiber.js +++ b/src/renderers/dom/fiber/ReactDOMFiber.js @@ -108,6 +108,18 @@ function validateContainer(container) { } } +function getReactRootElementInContainer(container : any) { + if (!container) { + return null; + } + + if (container.nodeType === DOC_NODE_TYPE) { + return container.documentElement; + } else { + return container.firstChild; + } +} + function shouldAutoFocusHostComponent( type : string, props : Props, @@ -386,9 +398,12 @@ var ReactDOM = { } if (__DEV__) { + const isRootRenderedBySomeReact = Boolean(container._reactRootContainer); + const isInTreeRenderedByThisReact = ReactDOMComponentTree.getInstanceFromNode(container); + warning( - !ReactDOMComponentTree.getInstanceFromNode(container) || - container._reactRootContainer, + !isInTreeRenderedByThisReact || + isRootRenderedBySomeReact, 'render(...): Replacing React-rendered children with a new root ' + 'component. If you intended to update the children of this node, ' + 'you should instead have the existing children update their state ' + @@ -427,7 +442,18 @@ var ReactDOM = { 'unmountComponentAtNode(...): Target container is not a DOM element.' ); warnAboutUnstableUse(); + if (container._reactRootContainer) { + if (__DEV__) { + const rootEl = getReactRootElementInContainer(container); + const renderedByDifferentReact = rootEl && !ReactDOMComponentTree.getInstanceFromNode(rootEl); + warning( + !renderedByDifferentReact, + 'unmountComponentAtNode(): The node you\'re attempting to unmount ' + + 'was rendered by another copy of React.' + ); + } + // Unmount should not be batched. return DOMRenderer.unbatchedUpdates(() => { return renderSubtreeIntoContainer(null, null, container, () => { From 255e5aeb3eeb5d3e8de5ab450bb9ac9682f647b4 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 6 Feb 2017 14:22:16 -0800 Subject: [PATCH 5/7] Rendering into an empty React-rendered element should not warn Only warn if the container has a React-rendered child. --- src/renderers/dom/fiber/ReactDOMFiber.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/renderers/dom/fiber/ReactDOMFiber.js b/src/renderers/dom/fiber/ReactDOMFiber.js index 65b547fb5a30a..5718e45c18845 100644 --- a/src/renderers/dom/fiber/ReactDOMFiber.js +++ b/src/renderers/dom/fiber/ReactDOMFiber.js @@ -399,10 +399,11 @@ var ReactDOM = { if (__DEV__) { const isRootRenderedBySomeReact = Boolean(container._reactRootContainer); - const isInTreeRenderedByThisReact = ReactDOMComponentTree.getInstanceFromNode(container); + const rootEl = getReactRootElementInContainer(container); + const hasNonRootReactChild = Boolean(rootEl && ReactDOMComponentTree.getInstanceFromNode(rootEl)); warning( - !isInTreeRenderedByThisReact || + !hasNonRootReactChild || isRootRenderedBySomeReact, 'render(...): Replacing React-rendered children with a new root ' + 'component. If you intended to update the children of this node, ' + From 9fcf761e904f012865342a4b7382e16f7e93192e Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 6 Feb 2017 14:22:27 -0800 Subject: [PATCH 6/7] Run test script --- scripts/fiber/tests-failing.txt | 2 -- scripts/fiber/tests-passing-except-dev.txt | 3 --- scripts/fiber/tests-passing.txt | 5 +++++ 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 06137eac04917..bd380aefce6fd 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -18,8 +18,6 @@ src/renderers/dom/shared/__tests__/ReactDOMTextComponent-test.js * can reconcile text arbitrarily split into multiple nodes on some substitutions only src/renderers/dom/shared/__tests__/ReactMount-test.js -* throws when given a string -* throws when given a factory * tracks root instances * marks top-level mounts diff --git a/scripts/fiber/tests-passing-except-dev.txt b/scripts/fiber/tests-passing-except-dev.txt index decc542712916..90cc310b424f9 100644 --- a/scripts/fiber/tests-passing-except-dev.txt +++ b/scripts/fiber/tests-passing-except-dev.txt @@ -7,10 +7,7 @@ src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js src/renderers/dom/shared/__tests__/ReactMount-test.js * should warn if mounting into dirty rendered markup -* should warn when mounting into document.body * should account for escaping on a checksum mismatch -* should warn if render removes React-rendered children -* should warn if the unmounted node was rendered by another copy of React src/renderers/dom/shared/__tests__/ReactMountDestruction-test.js * should warn when unmounting a non-container root node diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 3b85124db5684..0393d3c71e9cd 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -769,10 +769,15 @@ src/renderers/dom/shared/__tests__/ReactEventListener-test.js src/renderers/dom/shared/__tests__/ReactMount-test.js * throws when given a non-node +* throws when given a string +* throws when given a factory * should render different components in same root * should unmount and remount if the key changes * should reuse markup if rendering to the same target twice * should not warn if mounting into non-empty node +* should warn when mounting into document.body +* should warn if render removes React-rendered children +* should warn if the unmounted node was rendered by another copy of React * passes the correct callback context src/renderers/dom/shared/__tests__/ReactMountDestruction-test.js From 0564b08cf23a32124bdd21a4fc4f5f94dff65a75 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 6 Feb 2017 17:59:53 -0800 Subject: [PATCH 7/7] Split into multiple invariants That way the messages are extracted by the error code transform. --- src/renderers/dom/fiber/ReactDOMFiber.js | 45 +++++++++++++------- src/renderers/dom/stack/client/ReactMount.js | 45 +++++++++++++------- 2 files changed, 58 insertions(+), 32 deletions(-) diff --git a/src/renderers/dom/fiber/ReactDOMFiber.js b/src/renderers/dom/fiber/ReactDOMFiber.js index 5718e45c18845..cebe0414812b4 100644 --- a/src/renderers/dom/fiber/ReactDOMFiber.js +++ b/src/renderers/dom/fiber/ReactDOMFiber.js @@ -378,23 +378,36 @@ var ReactDOM = { // Top-level check occurs here instead of inside child reconciler because // because requirements vary between renderers. E.g. React Art // allows arrays. - invariant( - isValidElement(element), - 'ReactDOM.render(): Invalid component element.%s', - ( - typeof element === 'string' ? - ' Instead of passing a string like \'div\', pass ' + - 'React.createElement(\'div\') or
.' : - typeof element === 'function' ? - ' Instead of passing a class like Foo, pass ' + - 'React.createElement(Foo) or .' : + if (!isValidElement(element)) { + if (typeof element === 'string') { + invariant( + false, + 'ReactDOM.render(): Invalid component element. Instead of ' + + 'passing a string like \'div\', pass ' + + 'React.createElement(\'div\') or
.' + ); + } else if (typeof element === 'function') { + invariant( + false, + 'ReactDOM.render(): Invalid component element. Instead of ' + + 'passing a class like Foo, pass React.createElement(Foo) ' + + 'or .' + ); + } else if (element != null && typeof element.props !== 'undefined') { // Check if it quacks like an element - element != null && element.props !== undefined ? - ' This may be caused by unintentionally loading two independent ' + - 'copies of React.' : - '' - ) - ); + invariant( + false, + 'ReactDOM.render(): Invalid component element. This may be ' + + 'caused by unintentionally loading two independent copies ' + + 'of React.' + ); + } else { + invariant( + false, + 'ReactDOM.render(): Invalid component element.' + ); + } + } } if (__DEV__) { diff --git a/src/renderers/dom/stack/client/ReactMount.js b/src/renderers/dom/stack/client/ReactMount.js index 424cc4f678b11..cd3fe685035b1 100644 --- a/src/renderers/dom/stack/client/ReactMount.js +++ b/src/renderers/dom/stack/client/ReactMount.js @@ -434,23 +434,36 @@ var ReactMount = { _renderSubtreeIntoContainer: function(parentComponent, nextElement, container, callback) { validateCallback(callback, 'ReactDOM.render'); - invariant( - React.isValidElement(nextElement), - 'ReactDOM.render(): Invalid component element.%s', - ( - typeof nextElement === 'string' ? - ' Instead of passing a string like \'div\', pass ' + - 'React.createElement(\'div\') or
.' : - typeof nextElement === 'function' ? - ' Instead of passing a class like Foo, pass ' + - 'React.createElement(Foo) or .' : + if (!React.isValidElement(nextElement)) { + if (typeof nextElement === 'string') { + invariant( + false, + 'ReactDOM.render(): Invalid component element. Instead of ' + + 'passing a string like \'div\', pass ' + + 'React.createElement(\'div\') or
.' + ); + } else if (typeof nextElement === 'function') { + invariant( + false, + 'ReactDOM.render(): Invalid component element. Instead of ' + + 'passing a class like Foo, pass React.createElement(Foo) ' + + 'or .' + ); + } else if (nextElement != null && typeof nextElement.props !== 'undefined') { // Check if it quacks like an element - nextElement != null && nextElement.props !== undefined ? - ' This may be caused by unintentionally loading two independent ' + - 'copies of React.' : - '' - ) - ); + invariant( + false, + 'ReactDOM.render(): Invalid component element. This may be ' + + 'caused by unintentionally loading two independent copies ' + + 'of React.' + ); + } else { + invariant( + false, + 'ReactDOM.render(): Invalid component element.' + ); + } + } warning( !container || !container.tagName ||