Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 52 additions & 1 deletion src/browser/ui/ReactMount.js
Original file line number Diff line number Diff line change
Expand Up @@ -370,8 +370,21 @@ 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.'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a "If you intended to update the children of this node, you should instead have the component that rendered the existing children update its state and render the new components instead of calling the top-level React.render to update a subtree."

Also, s/renderComponent/render/. :)

);
}

var shouldReuseMarkup =
containerHasReactMarkup &&
!prevComponent &&
!containerHasNonRootReactChild;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one gets even more awkward. I tend to like a style here that we don't really do in FB, so here's what we should probably do:

    var shouldReuseMarkup =
      containerHasReactMarkup &&
      !prevComponent &&
      !containerHasNonRootReactChild;


var component = ReactMount._renderNewRootComponent(
nextDescriptor,
Expand Down Expand Up @@ -459,6 +472,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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nit: this should be

var containerHasNonRootReactChild =
  ReactMount.hasNonRootReactChild(container);

or

var containerHasNonRootReactChild = ReactMount.hasNonRootReactChild(
  container
);

(twice in this file)


// 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',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe "was rendered by React and cannot be unmounted using unmountComponentAtNode."? In the case that it's not a root, we could also add a "Instead, have the parent component update its state and rerender to remove this component." or similar.

(isContainerReactRoot ? ' You may have passed in a React root ' +
'node as argument, rather than its container.' : '')
);
}

return false;
}
ReactMount.unmountComponentFromNode(component, container);
Expand Down Expand Up @@ -560,6 +595,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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function looks good, but can you move it to just be a free function at the top of this file? no reason it needs to be on the ReactMount object.

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.
Expand Down
20 changes: 20 additions & 0 deletions src/browser/ui/__tests__/ReactMount-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,4 +114,24 @@ 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 <div><div /></div>;
}
});
React.renderComponent(<Component />, container);

// Test that blasting away children throws a warning
spyOn(console, 'warn');
var rootNode = container.firstChild;
React.renderComponent(<span />, 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.'
);
});
});
55 changes: 47 additions & 8 deletions src/browser/ui/__tests__/ReactMountDestruction-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,18 +26,12 @@ describe('ReactMount', function() {
var mainContainerDiv = document.createElement('div');
document.documentElement.appendChild(mainContainerDiv);

var instanceOne = (
<div className="firstReactDiv">
</div>
);
var instanceOne = <div className="firstReactDiv" />;
var firstRootDiv = document.createElement('div');
mainContainerDiv.appendChild(firstRootDiv);
React.renderComponent(instanceOne, firstRootDiv);

var instanceTwo = (
<div className="secondReactDiv">
</div>
);
var instanceTwo = <div className="secondReactDiv" />;
var secondRootDiv = document.createElement('div');
mainContainerDiv.appendChild(secondRootDiv);
React.renderComponent(instanceTwo, secondRootDiv);
Expand All @@ -52,4 +46,49 @@ 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 =
<div>
<div />
</div>;
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 =
<div>
<div>
<div />
</div>
</div>;
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.'
);
});
});