From e27720d7f586cdadb00b0de485e41ec4dbcebe52 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 7 Nov 2018 10:56:57 -0800 Subject: [PATCH] [Synchronous Suspense] Reuse deletions from primary tree (#14133) Fixes a bug where deletion effects in the primary tree were dropped before entering the second render pass. Because we no longer reset the effect list after the first render pass, I've also moved the deletion of the fallback children to the complete phase, after the tree successfully renders without suspending. Will need to revisit this heuristic when we implement resuming. --- .../src/ReactFiberBeginWork.js | 76 +++---------------- .../src/ReactFiberCompleteWork.js | 20 ++++- .../src/ReactFiberScheduler.js | 1 - .../__tests__/ReactSuspense-test.internal.js | 59 ++++++++++++++ 4 files changed, 88 insertions(+), 68 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 5849d84ff1f84..4e7ab1171d8b4 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -1127,11 +1127,7 @@ function updateSuspenseComponent( progressedState !== null ? (workInProgress.child: any).child : (workInProgress.child: any); - reuseProgressedPrimaryChild( - workInProgress, - primaryChildFragment, - progressedPrimaryChild, - ); + primaryChildFragment.child = progressedPrimaryChild; } const fallbackChildFragment = createFiberFromFragment( @@ -1186,11 +1182,7 @@ function updateSuspenseComponent( ? (workInProgress.child: any).child : (workInProgress.child: any); if (progressedPrimaryChild !== currentPrimaryChildFragment.child) { - reuseProgressedPrimaryChild( - workInProgress, - primaryChildFragment, - progressedPrimaryChild, - ); + primaryChildFragment.child = progressedPrimaryChild; } } @@ -1213,20 +1205,19 @@ function updateSuspenseComponent( // and remove the intermediate fragment fiber. const nextPrimaryChildren = nextProps.children; const currentPrimaryChild = currentPrimaryChildFragment.child; - const currentFallbackChild = currentFallbackChildFragment.child; const primaryChild = reconcileChildFibers( workInProgress, currentPrimaryChild, nextPrimaryChildren, renderExpirationTime, ); - // Delete the fallback children. - reconcileChildFibers( - workInProgress, - currentFallbackChild, - null, - renderExpirationTime, - ); + + // If this render doesn't suspend, we need to delete the fallback + // children. Wait until the complete phase, after we've confirmed the + // fallback is no longer needed. + // TODO: Would it be better to store the fallback fragment on + // the stateNode? + // Continue rendering the children, like we normally do. child = next = primaryChild; } @@ -1258,11 +1249,7 @@ function updateSuspenseComponent( progressedState !== null ? (workInProgress.child: any).child : (workInProgress.child: any); - reuseProgressedPrimaryChild( - workInProgress, - primaryChildFragment, - progressedPrimaryChild, - ); + primaryChildFragment.child = progressedPrimaryChild; } // Create a fragment from the fallback children, too. @@ -1298,49 +1285,6 @@ function updateSuspenseComponent( return next; } -function reuseProgressedPrimaryChild( - workInProgress: Fiber, - primaryChildFragment: Fiber, - progressedChild: Fiber | null, -) { - // This function is only called outside concurrent mode. Usually, if a work- - // in-progress primary tree suspends, we throw it out and revert back to - // current. Outside concurrent mode, though, we commit the suspended work-in- - // progress, even though it didn't complete. This function reuses the children - // and transfers the effects. - let child = (primaryChildFragment.child = progressedChild); - while (child !== null) { - // Ensure that the first and last effect of the parent corresponds - // to the children's first and last effect. - if (primaryChildFragment.firstEffect === null) { - primaryChildFragment.firstEffect = child.firstEffect; - } - if (child.lastEffect !== null) { - if (primaryChildFragment.lastEffect !== null) { - primaryChildFragment.lastEffect.nextEffect = child.firstEffect; - } - primaryChildFragment.lastEffect = child.lastEffect; - } - - // Append all the effects of the subtree and this fiber onto the effect - // list of the parent. The completion order of the children affects the - // side-effect order. - if (child.effectTag > PerformedWork) { - if (primaryChildFragment.lastEffect !== null) { - primaryChildFragment.lastEffect.nextEffect = child; - } else { - primaryChildFragment.firstEffect = child; - } - primaryChildFragment.lastEffect = child; - } - child.return = primaryChildFragment; - child = child.sibling; - } - - workInProgress.firstEffect = primaryChildFragment.firstEffect; - workInProgress.lastEffect = primaryChildFragment.lastEffect; -} - function updatePortalComponent( current: Fiber | null, workInProgress: Fiber, diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index d435d3296aeab..24126f026cc49 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -82,6 +82,7 @@ import { popHydrationState, } from './ReactFiberHydrationContext'; import {ConcurrentMode, NoContext} from './ReactTypeOfMode'; +import {reconcileChildFibers} from './ReactChildFiber'; function markUpdate(workInProgress: Fiber) { // Tag the fiber with an update effect. This turns a Placement into @@ -700,12 +701,29 @@ function completeWork( if ((workInProgress.effectTag & DidCapture) !== NoEffect) { // Something suspended. Re-render with the fallback children. workInProgress.expirationTime = renderExpirationTime; - workInProgress.firstEffect = workInProgress.lastEffect = null; + // Do not reset the effect list. return workInProgress; } const nextDidTimeout = nextState !== null; const prevDidTimeout = current !== null && current.memoizedState !== null; + + if (current !== null && !nextDidTimeout && prevDidTimeout) { + // We just switched from the fallback to the normal children. Delete + // the fallback. + // TODO: Would it be better to store the fallback fragment on + // the stateNode during the begin phase? + const currentFallbackChild: Fiber | null = (current.child: any).sibling; + if (currentFallbackChild !== null) { + reconcileChildFibers( + workInProgress, + currentFallbackChild, + null, + renderExpirationTime, + ); + } + } + // The children either timed out after previously being visible, or // were restored after previously being hidden. Schedule an effect // to update their visiblity. diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index d5e72788c980f..46475e7af2964 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -993,7 +993,6 @@ function completeUnitOfWork(workInProgress: Fiber): Fiber | null { if (nextUnitOfWork !== null) { // Completing this fiber spawned new work. Work on that next. - nextUnitOfWork.firstEffect = nextUnitOfWork.lastEffect = null; return nextUnitOfWork; } diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index 37fd4c70d0a7f..1d987f70fd020 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -779,5 +779,64 @@ describe('ReactSuspense', () => { expect(root).toFlushAndYield(['Child 1', 'Child 2']); expect(root).toMatchRenderedOutput(['Child 1', 'Child 2'].join('')); }); + + it('reuses effects, including deletions, from the suspended tree', () => { + const {useState} = React; + + let setTab; + function App() { + const [tab, _setTab] = useState(0); + setTab = _setTab; + + return ( + }> + + + + ); + } + + const root = ReactTestRenderer.create(); + expect(ReactTestRenderer).toHaveYielded([ + 'Suspend! [Tab: 0]', + ' + sibling', + 'Loading...', + ]); + expect(root).toMatchRenderedOutput('Loading...'); + jest.advanceTimersByTime(1000); + expect(ReactTestRenderer).toHaveYielded([ + 'Promise resolved [Tab: 0]', + 'Tab: 0', + ]); + expect(root).toMatchRenderedOutput('Tab: 0 + sibling'); + + setTab(1); + expect(ReactTestRenderer).toHaveYielded([ + 'Suspend! [Tab: 1]', + ' + sibling', + 'Loading...', + ]); + expect(root).toMatchRenderedOutput('Loading...'); + jest.advanceTimersByTime(1000); + expect(ReactTestRenderer).toHaveYielded([ + 'Promise resolved [Tab: 1]', + 'Tab: 1', + ]); + expect(root).toMatchRenderedOutput('Tab: 1 + sibling'); + + setTab(2); + expect(ReactTestRenderer).toHaveYielded([ + 'Suspend! [Tab: 2]', + ' + sibling', + 'Loading...', + ]); + expect(root).toMatchRenderedOutput('Loading...'); + jest.advanceTimersByTime(1000); + expect(ReactTestRenderer).toHaveYielded([ + 'Promise resolved [Tab: 2]', + 'Tab: 2', + ]); + expect(root).toMatchRenderedOutput('Tab: 2 + sibling'); + }); }); });