From 91355c071bb9f839f06cd0bcc3c9bd2a80d3feef Mon Sep 17 00:00:00 2001 From: ThomasCrvsr Date: Sat, 18 Oct 2014 13:18:02 +0200 Subject: [PATCH 01/11] Enhancement #2365 : Add markup root directly in ReactMount cache --- src/browser/ui/ReactDOMIDOperations.js | 3 ++- src/browser/ui/ReactMount.js | 22 ++++++++++++++++++++++ src/browser/ui/dom/Danger.js | 2 ++ 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/browser/ui/ReactDOMIDOperations.js b/src/browser/ui/ReactDOMIDOperations.js index 469d842b393..69635e38995 100644 --- a/src/browser/ui/ReactDOMIDOperations.js +++ b/src/browser/ui/ReactDOMIDOperations.js @@ -156,7 +156,8 @@ var ReactDOMIDOperations = { 'dangerouslyReplaceNodeWithMarkupByID', function(id, markup) { var node = ReactMount.getNode(id); - DOMChildrenOperations.dangerouslyReplaceNodeWithMarkup(node, markup); + var newNode = DOMChildrenOperations.dangerouslyReplaceNodeWithMarkup(node, markup); + ReactMount.cacheNodeById(id, newNode); } ), diff --git a/src/browser/ui/ReactMount.js b/src/browser/ui/ReactMount.js index ff0d3e2b40c..01c5ccfd67d 100644 --- a/src/browser/ui/ReactMount.js +++ b/src/browser/ui/ReactMount.js @@ -130,6 +130,26 @@ function getNode(id) { return nodeCache[id]; } +/** + * Allow to put a node in cache for an existing id without retraversing every sibling nodes + * + * @param {string} id A React-generated DOM ID. + * @param {DOMElement} node DOM node to put in cache + * + */ + +function cacheNodeById(id, node) { + invariant( + node instanceof HTMLElement, + 'ReactMount: invalid component element.', + node + ); + + if (nodeCache.hasOwnProperty(id)) { + nodeCache[id] = node; + } +} + /** * A node is "valid" if it is contained by a currently mounted container. * @@ -674,6 +694,8 @@ var ReactMount = { getNode: getNode, + cacheNodeById: cacheNodeById, + purgeID: purgeID }; diff --git a/src/browser/ui/dom/Danger.js b/src/browser/ui/dom/Danger.js index a4c90c235df..a20ef130d23 100644 --- a/src/browser/ui/dom/Danger.js +++ b/src/browser/ui/dom/Danger.js @@ -175,6 +175,8 @@ var Danger = { var newChild = createNodesFromMarkup(markup, emptyFunction)[0]; oldChild.parentNode.replaceChild(newChild, oldChild); + + return newChild; } }; From e8f80b68bf6d1927e9544f7fcb27316da9f463d7 Mon Sep 17 00:00:00 2001 From: ThomasCrvsr Date: Sat, 18 Oct 2014 13:20:41 +0200 Subject: [PATCH 02/11] Enhancement #2365 : Add markup root directly in ReactMount cache --- src/browser/ui/ReactMount.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/browser/ui/ReactMount.js b/src/browser/ui/ReactMount.js index 01c5ccfd67d..ad99a4c4802 100644 --- a/src/browser/ui/ReactMount.js +++ b/src/browser/ui/ReactMount.js @@ -141,8 +141,7 @@ function getNode(id) { function cacheNodeById(id, node) { invariant( node instanceof HTMLElement, - 'ReactMount: invalid component element.', - node + 'ReactMount: invalid component element.' ); if (nodeCache.hasOwnProperty(id)) { From 0ed5ddc86342fb752036d4a1f5846c017e7128c9 Mon Sep 17 00:00:00 2001 From: ThomasCrvsr Date: Sat, 18 Oct 2014 14:24:00 +0200 Subject: [PATCH 03/11] Enhancement #2365 : use ReactMount.getID on newNode --- src/browser/ui/ReactDOMIDOperations.js | 4 ++-- src/browser/ui/ReactMount.js | 19 ------------------- 2 files changed, 2 insertions(+), 21 deletions(-) diff --git a/src/browser/ui/ReactDOMIDOperations.js b/src/browser/ui/ReactDOMIDOperations.js index 69635e38995..a0a2a47852b 100644 --- a/src/browser/ui/ReactDOMIDOperations.js +++ b/src/browser/ui/ReactDOMIDOperations.js @@ -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); @@ -157,7 +157,7 @@ var ReactDOMIDOperations = { function(id, markup) { var node = ReactMount.getNode(id); var newNode = DOMChildrenOperations.dangerouslyReplaceNodeWithMarkup(node, markup); - ReactMount.cacheNodeById(id, newNode); + ReactMount.getID(newNode); } ), diff --git a/src/browser/ui/ReactMount.js b/src/browser/ui/ReactMount.js index ad99a4c4802..5e22ac94748 100644 --- a/src/browser/ui/ReactMount.js +++ b/src/browser/ui/ReactMount.js @@ -130,25 +130,6 @@ function getNode(id) { return nodeCache[id]; } -/** - * Allow to put a node in cache for an existing id without retraversing every sibling nodes - * - * @param {string} id A React-generated DOM ID. - * @param {DOMElement} node DOM node to put in cache - * - */ - -function cacheNodeById(id, node) { - invariant( - node instanceof HTMLElement, - 'ReactMount: invalid component element.' - ); - - if (nodeCache.hasOwnProperty(id)) { - nodeCache[id] = node; - } -} - /** * A node is "valid" if it is contained by a currently mounted container. * From 281ca7d46987de8fbfe53510a4752505fdbf1a38 Mon Sep 17 00:00:00 2001 From: ThomasCrvsr Date: Sat, 18 Oct 2014 14:26:50 +0200 Subject: [PATCH 04/11] Enhancement #2365 : remove cacheNodeById in ReactMount --- src/browser/ui/ReactMount.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/browser/ui/ReactMount.js b/src/browser/ui/ReactMount.js index 5e22ac94748..ff0d3e2b40c 100644 --- a/src/browser/ui/ReactMount.js +++ b/src/browser/ui/ReactMount.js @@ -674,8 +674,6 @@ var ReactMount = { getNode: getNode, - cacheNodeById: cacheNodeById, - purgeID: purgeID }; From 04f551236f830aef49178a46dffe6cc07347182b Mon Sep 17 00:00:00 2001 From: ThomasCrvsr Date: Sun, 19 Oct 2014 15:22:21 +0200 Subject: [PATCH 05/11] Enhancement #2365 : use getID on children in dangerouslyProcessChildrenUpdate --- src/browser/ui/ReactDOMIDOperations.js | 5 ++++- src/browser/ui/dom/DOMChildrenOperations.js | 3 +++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/browser/ui/ReactDOMIDOperations.js b/src/browser/ui/ReactDOMIDOperations.js index a0a2a47852b..1e59e3a6d1c 100644 --- a/src/browser/ui/ReactDOMIDOperations.js +++ b/src/browser/ui/ReactDOMIDOperations.js @@ -175,7 +175,10 @@ 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]); + } } ) }; diff --git a/src/browser/ui/dom/DOMChildrenOperations.js b/src/browser/ui/dom/DOMChildrenOperations.js index 0a566e5c00c..7c6f45307e4 100644 --- a/src/browser/ui/dom/DOMChildrenOperations.js +++ b/src/browser/ui/dom/DOMChildrenOperations.js @@ -91,6 +91,7 @@ var DOMChildrenOperations = { * * @param {array} updates List of update configurations. * @param {array} markupList List of markup strings. + * @return {array} renderedMarkup List of nodes rendered * @internal */ processUpdates: function(updates, markupList) { @@ -164,6 +165,8 @@ var DOMChildrenOperations = { break; } } + + return renderedMarkup; } }; From 48a907f17688630a7d2706613f5065cde37a03ea Mon Sep 17 00:00:00 2001 From: ThomasCrvsr Date: Sun, 19 Oct 2014 20:28:14 +0200 Subject: [PATCH 06/11] Enhancement #2365 : add @return comment for Danger.dangerouslyReplaceNodeWithMarkup --- src/browser/ui/dom/DOMChildrenOperations.js | 2 +- src/browser/ui/dom/Danger.js | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/browser/ui/dom/DOMChildrenOperations.js b/src/browser/ui/dom/DOMChildrenOperations.js index 7c6f45307e4..ecd094ea324 100644 --- a/src/browser/ui/dom/DOMChildrenOperations.js +++ b/src/browser/ui/dom/DOMChildrenOperations.js @@ -91,7 +91,7 @@ var DOMChildrenOperations = { * * @param {array} updates List of update configurations. * @param {array} markupList List of markup strings. - * @return {array} renderedMarkup List of nodes rendered + * @return {array} List of nodes rendered * @internal */ processUpdates: function(updates, markupList) { diff --git a/src/browser/ui/dom/Danger.js b/src/browser/ui/dom/Danger.js index a20ef130d23..964e0cb1124 100644 --- a/src/browser/ui/dom/Danger.js +++ b/src/browser/ui/dom/Danger.js @@ -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) { From 3939877631888c61d6e88438b6bde2d38da8e275 Mon Sep 17 00:00:00 2001 From: ThomasCrvsr Date: Sun, 19 Oct 2014 23:52:45 +0200 Subject: [PATCH 07/11] Enhancement #2365 : add regression test for ReactMount.findComponentRoot --- src/browser/ui/__tests__/ReactMount-test.js | 23 +++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/browser/ui/__tests__/ReactMount-test.js b/src/browser/ui/__tests__/ReactMount-test.js index 79701ec6668..30f71d13974 100644 --- a/src/browser/ui/__tests__/ReactMount-test.js +++ b/src/browser/ui/__tests__/ReactMount-test.js @@ -108,4 +108,27 @@ 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 ; + } + }); + + React.renderComponent(
, container); + React.renderComponent(
, container); + + expect(ReactMount.findComponentRoot.callCount).toBe(2); + + }); + }); From 9fe5fc63c4e19e7287b529d0a1b0936f98eaecd9 Mon Sep 17 00:00:00 2001 From: ThomasCrvsr Date: Mon, 20 Oct 2014 00:02:13 +0200 Subject: [PATCH 08/11] Enhancement #2365 : expect ReactMount.findComponentRoot to be called once for each element rendered --- src/browser/ui/__tests__/ReactMount-test.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/browser/ui/__tests__/ReactMount-test.js b/src/browser/ui/__tests__/ReactMount-test.js index 30f71d13974..f76a2243ba8 100644 --- a/src/browser/ui/__tests__/ReactMount-test.js +++ b/src/browser/ui/__tests__/ReactMount-test.js @@ -125,6 +125,9 @@ describe('ReactMount', function() { }); React.renderComponent(
, container); + + expect(ReactMount.findComponentRoot.callCount).toBe(1); + React.renderComponent(
, container); expect(ReactMount.findComponentRoot.callCount).toBe(2); From 579ae93b6996110fd6467f4ab960552a9ff2c94b Mon Sep 17 00:00:00 2001 From: ThomasCrvsr Date: Mon, 20 Oct 2014 09:48:54 +0200 Subject: [PATCH 09/11] Enhancement #2365 : add comment explaining why using getID --- src/browser/ui/ReactDOMIDOperations.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/browser/ui/ReactDOMIDOperations.js b/src/browser/ui/ReactDOMIDOperations.js index 1e59e3a6d1c..830cac5a023 100644 --- a/src/browser/ui/ReactDOMIDOperations.js +++ b/src/browser/ui/ReactDOMIDOperations.js @@ -157,6 +157,8 @@ var ReactDOMIDOperations = { function(id, markup) { var node = ReactMount.getNode(id); var newNode = DOMChildrenOperations.dangerouslyReplaceNodeWithMarkup(node, markup); + // As getID put the node in ReactMount cache, we use it to put newNode in the cache + // without recache every sibling nodes ReactMount.getID(newNode); } ), @@ -176,6 +178,8 @@ var ReactDOMIDOperations = { updates[i].parentNode = ReactMount.getNode(updates[i].parentID); } var children = DOMChildrenOperations.processUpdates(updates, markup); + + // Use of getID to put in ReactMount cache every children re-rendered for (var i = 0, len = children.length; i < len; i++) { ReactMount.getID(children[i]); } From a27dbf8ac7a98a0704cad166cae38065110727dc Mon Sep 17 00:00:00 2001 From: ThomasCrvsr Date: Mon, 20 Oct 2014 10:25:12 +0200 Subject: [PATCH 10/11] Enhancement #2365 : remove comment on dangerouslyProcessChildrenUpdates --- src/browser/ui/ReactDOMIDOperations.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/browser/ui/ReactDOMIDOperations.js b/src/browser/ui/ReactDOMIDOperations.js index 830cac5a023..90c6285c68d 100644 --- a/src/browser/ui/ReactDOMIDOperations.js +++ b/src/browser/ui/ReactDOMIDOperations.js @@ -179,7 +179,6 @@ var ReactDOMIDOperations = { } var children = DOMChildrenOperations.processUpdates(updates, markup); - // Use of getID to put in ReactMount cache every children re-rendered for (var i = 0, len = children.length; i < len; i++) { ReactMount.getID(children[i]); } From e8aed5c0539e981060b92af5875f5df422a6cb4e Mon Sep 17 00:00:00 2001 From: ThomasCrvsr Date: Mon, 20 Oct 2014 10:57:55 +0200 Subject: [PATCH 11/11] Enhancement #2365 : modify comment on dangerouslyReplaceNodeWithMarkupByID --- src/browser/ui/ReactDOMIDOperations.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/browser/ui/ReactDOMIDOperations.js b/src/browser/ui/ReactDOMIDOperations.js index 90c6285c68d..7685d9a6c41 100644 --- a/src/browser/ui/ReactDOMIDOperations.js +++ b/src/browser/ui/ReactDOMIDOperations.js @@ -157,8 +157,8 @@ var ReactDOMIDOperations = { function(id, markup) { var node = ReactMount.getNode(id); var newNode = DOMChildrenOperations.dangerouslyReplaceNodeWithMarkup(node, markup); - // As getID put the node in ReactMount cache, we use it to put newNode in the cache - // without recache every sibling nodes + // `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); } ),