From da27d9e12201edb964972c6de822440998aec03d Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 27 Mar 2020 13:52:25 -0700 Subject: [PATCH] Bugfix: Suspended update must finish to unhide --- .../ReactDevToolsHooksIntegration-test.js | 1 + .../src/ReactFiberBeginWork.js | 113 +++++++++---- .../src/ReactFiberHydrationContext.js | 3 +- .../src/ReactFiberSuspenseComponent.js | 1 + ...tSuspenseWithNoopRenderer-test.internal.js | 153 +++++++++++++++++- 5 files changed, 240 insertions(+), 31 deletions(-) diff --git a/packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js b/packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js index 7a9384ea68ea..71587ef89d25 100644 --- a/packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js +++ b/packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js @@ -265,6 +265,7 @@ describe('React hooks DevTools integration', () => { // Release the lock setSuspenseHandler(() => false); scheduleUpdate(fiber); // Re-render + Scheduler.unstable_flushAll(); expect(renderer.toJSON().children).toEqual(['Done']); scheduleUpdate(fiber); // Re-render expect(renderer.toJSON().children).toEqual(['Done']); diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 1d5ac8fcc4bc..37e7c7673469 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -1570,37 +1570,88 @@ function validateFunctionComponentInDev(workInProgress: Fiber, Component: any) { } } -const SUSPENDED_MARKER: SuspenseState = { - dehydrated: null, - retryTime: NoWork, -}; +function mountSuspenseState( + renderExpirationTime: ExpirationTime, +): SuspenseState { + return { + dehydrated: null, + skippedTime: renderExpirationTime, + retryTime: NoWork, + }; +} + +function updateSuspenseState( + prevSuspenseState: SuspenseState, + renderExpirationTime: ExpirationTime, +): SuspenseState { + const prevSuspendedTime = prevSuspenseState.skippedTime; + return { + dehydrated: null, + skippedTime: + // Choose whichever time is a superset of the other one + prevSuspendedTime !== NoWork && prevSuspendedTime < renderExpirationTime + ? prevSuspendedTime + : renderExpirationTime, + retryTime: NoWork, + }; +} function shouldRemainOnFallback( suspenseContext: SuspenseContext, current: null | Fiber, workInProgress: Fiber, + renderExpirationTime: ExpirationTime, ) { - // If the context is telling us that we should show a fallback, and we're not - // already showing content, then we should show the fallback instead. - return ( - hasSuspenseContext( + if (current === null) { + return hasSuspenseContext( suspenseContext, (ForceSuspenseFallback: SuspenseContext), - ) && - (current === null || current.memoizedState !== null) + ); + } + + const suspenseState: SuspenseState = current.memoizedState; + if (suspenseState === null) { + // Not already suspended. + return false; + } + + const skippedTime = suspenseState.skippedTime; + if (skippedTime !== NoWork && skippedTime < renderExpirationTime) { + return true; + } + + return hasSuspenseContext( + suspenseContext, + (ForceSuspenseFallback: SuspenseContext), ); } function getRemainingWorkInPrimaryTree( - workInProgress, - currentChildExpirationTime, + current: Fiber, + workInProgress: Fiber, + currentPrimaryChildFragment: Fiber | null, renderExpirationTime, ) { + const currentSuspenseState: SuspenseState = current.memoizedState; + if (currentSuspenseState !== null) { + const skippedTime = currentSuspenseState.skippedTime; + if (skippedTime !== NoWork && skippedTime < renderExpirationTime) { + return skippedTime; + } + } + + const currentParentOfPrimaryChildren = + currentPrimaryChildFragment !== null + ? currentPrimaryChildFragment + : current; + const currentChildExpirationTime = + currentParentOfPrimaryChildren.childExpirationTime; if (currentChildExpirationTime < renderExpirationTime) { // The highest priority remaining work is not part of this render. So the // remaining work has not changed. return currentChildExpirationTime; } + if ((workInProgress.mode & BlockingMode) !== NoMode) { // The highest priority remaining work is part of this render. Since we only // keep track of the highest level, we don't know if there's a lower @@ -1642,7 +1693,12 @@ function updateSuspenseComponent( if ( didSuspend || - shouldRemainOnFallback(suspenseContext, current, workInProgress) + shouldRemainOnFallback( + suspenseContext, + current, + workInProgress, + renderExpirationTime, + ) ) { // Something in this boundary's subtree already suspended. Switch to // rendering the fallback children. @@ -1758,7 +1814,7 @@ function updateSuspenseComponent( primaryChildFragment.sibling = fallbackChildFragment; // Skip the primary children, and continue working on the // fallback children. - workInProgress.memoizedState = SUSPENDED_MARKER; + workInProgress.memoizedState = mountSuspenseState(renderExpirationTime); workInProgress.child = primaryChildFragment; return fallbackChildFragment; } else { @@ -1862,15 +1918,15 @@ function updateSuspenseComponent( primaryChildFragment.sibling = fallbackChildFragment; fallbackChildFragment.effectTag |= Placement; primaryChildFragment.childExpirationTime = getRemainingWorkInPrimaryTree( + current, workInProgress, - // This argument represents the remaining work in the current - // primary tree. Since the current tree did not already time out - // the direct parent of the primary children is the Suspense - // fiber, not a fragment. - current.childExpirationTime, + null, + renderExpirationTime, + ); + workInProgress.memoizedState = updateSuspenseState( + current.memoizedState, renderExpirationTime, ); - workInProgress.memoizedState = SUSPENDED_MARKER; workInProgress.child = primaryChildFragment; // Skip the primary children, and continue working on the @@ -1933,13 +1989,17 @@ function updateSuspenseComponent( fallbackChildFragment.return = workInProgress; primaryChildFragment.sibling = fallbackChildFragment; primaryChildFragment.childExpirationTime = getRemainingWorkInPrimaryTree( + current, workInProgress, - currentPrimaryChildFragment.childExpirationTime, + currentPrimaryChildFragment, renderExpirationTime, ); // Skip the primary children, and continue working on the // fallback children. - workInProgress.memoizedState = SUSPENDED_MARKER; + workInProgress.memoizedState = updateSuspenseState( + current.memoizedState, + renderExpirationTime, + ); workInProgress.child = primaryChildFragment; return fallbackChildFragment; } else { @@ -2031,17 +2091,14 @@ function updateSuspenseComponent( primaryChildFragment.sibling = fallbackChildFragment; fallbackChildFragment.effectTag |= Placement; primaryChildFragment.childExpirationTime = getRemainingWorkInPrimaryTree( + current, workInProgress, - // This argument represents the remaining work in the current - // primary tree. Since the current tree did not already time out - // the direct parent of the primary children is the Suspense - // fiber, not a fragment. - current.childExpirationTime, + null, renderExpirationTime, ); // Skip the primary children, and continue working on the // fallback children. - workInProgress.memoizedState = SUSPENDED_MARKER; + workInProgress.memoizedState = mountSuspenseState(renderExpirationTime); workInProgress.child = primaryChildFragment; return fallbackChildFragment; } else { diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.js b/packages/react-reconciler/src/ReactFiberHydrationContext.js index 76add94ea6f6..e3160fabc2cb 100644 --- a/packages/react-reconciler/src/ReactFiberHydrationContext.js +++ b/packages/react-reconciler/src/ReactFiberHydrationContext.js @@ -55,7 +55,7 @@ import { didNotFindHydratableSuspenseInstance, } from './ReactFiberHostConfig'; import {enableSuspenseServerRenderer} from 'shared/ReactFeatureFlags'; -import {Never} from './ReactFiberExpirationTime'; +import {Never, NoWork} from './ReactFiberExpirationTime'; // The deepest Fiber on the stack involved in a hydration context. // This may have been an insertion or a hydration. @@ -231,6 +231,7 @@ function tryHydrate(fiber, nextInstance) { if (suspenseInstance !== null) { const suspenseState: SuspenseState = { dehydrated: suspenseInstance, + skippedTime: NoWork, retryTime: Never, }; fiber.memoizedState = suspenseState; diff --git a/packages/react-reconciler/src/ReactFiberSuspenseComponent.js b/packages/react-reconciler/src/ReactFiberSuspenseComponent.js index 1c5f90735af5..79ed7f003250 100644 --- a/packages/react-reconciler/src/ReactFiberSuspenseComponent.js +++ b/packages/react-reconciler/src/ReactFiberSuspenseComponent.js @@ -35,6 +35,7 @@ export type SuspenseState = {| // here to indicate that it is dehydrated (flag) and for quick access // to check things like isSuspenseInstancePending. dehydrated: null | SuspenseInstance, + skippedTime: ExpirationTime, // Represents the earliest expiration time we should attempt to hydrate // a dehydrated boundary at. // Never is the default for dehydrated boundaries. diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index 55793fb6c3b3..3f925ffdf48b 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -3168,9 +3168,14 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); expect(Scheduler).toHaveYielded([ - // First try to update the suspended tree. It's still suspended. - 'Suspend! [C]', + // First try to render the high pri update. We won't try to re-render + // the suspended tree during this pass, because it still has unfinished + // updates at a lower priority. 'Loading...', + + // Now try the suspended update again. It's still suspended. + 'Suspend! [C]', + // Then complete the update to the fallback. 'Still loading...', ]); @@ -3260,4 +3265,148 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(root).toMatchRenderedOutput(); }, ); + + it( + 'after showing fallback, should not flip back to primary content until ' + + 'the update that suspended finishes', + async () => { + const {useState, useEffect} = React; + const root = ReactNoop.createRoot(); + + let setOuterText; + function Parent({step}) { + const [text, _setText] = useState('A'); + setOuterText = _setText; + return ( + <> + + + }> + + + + ); + } + + let setInnerText; + function Child({step, outerText}) { + const [text, _setText] = useState('A'); + setInnerText = _setText; + + // This will log if the component commits in an inconsistent state + useEffect(() => { + if (text === outerText) { + Scheduler.unstable_yieldValue('Commit Child'); + } else { + Scheduler.unstable_yieldValue( + 'FIXME: Texts are inconsistent (tearing)', + ); + } + }, [text, outerText]); + + return ( + <> + + + + ); + } + + // These always update simultaneously. They must be consistent. + function setText(text) { + setOuterText(text); + setInnerText(text); + } + + // Mount an initial tree. Resolve A so that it doesn't suspend. + await resolveText('Inner text: A'); + await ReactNoop.act(async () => { + root.render(); + }); + expect(Scheduler).toHaveYielded([ + 'Outer text: A', + 'Outer step: 0', + 'Inner text: A', + 'Inner step: 0', + 'Commit Child', + ]); + expect(root).toMatchRenderedOutput( + <> + + + + + , + ); + + // Update. This causes the inner component to suspend. + await ReactNoop.act(async () => { + setText('B'); + }); + expect(Scheduler).toHaveYielded([ + 'Outer text: B', + 'Outer step: 0', + 'Suspend! [Inner text: B]', + 'Inner step: 0', + 'Loading...', + ]); + // Commit the placeholder + await advanceTimers(250); + expect(root).toMatchRenderedOutput( + <> + + +