From 8eb192b92f78ca10a2ed840484e871cf5cfdacc8 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 8 Dec 2016 20:19:52 +0000 Subject: [PATCH 01/13] Enable additional (failing) portal tests --- scripts/fiber/tests-failing.txt | 4 +++ scripts/fiber/tests-passing.txt | 2 -- .../dom/fiber/__tests__/ReactDOMFiber-test.js | 25 ++++++++++++------- 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 8d705c30e8353..04626aca6a312 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -18,6 +18,10 @@ src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js src/renderers/dom/__tests__/ReactDOMProduction-test.js * should throw with an error code in production +src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js +* should keep track of namespace across portals (medium) +* should unwind namespaces on caught errors in a portal + src/renderers/dom/shared/__tests__/ReactDOM-test.js * throws in render() if the mount callback is not a function * throws in render() if the update callback is not a function diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 7143e78caac9a..119983286a6ef 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -515,11 +515,9 @@ src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js * should render many portals * should render nested portals * should keep track of namespace across portals (simple) -* should keep track of namespace across portals (medium) * should keep track of namespace across portals (complex) * should unwind namespaces on uncaught errors * should unwind namespaces on caught errors -* should unwind namespaces on caught errors in a portal * should pass portal context when rendering subtree elsewhere * should update portal context if it changes due to setState * should update portal context if it changes due to re-render diff --git a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js index 29f01844bc2a1..fd20ef8a8c0fa 100644 --- a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js +++ b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js @@ -400,6 +400,19 @@ describe('ReactDOMFiber', () => { }); it('should keep track of namespace across portals (medium)', () => { + assertNamespacesMatch( + + + {usePortal( +
+ )} + + {usePortal( +
+ )} + + + ); assertNamespacesMatch(
@@ -613,15 +626,9 @@ describe('ReactDOMFiber', () => {
)} - { - /* - * TODO: enable. Currently this leads to stack overflow - * but it might be a bug in error boundaries rather than SVG or portals. - usePortal( -
- ) - */ - } + {usePortal( +
+ )} ); }); From 3ba8069fefed06dae0e3be74b9c42734752e1580 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 9 Dec 2016 15:17:17 +0000 Subject: [PATCH 02/13] Fix portal unmounting When unmount a portal, we need to unmount *its* children from itself. This is similar to what we would do for a root if we allowed deleting roots. --- scripts/fiber/tests-failing.txt | 1 - scripts/fiber/tests-passing.txt | 1 + .../shared/fiber/ReactFiberCommitWork.js | 23 +++++++++++++++---- 3 files changed, 20 insertions(+), 5 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 04626aca6a312..e0ccac276681e 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -19,7 +19,6 @@ src/renderers/dom/__tests__/ReactDOMProduction-test.js * should throw with an error code in production src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js -* should keep track of namespace across portals (medium) * should unwind namespaces on caught errors in a portal src/renderers/dom/shared/__tests__/ReactDOM-test.js diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 119983286a6ef..da2bae16f6a60 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -515,6 +515,7 @@ src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js * should render many portals * should render nested portals * should keep track of namespace across portals (simple) +* should keep track of namespace across portals (medium) * should keep track of namespace across portals (complex) * should unwind namespaces on uncaught errors * should unwind namespaces on caught errors diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index 5e31b11fcc12b..5432935624e03 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js @@ -265,16 +265,19 @@ module.exports = function( // After all the children have unmounted, it is now safe to remove the // node from the tree. removeChild(parent, node.stateNode); + // Don't visit children because we already visited them. } else if (node.tag === HostPortal) { // When we go into a portal, it becomes the parent to remove from. // We will reassign it back when we pop the portal on the way up. parent = node.stateNode.containerInfo; + // Visit children because portals might contain host components. if (node.child) { node = node.child; continue; } } else { commitUnmount(node); + // Visit children because we may find more host components below. if (node.child) { // TODO: Coroutines need to visit the stateNode. node.child.return = node; @@ -302,10 +305,22 @@ module.exports = function( } function commitDeletion(current : Fiber) : void { - // Recursively delete all host nodes from the parent. - const parent = getHostParent(current); - // Detach refs and call componentWillUnmount() on the whole subtree. - unmountHostComponents(parent, current); + // Recursively delete the closest child host nodes from the closest host parent. + // Then detach refs and call componentWillUnmount() on the whole subtree. + if (current.tag === HostPortal) { + // When deleting a portal, there are no host nodes above it. + // It is as a host parent itself. + const parent = current.stateNode.containerInfo; + let child = current.child; + while (child) { + unmountHostComponents(parent, child); + child = child.sibling; + } + } else { + // When deleting anything other than a portal, search for the host parent. + const parent = getHostParent(current); + unmountHostComponents(parent, current); + } // Cut off the return pointers to disconnect it from the tree. Ideally, we // should clear the child pointer of the parent alternate to let this From 4596fc48ba371237ef83868d411fe684e1c877df Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 9 Dec 2016 15:48:39 +0000 Subject: [PATCH 03/13] Skip portals when looking for host siblings A portal is not part of that host tree despite being a child. --- scripts/fiber/tests-failing.txt | 3 --- scripts/fiber/tests-passing.txt | 1 + src/renderers/shared/fiber/ReactFiberCommitWork.js | 2 +- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index e0ccac276681e..8d705c30e8353 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -18,9 +18,6 @@ src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js src/renderers/dom/__tests__/ReactDOMProduction-test.js * should throw with an error code in production -src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js -* should unwind namespaces on caught errors in a portal - src/renderers/dom/shared/__tests__/ReactDOM-test.js * throws in render() if the mount callback is not a function * throws in render() if the update callback is not a function diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index da2bae16f6a60..7143e78caac9a 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -519,6 +519,7 @@ src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js * should keep track of namespace across portals (complex) * should unwind namespaces on uncaught errors * should unwind namespaces on caught errors +* should unwind namespaces on caught errors in a portal * should pass portal context when rendering subtree elsewhere * should update portal context if it changes due to setState * should update portal context if it changes due to re-render diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index 5432935624e03..379c226b6695d 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js @@ -152,7 +152,7 @@ module.exports = function( // If we don't have a child, try the siblings instead. continue siblings; } - if (!node.child) { + if (!node.child || node.tag === HostPortal) { continue siblings; } else { node.child.return = node; From cb8184f7b068f75071e62e858272c57a7c31b6f0 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 9 Dec 2016 16:18:45 +0000 Subject: [PATCH 04/13] Fix comment typo --- src/renderers/shared/fiber/ReactFiberCommitWork.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index 379c226b6695d..393cc8d47166d 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js @@ -309,7 +309,7 @@ module.exports = function( // Then detach refs and call componentWillUnmount() on the whole subtree. if (current.tag === HostPortal) { // When deleting a portal, there are no host nodes above it. - // It is as a host parent itself. + // It is a host parent itself. const parent = current.stateNode.containerInfo; let child = current.child; while (child) { From 3f2129aeee8ba11ca803f68395a6978a8c8cf3b3 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 9 Dec 2016 18:41:35 +0000 Subject: [PATCH 05/13] Add a failing test for portal child reconciliation It is failing because portal bails out of update, seeing null in pendingProps. It is null because we set pendingProps to nextPortal.children, which is null in this test. --- scripts/fiber/tests-failing.txt | 3 + .../dom/fiber/__tests__/ReactDOMFiber-test.js | 76 +++++++++++++++++++ 2 files changed, 79 insertions(+) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 8d705c30e8353..8db2ea6703a63 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -18,6 +18,9 @@ src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js src/renderers/dom/__tests__/ReactDOMProduction-test.js * should throw with an error code in production +src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js +* should reconcile portal children + src/renderers/dom/shared/__tests__/ReactDOM-test.js * throws in render() if the mount callback is not a function * throws in render() if the update callback is not a function diff --git a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js index fd20ef8a8c0fa..6dd3146541107 100644 --- a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js +++ b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js @@ -367,6 +367,82 @@ describe('ReactDOMFiber', () => { expect(container.innerHTML).toBe(''); }); + it('should reconcile portal children', () => { + var portalContainer = document.createElement('div'); + + ReactDOM.render( +
+ {ReactDOM.unstable_createPortal( +
portal:1
, + portalContainer + )} +
, + container + ); + expect(portalContainer.innerHTML).toBe('
portal:1
'); + expect(container.innerHTML).toBe('
'); + + ReactDOM.render( +
+ {ReactDOM.unstable_createPortal( +
portal:2
, + portalContainer + )} +
, + container + ); + expect(portalContainer.innerHTML).toBe('
portal:2
'); + expect(container.innerHTML).toBe('
'); + + ReactDOM.render( +
+ {ReactDOM.unstable_createPortal( +

portal:3

, + portalContainer + )} +
, + container + ); + expect(portalContainer.innerHTML).toBe('

portal:3

'); + expect(container.innerHTML).toBe('
'); + + ReactDOM.render( +
+ {ReactDOM.unstable_createPortal( + ['Hi', 'Bye'], + portalContainer + )} +
, + container + ); + expect(portalContainer.innerHTML).toBe('HiBye'); + expect(container.innerHTML).toBe('
'); + + ReactDOM.render( +
+ {ReactDOM.unstable_createPortal( + ['Bye', 'Hi'], + portalContainer + )} +
, + container + ); + expect(portalContainer.innerHTML).toBe('ByeHi'); + expect(container.innerHTML).toBe('
'); + + ReactDOM.render( +
+ {ReactDOM.unstable_createPortal( + null, + portalContainer + )} +
, + container + ); + expect(portalContainer.innerHTML).toBe(''); + expect(container.innerHTML).toBe('
'); + }); + it('should keep track of namespace across portals (simple)', () => { assertNamespacesMatch( From ed9747deedf11455ba3eb65648007ab99c26ea58 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 9 Dec 2016 18:45:34 +0000 Subject: [PATCH 06/13] Fix the bug when switching to a null portal child If pendingProps is null, we do a bailout in beginWork. This prevents unmounting of the existing child when the new child is null. We fix this by changing portal fiber's pendingProps to be the portal object itself instead of its children. This way, it is never null, and thus doesn't cause a false positive in the bailout condition. --- scripts/fiber/tests-failing.txt | 3 --- scripts/fiber/tests-passing.txt | 1 + src/renderers/shared/fiber/ReactChildFiber.js | 4 ++-- src/renderers/shared/fiber/ReactFiber.js | 2 +- src/renderers/shared/fiber/ReactFiberBeginWork.js | 2 +- 5 files changed, 5 insertions(+), 7 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 8db2ea6703a63..8d705c30e8353 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -18,9 +18,6 @@ src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js src/renderers/dom/__tests__/ReactDOMProduction-test.js * should throw with an error code in production -src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js -* should reconcile portal children - src/renderers/dom/shared/__tests__/ReactDOM-test.js * throws in render() if the mount callback is not a function * throws in render() if the update callback is not a function diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 7143e78caac9a..6eca915bf16b1 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -514,6 +514,7 @@ src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js * should render one portal * should render many portals * should render nested portals +* should reconcile portal children * should keep track of namespace across portals (simple) * should keep track of namespace across portals (medium) * should keep track of namespace across portals (complex) diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index 413fd234f6ab7..175dded7c0c1a 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -341,7 +341,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { } else { // Update const existing = useFiber(current, priority); - existing.pendingProps = portal.children; + existing.pendingProps = portal; existing.return = returnFiber; return existing; } @@ -976,7 +976,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { ) { deleteRemainingChildren(returnFiber, child.sibling); const existing = useFiber(child, priority); - existing.pendingProps = portal.children; + existing.pendingProps = portal; existing.return = returnFiber; return existing; } else { diff --git a/src/renderers/shared/fiber/ReactFiber.js b/src/renderers/shared/fiber/ReactFiber.js index 98e47ecaab3f1..e6badf8e93c30 100644 --- a/src/renderers/shared/fiber/ReactFiber.js +++ b/src/renderers/shared/fiber/ReactFiber.js @@ -346,7 +346,7 @@ exports.createFiberFromYield = function(yieldNode : ReactYield, priorityLevel : exports.createFiberFromPortal = function(portal : ReactPortal, priorityLevel : PriorityLevel) : Fiber { const fiber = createFiber(HostPortal, portal.key); - fiber.pendingProps = portal.children; + fiber.pendingProps = portal; fiber.pendingWorkPriority = priorityLevel; fiber.stateNode = { containerInfo: portal.containerInfo, diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index ca68c0d01bab6..62447db5c3f6d 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -325,7 +325,7 @@ module.exports = function( function updatePortalComponent(current, workInProgress) { const priorityLevel = workInProgress.pendingWorkPriority; - const nextChildren = workInProgress.pendingProps; + const nextChildren = workInProgress.pendingProps.children; if (!current) { // Portals are special because we don't append the children during mount // but at commit. Therefore we need to track insertions which the normal From c69be1c5f499e24dd08878df612321edc6a8135f Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 9 Dec 2016 20:32:54 +0000 Subject: [PATCH 07/13] Add a comment about HostPortal in getHostSibling --- src/renderers/shared/fiber/ReactFiberCommitWork.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index 393cc8d47166d..5ba206f7be538 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js @@ -152,6 +152,8 @@ module.exports = function( // If we don't have a child, try the siblings instead. continue siblings; } + // If we don't have a child, try the siblings instead. + // We also skip portals because they are not part of this host tree. if (!node.child || node.tag === HostPortal) { continue siblings; } else { From 4fe06f50bef3216b90ead0efbba27bb8cca2dfdd Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Sat, 10 Dec 2016 02:12:39 +0000 Subject: [PATCH 08/13] Revert the fix because I don't know why it worked unmountHostComponents() should have worked despite finding the wrong parent because it should have changed the parent when pushing and popping the portals. --- scripts/fiber/tests-failing.txt | 3 +++ scripts/fiber/tests-passing.txt | 1 - .../shared/fiber/ReactFiberCommitWork.js | 20 ++++--------------- 3 files changed, 7 insertions(+), 17 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 8d705c30e8353..0d32841280e09 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -18,6 +18,9 @@ src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js src/renderers/dom/__tests__/ReactDOMProduction-test.js * should throw with an error code in production +src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js +* should keep track of namespace across portals (medium) + src/renderers/dom/shared/__tests__/ReactDOM-test.js * throws in render() if the mount callback is not a function * throws in render() if the update callback is not a function diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 6eca915bf16b1..d46024f5b5029 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -516,7 +516,6 @@ src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js * should render nested portals * should reconcile portal children * should keep track of namespace across portals (simple) -* should keep track of namespace across portals (medium) * should keep track of namespace across portals (complex) * should unwind namespaces on uncaught errors * should unwind namespaces on caught errors diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index 5ba206f7be538..020ace283540c 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js @@ -307,22 +307,10 @@ module.exports = function( } function commitDeletion(current : Fiber) : void { - // Recursively delete the closest child host nodes from the closest host parent. - // Then detach refs and call componentWillUnmount() on the whole subtree. - if (current.tag === HostPortal) { - // When deleting a portal, there are no host nodes above it. - // It is a host parent itself. - const parent = current.stateNode.containerInfo; - let child = current.child; - while (child) { - unmountHostComponents(parent, child); - child = child.sibling; - } - } else { - // When deleting anything other than a portal, search for the host parent. - const parent = getHostParent(current); - unmountHostComponents(parent, current); - } + // Recursively delete all host nodes from the parent. + const parent = getHostParent(current); + // Detach refs and call componentWillUnmount() on the whole subtree. + unmountHostComponents(parent, current); // Cut off the return pointers to disconnect it from the tree. Ideally, we // should clear the child pointer of the parent alternate to let this From 8b65c5645fcc0ec06ede99687b5bb62f597d35d3 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Sat, 10 Dec 2016 03:03:09 +0000 Subject: [PATCH 09/13] Don't call commitDeletion recursively This leads to a "Cannot find host parent" bug because commitDeletion() clears the return field. When we're inside the loop, we assign node.sibling.return to node.return but by this moment node.return has already been nulled. As a solution we inline code from commitDeletion() without the nulling. It still fails tests but for a different reason (unrelated bug). --- scripts/fiber/tests-failing.txt | 1 + scripts/fiber/tests-passing.txt | 1 - src/renderers/shared/fiber/ReactFiberCommitWork.js | 5 ++++- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 0d32841280e09..8859881c0f727 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -20,6 +20,7 @@ src/renderers/dom/__tests__/ReactDOMProduction-test.js src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js * should keep track of namespace across portals (medium) +* should keep track of namespace across portals (complex) src/renderers/dom/shared/__tests__/ReactDOM-test.js * throws in render() if the mount callback is not a function diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index d46024f5b5029..4a3eed5713867 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -516,7 +516,6 @@ src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js * should render nested portals * should reconcile portal children * should keep track of namespace across portals (simple) -* should keep track of namespace across portals (complex) * should unwind namespaces on uncaught errors * should unwind namespaces on caught errors * should unwind namespaces on caught errors in a portal diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index 020ace283540c..1504da5feb193 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js @@ -348,7 +348,10 @@ module.exports = function( } case HostPortal: { // TODO: this is recursive. - commitDeletion(current); + // We are also not using this parent because + // the portal will get pushed immediately. + const parent = getHostParent(current); + unmountHostComponents(parent, current); return; } } From 9db4d21cb466ea72e1c6208e94057f87403295ae Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Sat, 10 Dec 2016 03:13:51 +0000 Subject: [PATCH 10/13] Skip portal children in commitNestedUnmounts() We are currently already visiting them in commitUnmount() portal case since it's recursive. This condition avoids visting them twice. --- scripts/fiber/tests-failing.txt | 4 ---- scripts/fiber/tests-passing.txt | 2 ++ src/renderers/shared/fiber/ReactFiberCommitWork.js | 4 +++- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 8859881c0f727..8d705c30e8353 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -18,10 +18,6 @@ src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js src/renderers/dom/__tests__/ReactDOMProduction-test.js * should throw with an error code in production -src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js -* should keep track of namespace across portals (medium) -* should keep track of namespace across portals (complex) - src/renderers/dom/shared/__tests__/ReactDOM-test.js * throws in render() if the mount callback is not a function * throws in render() if the update callback is not a function diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 4a3eed5713867..6eca915bf16b1 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -516,6 +516,8 @@ src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js * should render nested portals * should reconcile portal children * should keep track of namespace across portals (simple) +* should keep track of namespace across portals (medium) +* should keep track of namespace across portals (complex) * should unwind namespaces on uncaught errors * should unwind namespaces on caught errors * should unwind namespaces on caught errors in a portal diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index 1504da5feb193..067f67088d4ad 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js @@ -237,7 +237,9 @@ module.exports = function( let node : Fiber = root; while (true) { commitUnmount(node); - if (node.child) { + // Visit children because they may contain more composite or host nodes. + // Skip portals because commitUnmount() currently visits them recursively. + if (node.child && node.tag !== HostPortal) { // TODO: Coroutines need to visit the stateNode. node.child.return = node; node = node.child; From eb5636d96cd5b320f1c7bd67c7428ab88d140091 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Sat, 10 Dec 2016 03:17:35 +0000 Subject: [PATCH 11/13] Set node.child.return before going deeper It doesn't seem to influence existing tests but we have this protection in all other similar places. It protects against infinite loops. --- src/renderers/shared/fiber/ReactFiberCommitWork.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index 067f67088d4ad..46f0fdc94b560 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js @@ -276,6 +276,7 @@ module.exports = function( parent = node.stateNode.containerInfo; // Visit children because portals might contain host components. if (node.child) { + node.child.return = node; node = node.child; continue; } From a0ce225a7056573f7012597803d5598798834dde Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Sat, 10 Dec 2016 03:24:25 +0000 Subject: [PATCH 12/13] Revert "Fix the bug when switching to a null portal child" This reverts commit ed9747deedf11455ba3eb65648007ab99c26ea58. I'll solve this by using an array in place of null instead. --- scripts/fiber/tests-failing.txt | 3 +++ scripts/fiber/tests-passing.txt | 1 - src/renderers/shared/fiber/ReactChildFiber.js | 4 ++-- src/renderers/shared/fiber/ReactFiber.js | 2 +- src/renderers/shared/fiber/ReactFiberBeginWork.js | 2 +- 5 files changed, 7 insertions(+), 5 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 8d705c30e8353..8db2ea6703a63 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -18,6 +18,9 @@ src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js src/renderers/dom/__tests__/ReactDOMProduction-test.js * should throw with an error code in production +src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js +* should reconcile portal children + src/renderers/dom/shared/__tests__/ReactDOM-test.js * throws in render() if the mount callback is not a function * throws in render() if the update callback is not a function diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 6eca915bf16b1..7143e78caac9a 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -514,7 +514,6 @@ src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js * should render one portal * should render many portals * should render nested portals -* should reconcile portal children * should keep track of namespace across portals (simple) * should keep track of namespace across portals (medium) * should keep track of namespace across portals (complex) diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index 175dded7c0c1a..413fd234f6ab7 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -341,7 +341,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { } else { // Update const existing = useFiber(current, priority); - existing.pendingProps = portal; + existing.pendingProps = portal.children; existing.return = returnFiber; return existing; } @@ -976,7 +976,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { ) { deleteRemainingChildren(returnFiber, child.sibling); const existing = useFiber(child, priority); - existing.pendingProps = portal; + existing.pendingProps = portal.children; existing.return = returnFiber; return existing; } else { diff --git a/src/renderers/shared/fiber/ReactFiber.js b/src/renderers/shared/fiber/ReactFiber.js index e6badf8e93c30..98e47ecaab3f1 100644 --- a/src/renderers/shared/fiber/ReactFiber.js +++ b/src/renderers/shared/fiber/ReactFiber.js @@ -346,7 +346,7 @@ exports.createFiberFromYield = function(yieldNode : ReactYield, priorityLevel : exports.createFiberFromPortal = function(portal : ReactPortal, priorityLevel : PriorityLevel) : Fiber { const fiber = createFiber(HostPortal, portal.key); - fiber.pendingProps = portal; + fiber.pendingProps = portal.children; fiber.pendingWorkPriority = priorityLevel; fiber.stateNode = { containerInfo: portal.containerInfo, diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index 62447db5c3f6d..ca68c0d01bab6 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -325,7 +325,7 @@ module.exports = function( function updatePortalComponent(current, workInProgress) { const priorityLevel = workInProgress.pendingWorkPriority; - const nextChildren = workInProgress.pendingProps.children; + const nextChildren = workInProgress.pendingProps; if (!current) { // Portals are special because we don't append the children during mount // but at commit. Therefore we need to track insertions which the normal From 9ccd1add4f3c7fa57ab7e0980efdeed09890bec5 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Sat, 10 Dec 2016 03:27:03 +0000 Subject: [PATCH 13/13] Use [] for empty Portal pendingProps This avoids a false positive bailout with pendingProps == null when portal is empty. --- scripts/fiber/tests-failing.txt | 3 --- scripts/fiber/tests-passing.txt | 1 + src/renderers/shared/fiber/ReactChildFiber.js | 4 ++-- src/renderers/shared/fiber/ReactFiber.js | 2 +- 4 files changed, 4 insertions(+), 6 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 8db2ea6703a63..8d705c30e8353 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -18,9 +18,6 @@ src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js src/renderers/dom/__tests__/ReactDOMProduction-test.js * should throw with an error code in production -src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js -* should reconcile portal children - src/renderers/dom/shared/__tests__/ReactDOM-test.js * throws in render() if the mount callback is not a function * throws in render() if the update callback is not a function diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 7143e78caac9a..6eca915bf16b1 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -514,6 +514,7 @@ src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js * should render one portal * should render many portals * should render nested portals +* should reconcile portal children * should keep track of namespace across portals (simple) * should keep track of namespace across portals (medium) * should keep track of namespace across portals (complex) diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index 413fd234f6ab7..e7ee06f88c41f 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -341,7 +341,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { } else { // Update const existing = useFiber(current, priority); - existing.pendingProps = portal.children; + existing.pendingProps = portal.children || []; existing.return = returnFiber; return existing; } @@ -976,7 +976,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { ) { deleteRemainingChildren(returnFiber, child.sibling); const existing = useFiber(child, priority); - existing.pendingProps = portal.children; + existing.pendingProps = portal.children || []; existing.return = returnFiber; return existing; } else { diff --git a/src/renderers/shared/fiber/ReactFiber.js b/src/renderers/shared/fiber/ReactFiber.js index 98e47ecaab3f1..7d3097ffaf4f7 100644 --- a/src/renderers/shared/fiber/ReactFiber.js +++ b/src/renderers/shared/fiber/ReactFiber.js @@ -346,7 +346,7 @@ exports.createFiberFromYield = function(yieldNode : ReactYield, priorityLevel : exports.createFiberFromPortal = function(portal : ReactPortal, priorityLevel : PriorityLevel) : Fiber { const fiber = createFiber(HostPortal, portal.key); - fiber.pendingProps = portal.children; + fiber.pendingProps = portal.children || []; fiber.pendingWorkPriority = priorityLevel; fiber.stateNode = { containerInfo: portal.containerInfo,