From f8930e72751e2673717530d73040dd817ce718f8 Mon Sep 17 00:00:00 2001 From: Charles Marsh Date: Wed, 13 Aug 2014 16:03:42 -0400 Subject: [PATCH 1/3] Warn when passing invalid containers to renderComponent and unmountComponentAtNode --- src/browser/ui/ReactMount.js | 51 ++++++++++++++++++- src/browser/ui/__tests__/ReactMount-test.js | 22 ++++++++ .../__tests__/ReactMountDestruction-test.js | 47 +++++++++++++++++ 3 files changed, 119 insertions(+), 1 deletion(-) diff --git a/src/browser/ui/ReactMount.js b/src/browser/ui/ReactMount.js index 581a904427d..82e047e1773 100644 --- a/src/browser/ui/ReactMount.js +++ b/src/browser/ui/ReactMount.js @@ -370,8 +370,19 @@ var ReactMount = { var reactRootElement = getReactRootElementInContainer(container); var containerHasReactMarkup = reactRootElement && ReactMount.isRenderedByReact(reactRootElement); + var containerHasNonRootReactChild = ReactMount.hasNonRootReactChild( + container); - var shouldReuseMarkup = containerHasReactMarkup && !prevComponent; + if (__DEV__) { + warning( + !containerHasNonRootReactChild, + 'renderComponent(...): Replacing React-rendered children with a new ' + + 'root component.' + ); + } + + var shouldReuseMarkup = containerHasReactMarkup && !prevComponent && + !containerHasNonRootReactChild; var component = ReactMount._renderNewRootComponent( nextDescriptor, @@ -459,6 +470,28 @@ var ReactMount = { var reactRootID = getReactRootID(container); var component = instancesByReactRootID[reactRootID]; if (!component) { + // Check if the node being unmounted was rendered by React, but isn't a + // root node. + var containerHasNonRootReactChild = ReactMount.hasNonRootReactChild( + container); + + // Check if the container itself is a React root node. + var containerID = ReactMount.getID(container); + var containerRootID = ReactInstanceHandles.getReactRootIDFromNodeID( + containerID); + var isContainerReactRoot = + containerID && containerRootID && containerID === containerRootID; + + if (__DEV__) { + warning( + !containerHasNonRootReactChild, + 'unmountComponentAtNode(): The node you\'re attempting to unmount ' + + 'is not a valid React root node, and thus cannot be unmounted.%s', + (isContainerReactRoot ? ' You may have passed in a React root ' + + 'node as argument, rather than its container.' : '') + ); + } + return false; } ReactMount.unmountComponentFromNode(component, container); @@ -560,6 +593,22 @@ var ReactMount = { return id ? id.charAt(0) === SEPARATOR : false; }, + /** + * True if the supplied DOM node has a direct React-rendered child that is + * not a React root element. Useful for warning in renderComponent`, + * `unmountComponentAtNode`, etc. + * + * @param {?DOMElement} node The candidate DOM node. + * @return {boolean} True if the DOM element contains a direct child that was + * rendered by React but is not a root element. + * @internal + */ + hasNonRootReactChild: function(node) { + var reactRootID = getReactRootID(node); + return reactRootID ? reactRootID !== + ReactInstanceHandles.getReactRootIDFromNodeID(reactRootID) : false; + }, + /** * Traverses up the ancestors of the supplied node to find a node that is a * DOM representation of a React component. diff --git a/src/browser/ui/__tests__/ReactMount-test.js b/src/browser/ui/__tests__/ReactMount-test.js index 4dd3448e672..156ea3e9d40 100644 --- a/src/browser/ui/__tests__/ReactMount-test.js +++ b/src/browser/ui/__tests__/ReactMount-test.js @@ -114,4 +114,26 @@ describe('ReactMount', function() { expect(instance1 === instance2).toBe(true); }); + + it('should warn if render removes React-rendered children', function() { + var container = document.createElement('container'); + var Component = React.createClass({ + render: function() { + return
+
+
; + } + }); + React.renderComponent(, container); + + // Test that blasting away children throws a warning + spyOn(console, 'warn'); + var rootNode = container.firstChild; + React.renderComponent(, rootNode); + expect(console.warn.callCount).toBe(1); + expect(console.warn.mostRecentCall.args[0]).toBe( + 'Warning: renderComponent(...): Replacing React-rendered children ' + + 'with a new root component.' + ); + }); }); diff --git a/src/browser/ui/__tests__/ReactMountDestruction-test.js b/src/browser/ui/__tests__/ReactMountDestruction-test.js index c73ddb461f6..743c526fd09 100644 --- a/src/browser/ui/__tests__/ReactMountDestruction-test.js +++ b/src/browser/ui/__tests__/ReactMountDestruction-test.js @@ -52,4 +52,51 @@ describe('ReactMount', function() { React.unmountComponentAtNode(secondRootDiv); expect(secondRootDiv.firstChild).toBeNull(); }); + + it("should warn when unmounting a non-container root node", function() { + var mainContainerDiv = document.createElement('div'); + + var component = ( +
+
+
+ ); + React.renderComponent(component, mainContainerDiv); + + // Test that unmounting at a root node gives a helpful warning + var rootDiv = mainContainerDiv.firstChild; + spyOn(console, 'warn'); + React.unmountComponentAtNode(rootDiv); + expect(console.warn.callCount).toBe(1); + expect(console.warn.mostRecentCall.args[0]).toBe( + 'Warning: unmountComponentAtNode(): The node you\'re attempting to ' + + 'unmount is not a valid React root node, and thus cannot be ' + + 'unmounted. You may have passed in a React root node as argument, ' + + 'rather than its container.' + ); + }); + + it("should warn when unmounting a non-container, non-root node", function() { + var mainContainerDiv = document.createElement('div'); + + var component = ( +
+
+
+
+
+ ); + React.renderComponent(component, mainContainerDiv); + + // Test that unmounting at a non-root node gives a different warning + var nonRootDiv = mainContainerDiv.firstChild.firstChild; + spyOn(console, 'warn'); + React.unmountComponentAtNode(nonRootDiv); + expect(console.warn.callCount).toBe(1); + expect(console.warn.mostRecentCall.args[0]).toBe( + 'Warning: unmountComponentAtNode(): The node you\'re attempting to ' + + 'unmount is not a valid React root node, and thus cannot be ' + + 'unmounted.' + ); + }); }); From 8ed5d7049ecd30798ac405b5f9001d095d63bab4 Mon Sep 17 00:00:00 2001 From: Charles Marsh Date: Thu, 21 Aug 2014 09:19:20 +0800 Subject: [PATCH 2/3] Addressed style nits --- src/browser/ui/ReactMount.js | 8 +++++--- src/browser/ui/__tests__/ReactMount-test.js | 4 +--- src/browser/ui/__tests__/ReactMountDestruction-test.js | 10 ++++------ 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/browser/ui/ReactMount.js b/src/browser/ui/ReactMount.js index 82e047e1773..38b1e5e7752 100644 --- a/src/browser/ui/ReactMount.js +++ b/src/browser/ui/ReactMount.js @@ -370,8 +370,8 @@ var ReactMount = { var reactRootElement = getReactRootElementInContainer(container); var containerHasReactMarkup = reactRootElement && ReactMount.isRenderedByReact(reactRootElement); - var containerHasNonRootReactChild = ReactMount.hasNonRootReactChild( - container); + var containerHasNonRootReactChild = + ReactMount.hasNonRootReactChild(container); if (__DEV__) { warning( @@ -381,7 +381,9 @@ var ReactMount = { ); } - var shouldReuseMarkup = containerHasReactMarkup && !prevComponent && + var shouldReuseMarkup = + containerHasReactMarkup && + !prevComponent && !containerHasNonRootReactChild; var component = ReactMount._renderNewRootComponent( diff --git a/src/browser/ui/__tests__/ReactMount-test.js b/src/browser/ui/__tests__/ReactMount-test.js index 156ea3e9d40..92493cfc106 100644 --- a/src/browser/ui/__tests__/ReactMount-test.js +++ b/src/browser/ui/__tests__/ReactMount-test.js @@ -119,9 +119,7 @@ describe('ReactMount', function() { var container = document.createElement('container'); var Component = React.createClass({ render: function() { - return
-
-
; + return
; } }); React.renderComponent(, container); diff --git a/src/browser/ui/__tests__/ReactMountDestruction-test.js b/src/browser/ui/__tests__/ReactMountDestruction-test.js index 743c526fd09..2cafa0a4261 100644 --- a/src/browser/ui/__tests__/ReactMountDestruction-test.js +++ b/src/browser/ui/__tests__/ReactMountDestruction-test.js @@ -56,11 +56,10 @@ describe('ReactMount', function() { it("should warn when unmounting a non-container root node", function() { var mainContainerDiv = document.createElement('div'); - var component = ( + var component =
-
- ); +
; React.renderComponent(component, mainContainerDiv); // Test that unmounting at a root node gives a helpful warning @@ -79,13 +78,12 @@ describe('ReactMount', function() { it("should warn when unmounting a non-container, non-root node", function() { var mainContainerDiv = document.createElement('div'); - var component = ( + var component =
-
- ); +
; React.renderComponent(component, mainContainerDiv); // Test that unmounting at a non-root node gives a different warning From 0323b3ce83b51518e013f7bbe4be3d9da206756a Mon Sep 17 00:00:00 2001 From: Charles Marsh Date: Sat, 23 Aug 2014 10:14:00 +0800 Subject: [PATCH 3/3] Put test components on a single line --- src/browser/ui/__tests__/ReactMountDestruction-test.js | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/browser/ui/__tests__/ReactMountDestruction-test.js b/src/browser/ui/__tests__/ReactMountDestruction-test.js index 2cafa0a4261..56bc1428af7 100644 --- a/src/browser/ui/__tests__/ReactMountDestruction-test.js +++ b/src/browser/ui/__tests__/ReactMountDestruction-test.js @@ -26,18 +26,12 @@ describe('ReactMount', function() { var mainContainerDiv = document.createElement('div'); document.documentElement.appendChild(mainContainerDiv); - var instanceOne = ( -
-
- ); + var instanceOne =
; var firstRootDiv = document.createElement('div'); mainContainerDiv.appendChild(firstRootDiv); React.renderComponent(instanceOne, firstRootDiv); - var instanceTwo = ( -
-
- ); + var instanceTwo =
; var secondRootDiv = document.createElement('div'); mainContainerDiv.appendChild(secondRootDiv); React.renderComponent(instanceTwo, secondRootDiv);