Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ehancement #2365 : Add markup root directly in ReactMount cache #2377

Closed
wants to merge 11 commits into from
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
13 changes: 10 additions & 3 deletions src/browser/ui/ReactDOMIDOperations.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ var ReactDOMIDOperations = {
);

// If we're updating to null or undefined, we should remove the property
// from the DOM node instead of inadvertantly setting to a string. This
// from the DOM node instead of inadvertantly setting to a string. This
// brings us in line with the same behavior we have on initial render.
if (value != null) {
DOMPropertyOperations.setValueForProperty(node, name, value);
Expand Down Expand Up @@ -156,7 +156,10 @@ var ReactDOMIDOperations = {
'dangerouslyReplaceNodeWithMarkupByID',
function(id, markup) {
var node = ReactMount.getNode(id);
DOMChildrenOperations.dangerouslyReplaceNodeWithMarkup(node, markup);
var newNode = DOMChildrenOperations.dangerouslyReplaceNodeWithMarkup(node, markup);
// `getNode` populates ReactMount's node cache with all siblings, but the
// replaced node creates a hole. `getID` fills the hole with the new node.
ReactMount.getID(newNode);
Copy link
Contributor

Choose a reason for hiding this comment

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

One last thing :)
A comment briefly explaining that getID puts the node in the cache and that we do this to avoid holes in the cache (which can otherwise cause all siblings to be unnecessarily re-cached). It would be great info for anyone else stumbling upon this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, it's done !

}
),

Expand All @@ -174,7 +177,11 @@ var ReactDOMIDOperations = {
for (var i = 0; i < updates.length; i++) {
updates[i].parentNode = ReactMount.getNode(updates[i].parentID);
}
DOMChildrenOperations.processUpdates(updates, markup);
var children = DOMChildrenOperations.processUpdates(updates, markup);

for (var i = 0, len = children.length; i < len; i++) {
ReactMount.getID(children[i]);
}
}
)
};
Expand Down
26 changes: 26 additions & 0 deletions src/browser/ui/__tests__/ReactMount-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,4 +108,30 @@ describe('ReactMount', function() {

expect(instance1 === instance2).toBe(true);
});

it('should call findComponentRoot once for each component rendered', function () {
spyOn(ReactMount, "findComponentRoot");

var container = document.createElement('container');
document.documentElement.appendChild(container);

var FindDOMNode = React.createClass({
componentDidMount: function() {
this.getDOMNode();
},
render: function () {
return <span />;
}
});

React.renderComponent(<div><FindDOMNode /></div>, container);

expect(ReactMount.findComponentRoot.callCount).toBe(1);

React.renderComponent(<div><div><FindDOMNode /></ div></div>, container);

expect(ReactMount.findComponentRoot.callCount).toBe(2);

});

});
3 changes: 3 additions & 0 deletions src/browser/ui/dom/DOMChildrenOperations.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ var DOMChildrenOperations = {
*
* @param {array<object>} updates List of update configurations.
* @param {array<string>} markupList List of markup strings.
* @return {array<DOMElement>} List of nodes rendered
* @internal
*/
processUpdates: function(updates, markupList) {
Expand Down Expand Up @@ -164,6 +165,8 @@ var DOMChildrenOperations = {
break;
}
}

return renderedMarkup;
}

};
Expand Down
3 changes: 3 additions & 0 deletions src/browser/ui/dom/Danger.js
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ var Danger = {
*
* @param {DOMElement} oldChild Child node to replace.
* @param {string} markup Markup to render in place of the child node.
* @return {DOMElement} New rendered node
* @internal
*/
dangerouslyReplaceNodeWithMarkup: function(oldChild, markup) {
Expand All @@ -175,6 +176,8 @@ var Danger = {

var newChild = createNodesFromMarkup(markup, emptyFunction)[0];
oldChild.parentNode.replaceChild(newChild, oldChild);

return newChild;
}

};
Expand Down