From 64ac43295b24812573aaa010ffa9c8b4e4ef055c Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 24 Apr 2020 23:16:25 -0700 Subject: [PATCH] Unhide Suspense trees without entanglement When a Suspense boundary is in its fallback state, you cannot switch back to the main content without also finishing any updates inside the tree that might have been skipped. That would be a form of tearing. Before we fixed this in #18411, the way this bug manfiested was that a boundary was suspended by an update that originated from a child component (as opposed to props from a parent). While the fallback was showing, it received another update, this time at high priority. React would render the high priority update without also including the original update. That would cause the fallback to switch back to the main content, since the update that caused the tree to suspend was no longer part of the render. But then, React would immediately try to render the original update, which would again suspend and show the fallback, leading to a momentary flicker in the UI. The approach added in #18411 is, when receiving a high priority update to a Suspense tree that's in its fallback state is to bail out, keep showing the fallback and finish the update in the rest of the tree. After that commits, render again at the original priority. Because low priority expiration times are inclusive of higher priority expiration times, this ensures that all the updates are committed together. The new approach in this commit is to turn `renderExpirationTime` into a context-like value that lives on the stack. Then, when unhiding the Suspense boundary, we can push a new `renderExpirationTime` that is inclusive of both the high pri update and the original update that suspended. Then the boundary can be unblocked in a single render pass. An advantage of the old approach is that by deferring the work of unhiding, there's less work to do in the high priority update. The key advantage of the new approach is that it solves the consistency problem without having to entangle the entire root. --- .../src/ReactFiberBeginWork.new.js | 129 +++++++++--------- .../src/ReactFiberCompleteWork.new.js | 2 + .../src/ReactFiberHydrationContext.new.js | 3 +- .../src/ReactFiberSuspenseComponent.new.js | 4 - .../src/ReactFiberUnwindWork.new.js | 8 ++ .../src/ReactFiberWorkLoop.new.js | 31 +++++ .../ReactSuspenseWithNoopRenderer-test.js | 114 ++++++++++++---- 7 files changed, 191 insertions(+), 100 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js index 86c95a3978222..fd0350d388eb3 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.new.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.new.js @@ -187,6 +187,7 @@ import { renderDidSuspendDelayIfPossible, markUnprocessedUpdateTime, getWorkInProgressRoot, + pushRenderExpirationTime, } from './ReactFiberWorkLoop.new'; import {disableLogs, reenableLogs} from 'shared/ConsolePatchingDev'; @@ -569,17 +570,30 @@ function updateOffscreenComponent( const nextProps: OffscreenProps = workInProgress.pendingProps; const nextChildren = nextProps.children; + let subtreeRenderTime = renderExpirationTime; if (current !== null) { if (nextProps.mode === 'hidden') { // TODO: Should currently be unreachable because Offscreen is only used as // an implementation detail of Suspense. Once this is a public API, it // will need to create an OffscreenState. } else { - // Clear the offscreen state. - workInProgress.memoizedState = null; + const prevState: OffscreenState | null = current.memoizedState; + if (prevState !== null) { + const baseTime = prevState.baseTime; + subtreeRenderTime = !isSameOrHigherPriority( + baseTime, + renderExpirationTime, + ) + ? baseTime + : renderExpirationTime; + + // Since we're not hidden anymore, reset the state + workInProgress.memoizedState = null; + } } } + pushRenderExpirationTime(workInProgress, subtreeRenderTime); reconcileChildren( current, workInProgress, @@ -1651,33 +1665,32 @@ function validateFunctionComponentInDev(workInProgress: Fiber, Component: any) { } } -function mountSuspenseState( +const SUSPENDED_MARKER: SuspenseState = { + dehydrated: null, + retryTime: NoWork, +}; + +function mountSuspenseOffscreenState( renderExpirationTime: ExpirationTimeOpaque, -): SuspenseState { +): OffscreenState { return { - dehydrated: null, baseTime: renderExpirationTime, - retryTime: NoWork, }; } -function updateSuspenseState( - prevSuspenseState: SuspenseState, +function updateSuspenseOffscreenState( + prevOffscreenState: OffscreenState, renderExpirationTime: ExpirationTimeOpaque, -): SuspenseState { - const prevSuspendedTime = prevSuspenseState.baseTime; +): OffscreenState { + const prevBaseTime = prevOffscreenState.baseTime; return { - dehydrated: null, + // Choose whichever time is inclusive of the other one. This represents + // the union of all the levels that suspended. baseTime: - // Choose whichever time is inclusive of the other one. This represents - // the union of all the levels that suspended. - !isSameExpirationTime( - prevSuspendedTime, - (NoWork: ExpirationTimeOpaque), - ) && !isSameOrHigherPriority(prevSuspendedTime, renderExpirationTime) - ? prevSuspendedTime + !isSameExpirationTime(prevBaseTime, (NoWork: ExpirationTimeOpaque)) && + !isSameOrHigherPriority(prevBaseTime, renderExpirationTime) + ? prevBaseTime : renderExpirationTime, - retryTime: NoWork, }; } @@ -1692,19 +1705,7 @@ function shouldRemainOnFallback( // For example, SuspenseList coordinates when nested content appears. if (current !== null) { const suspenseState: SuspenseState = current.memoizedState; - if (suspenseState !== null) { - // Currently showing a fallback. If the current render includes - // the level that triggered the fallback, we must continue showing it, - // regardless of what the Suspense context says. - const baseTime = suspenseState.baseTime; - if ( - !isSameExpirationTime(baseTime, (NoWork: ExpirationTimeOpaque)) && - !isSameOrHigherPriority(baseTime, renderExpirationTime) - ) { - return true; - } - // Otherwise, fall through to check the Suspense context. - } else { + if (suspenseState === null) { // Currently showing content. Don't hide it, even if ForceSuspenseFallack // is true. More precise name might be "ForceRemainSuspenseFallback". // Note: This is a factoring smell. Can't remain on a fallback if there's @@ -1712,6 +1713,7 @@ function shouldRemainOnFallback( return false; } } + // Not currently showing content. Consult the Suspense context. return hasSuspenseContext( suspenseContext, @@ -1725,20 +1727,6 @@ function getRemainingWorkInPrimaryTree( renderExpirationTime, ) { const currentChildExpirationTime = current.childExpirationTime_opaque; - const currentSuspenseState: SuspenseState = current.memoizedState; - if (currentSuspenseState !== null) { - // This boundary already timed out. Check if this render includes the level - // that previously suspended. - const baseTime = currentSuspenseState.baseTime; - if ( - !isSameExpirationTime(baseTime, (NoWork: ExpirationTimeOpaque)) && - !isSameOrHigherPriority(baseTime, renderExpirationTime) - ) { - // There's pending work at a lower level that might now be unblocked. - return baseTime; - } - } - if ( !isSameOrHigherPriority(currentChildExpirationTime, renderExpirationTime) ) { @@ -1880,8 +1868,10 @@ function updateSuspenseComponent( renderExpirationTime, ); const primaryChildFragment: Fiber = (workInProgress.child: any); - primaryChildFragment.memoizedState = ({baseTime: NoWork}: OffscreenState); - workInProgress.memoizedState = mountSuspenseState(renderExpirationTime); + primaryChildFragment.memoizedState = mountSuspenseOffscreenState( + renderExpirationTime, + ); + workInProgress.memoizedState = SUSPENDED_MARKER; return fallbackFragment; } else { const nextPrimaryChildren = nextProps.children; @@ -1935,14 +1925,10 @@ function updateSuspenseComponent( renderExpirationTime, ); const primaryChildFragment: Fiber = (workInProgress.child: any); - primaryChildFragment.memoizedState = ({ - baseTime: NoWork, - }: OffscreenState); - workInProgress.memoizedState = updateSuspenseState( - current.memoizedState, + primaryChildFragment.memoizedState = mountSuspenseOffscreenState( renderExpirationTime, ); - + workInProgress.memoizedState = SUSPENDED_MARKER; return fallbackChildFragment; } } @@ -1959,18 +1945,21 @@ function updateSuspenseComponent( renderExpirationTime, ); const primaryChildFragment: Fiber = (workInProgress.child: any); - primaryChildFragment.memoizedState = ({ - baseTime: NoWork, - }: OffscreenState); + const prevOffscreenState: OffscreenState | null = (current.child: any) + .memoizedState; + primaryChildFragment.memoizedState = + prevOffscreenState === null + ? mountSuspenseOffscreenState(renderExpirationTime) + : updateSuspenseOffscreenState( + prevOffscreenState, + renderExpirationTime, + ); primaryChildFragment.childExpirationTime_opaque = getRemainingWorkInPrimaryTree( current, workInProgress, renderExpirationTime, ); - workInProgress.memoizedState = updateSuspenseState( - current.memoizedState, - renderExpirationTime, - ); + workInProgress.memoizedState = SUSPENDED_MARKER; return fallbackChildFragment; } else { const nextPrimaryChildren = nextProps.children; @@ -1997,9 +1986,15 @@ function updateSuspenseComponent( renderExpirationTime, ); const primaryChildFragment: Fiber = (workInProgress.child: any); - primaryChildFragment.memoizedState = ({ - baseTime: NoWork, - }: OffscreenState); + const prevOffscreenState: OffscreenState | null = (current.child: any) + .memoizedState; + primaryChildFragment.memoizedState = + prevOffscreenState === null + ? mountSuspenseOffscreenState(renderExpirationTime) + : updateSuspenseOffscreenState( + prevOffscreenState, + renderExpirationTime, + ); primaryChildFragment.childExpirationTime_opaque = getRemainingWorkInPrimaryTree( current, workInProgress, @@ -2007,7 +2002,7 @@ function updateSuspenseComponent( ); // Skip the primary children, and continue working on the // fallback children. - workInProgress.memoizedState = mountSuspenseState(renderExpirationTime); + workInProgress.memoizedState = SUSPENDED_MARKER; return fallbackChildFragment; } else { // Still haven't timed out. Continue rendering the children, like we @@ -3390,6 +3385,10 @@ function beginWork( return null; } } + case OffscreenComponent: { + pushRenderExpirationTime(workInProgress, renderExpirationTime); + break; + } } return bailoutOnAlreadyFinishedWork( current, diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js index 2299e61f6e580..4419c5c05f507 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js @@ -125,6 +125,7 @@ import { renderDidSuspend, renderDidSuspendDelayIfPossible, renderHasNotSuspendedYet, + popRenderExpirationTime, } from './ReactFiberWorkLoop.new'; import {createFundamentalStateInstance} from './ReactFiberFundamental.new'; import { @@ -1291,6 +1292,7 @@ function completeWork( } break; case OffscreenComponent: { + popRenderExpirationTime(workInProgress); if (current !== null) { const nextState: OffscreenState | null = workInProgress.memoizedState; const prevState: OffscreenState | null = current.memoizedState; diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js index 88d7511cef196..f1ed2e73cccdb 100644 --- a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js +++ b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js @@ -55,7 +55,7 @@ import { didNotFindHydratableSuspenseInstance, } from './ReactFiberHostConfig'; import {enableSuspenseServerRenderer} from 'shared/ReactFeatureFlags'; -import {Never, NoWork} from './ReactFiberExpirationTime.new'; +import {Never} from './ReactFiberExpirationTime.new'; // The deepest Fiber on the stack involved in a hydration context. // This may have been an insertion or a hydration. @@ -231,7 +231,6 @@ function tryHydrate(fiber, nextInstance) { if (suspenseInstance !== null) { const suspenseState: SuspenseState = { dehydrated: suspenseInstance, - baseTime: NoWork, retryTime: Never, }; fiber.memoizedState = suspenseState; diff --git a/packages/react-reconciler/src/ReactFiberSuspenseComponent.new.js b/packages/react-reconciler/src/ReactFiberSuspenseComponent.new.js index 26912af8f11a1..a45f1710138fb 100644 --- a/packages/react-reconciler/src/ReactFiberSuspenseComponent.new.js +++ b/packages/react-reconciler/src/ReactFiberSuspenseComponent.new.js @@ -29,10 +29,6 @@ export type SuspenseState = {| // here to indicate that it is dehydrated (flag) and for quick access // to check things like isSuspenseInstancePending. dehydrated: null | SuspenseInstance, - // Represents the work that was deprioritized when we committed the fallback. - // The work outside the boundary already committed at this level, so we cannot - // unhide the content without including it. - baseTime: ExpirationTimeOpaque, // 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/ReactFiberUnwindWork.new.js b/packages/react-reconciler/src/ReactFiberUnwindWork.new.js index d7d5585b5cc41..ae243c04115a0 100644 --- a/packages/react-reconciler/src/ReactFiberUnwindWork.new.js +++ b/packages/react-reconciler/src/ReactFiberUnwindWork.new.js @@ -20,6 +20,7 @@ import { ContextProvider, SuspenseComponent, SuspenseListComponent, + OffscreenComponent, } from './ReactWorkTags'; import {DidCapture, NoEffect, ShouldCapture} from './ReactSideEffectTags'; import {enableSuspenseServerRenderer} from 'shared/ReactFeatureFlags'; @@ -33,6 +34,7 @@ import { popTopLevelContextObject as popTopLevelLegacyContextObject, } from './ReactFiberContext.new'; import {popProvider} from './ReactFiberNewContext.new'; +import {popRenderExpirationTime} from './ReactFiberWorkLoop.new'; import invariant from 'shared/invariant'; @@ -105,6 +107,9 @@ function unwindWork( case ContextProvider: popProvider(workInProgress); return null; + case OffscreenComponent: + popRenderExpirationTime(workInProgress); + return null; default: return null; } @@ -141,6 +146,9 @@ function unwindInterruptedWork(interruptedWork: Fiber) { case ContextProvider: popProvider(interruptedWork); break; + case OffscreenComponent: + popRenderExpirationTime(interruptedWork); + break; default: break; } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 5d8b99cbc9a90..fed70d4c1a76f 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -15,6 +15,7 @@ import type {Interaction} from 'scheduler/src/Tracing'; import type {SuspenseConfig} from './ReactFiberSuspenseConfig'; import type {SuspenseState} from './ReactFiberSuspenseComponent.new'; import type {Effect as HookEffect} from './ReactFiberHooks.new'; +import type {StackCursor} from './ReactFiberStack.new'; import { warnAboutDeprecatedLifecycles, @@ -168,6 +169,11 @@ import { getIsUpdatingOpaqueValueInRenderPhaseInDEV, } from './ReactFiberHooks.new'; import {createCapturedValue} from './ReactCapturedValue'; +import { + push as pushToStack, + pop as popFromStack, + createCursor, +} from './ReactFiberStack.new'; import { recordCommitTime, @@ -1262,6 +1268,31 @@ export function flushControlled(fn: () => mixed): void { } } +let prevRenderExpirationTime: ExpirationTimeOpaque = NoWork; +let renderExpirationTimeCursorInDEV: StackCursor | null = null; +if (__DEV__) { + // This is only used in DEV to catch push/pop misalignment bugs. + renderExpirationTimeCursorInDEV = createCursor(null); +} + +export function pushRenderExpirationTime( + fiber: Fiber, + subtreeRenderTime: ExpirationTimeOpaque, +) { + if (__DEV__ && renderExpirationTimeCursorInDEV !== null) { + pushToStack(renderExpirationTimeCursorInDEV, null, fiber); + } + prevRenderExpirationTime = renderExpirationTime; + renderExpirationTime = subtreeRenderTime; +} + +export function popRenderExpirationTime(fiber: Fiber) { + if (__DEV__ && renderExpirationTimeCursorInDEV !== null) { + popFromStack(renderExpirationTimeCursorInDEV, fiber); + } + renderExpirationTime = prevRenderExpirationTime; +} + function prepareFreshStack(root, expirationTime) { root.finishedWork = null; root.finishedExpirationTime_opaque = NoWork; diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js index f6e7d69382858..bf1f1a696ea2f 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js @@ -3180,18 +3180,49 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); setFallbackText('Still loading...'); - expect(Scheduler).toFlushAndYield([ - // 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...', - ]); + expect(Scheduler).toFlushAndYield( + gate(flags => + flags.new + ? [ + // First try to render the high pri update. Still suspended. + 'Suspend! [C]', + 'Loading...', + + // In the expiration times model, once the high pri update + // suspends, we can't be sure if there's additional work at a + // lower priority that might unblock the tree. We do know that + // there's a lower priority update *somehwere* in the entire + // root, though (the update to the fallback). So we try + // rendering one more time, just in case. + // TODO: We shouldn't need to do this with lanes, because we + // always know exactly which lanes have pending work in + // each tree. + 'Suspend! [C]', + + // Then complete the update to the fallback. + 'Still loading...', + ] + : [ + // In the old reconciler, we don't attempt to unhdie the + // Suspense boundary at high priority. Instead, we bailout, + // then try again at the original priority that the component + // suspended. This is mostly an implementation compromise, + // though there are some advantages to this behavior, because + // attempt to unhide could slow down the rest of the update. + // + // Render that only includes the fallback, since we bailed + // out on the primary tree. + 'Loading...', + + // Now try the suspended update again at the original + // priority. It's still suspended. + 'Suspend! [C]', + + // Then complete the update to the fallback. + 'Still loading...', + ], + ), + ); expect(root).toMatchRenderedOutput( <>