diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index 5ea22d07feae2..dbe645d792fd0 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -126,7 +126,6 @@ import { setShallowSuspenseListContext, ForceSuspenseFallback, setDefaultShallowSuspenseListContext, - isBadSuspenseFallback, } from './ReactFiberSuspenseContext'; import {popHiddenContext} from './ReactFiberHiddenContext'; import {findFirstSuspended} from './ReactFiberSuspenseComponent'; @@ -148,8 +147,6 @@ import { upgradeHydrationErrorsToRecoverable, } from './ReactFiberHydrationContext'; import { - renderDidSuspend, - renderDidSuspendDelayIfPossible, renderHasNotSuspendedYet, getRenderTargetTime, getWorkInProgressTransitions, @@ -1257,24 +1254,6 @@ function completeWork( if (nextDidTimeout) { const offscreenFiber: Fiber = (workInProgress.child: any); offscreenFiber.flags |= Visibility; - - // TODO: This will still suspend a synchronous tree if anything - // in the concurrent tree already suspended during this render. - // This is a known bug. - if ((workInProgress.mode & ConcurrentMode) !== NoMode) { - // TODO: Move this back to throwException because this is too late - // if this is a large tree which is common for initial loads. We - // don't know if we should restart a render or not until we get - // this marker, and this is too late. - // If this render already had a ping or lower pri updates, - // and this is the first time we know we're going to suspend we - // should be able to immediately restart from within throwException. - if (isBadSuspenseFallback(current, newProps)) { - renderDidSuspendDelayIfPossible(); - } else { - renderDidSuspend(); - } - } } } diff --git a/packages/react-reconciler/src/ReactFiberSuspenseContext.js b/packages/react-reconciler/src/ReactFiberSuspenseContext.js index c9e520e276558..b1010ec30dc23 100644 --- a/packages/react-reconciler/src/ReactFiberSuspenseContext.js +++ b/packages/react-reconciler/src/ReactFiberSuspenseContext.js @@ -9,12 +9,13 @@ import type {Fiber} from './ReactInternalTypes'; import type {StackCursor} from './ReactFiberStack'; -import type {SuspenseState, SuspenseProps} from './ReactFiberSuspenseComponent'; +import type {SuspenseProps, SuspenseState} from './ReactFiberSuspenseComponent'; +import type {OffscreenState} from './ReactFiberOffscreenComponent'; import {enableSuspenseAvoidThisFallback} from 'shared/ReactFeatureFlags'; import {createCursor, push, pop} from './ReactFiberStack'; import {isCurrentTreeHidden} from './ReactFiberHiddenContext'; -import {SuspenseComponent, OffscreenComponent} from './ReactWorkTags'; +import {OffscreenComponent} from './ReactWorkTags'; // The Suspense handler is the boundary that should capture if something // suspends, i.e. it's the nearest `catch` block on the stack. @@ -22,81 +23,72 @@ const suspenseHandlerStackCursor: StackCursor = createCursor( null, ); -function shouldAvoidedBoundaryCapture( - workInProgress: Fiber, - handlerOnStack: Fiber, - props: any, -): boolean { - if (enableSuspenseAvoidThisFallback) { - // If the parent is already showing content, and we're not inside a hidden - // tree, then we should show the avoided fallback. - if (handlerOnStack.alternate !== null && !isCurrentTreeHidden()) { - return true; - } - - // If the handler on the stack is also an avoided boundary, then we should - // favor this inner one. - if ( - handlerOnStack.tag === SuspenseComponent && - handlerOnStack.memoizedProps.unstable_avoidThisFallback === true - ) { - return true; - } - - // If this avoided boundary is dehydrated, then it should capture. - const suspenseState: SuspenseState | null = workInProgress.memoizedState; - if (suspenseState !== null && suspenseState.dehydrated !== null) { - return true; - } - } - - // If none of those cases apply, then we should avoid this fallback and show - // the outer one instead. - return false; -} - -export function isBadSuspenseFallback( - current: Fiber | null, - nextProps: SuspenseProps, -): boolean { - // Check if this is a "bad" fallback state or a good one. A bad fallback state - // is one that we only show as a last resort; if this is a transition, we'll - // block it from displaying, and wait for more data to arrive. - if (current !== null) { - const prevState: SuspenseState = current.memoizedState; - const isShowingFallback = prevState !== null; - if (!isShowingFallback && !isCurrentTreeHidden()) { - // It's bad to switch to a fallback if content is already visible - return true; - } - } - - if ( - enableSuspenseAvoidThisFallback && - nextProps.unstable_avoidThisFallback === true - ) { - // Experimental: Some fallbacks are always bad - return true; - } - - return false; +// Represents the outermost boundary that is not visible in the current tree. +// Everything above this is the "shell". When this is null, it means we're +// rendering in the shell of the app. If it's non-null, it means we're rendering +// deeper than the shell, inside a new tree that wasn't already visible. +// +// The main way we use this concept is to determine whether showing a fallback +// would result in a desirable or undesirable loading state. Activing a fallback +// in the shell is considered an undersirable loading state, because it would +// mean hiding visible (albeit stale) content in the current tree — we prefer to +// show the stale content, rather than switch to a fallback. But showing a +// fallback in a new tree is fine, because there's no stale content to +// prefer instead. +let shellBoundary: Fiber | null = null; + +export function getShellBoundary(): Fiber | null { + return shellBoundary; } export function pushPrimaryTreeSuspenseHandler(handler: Fiber): void { - const props = handler.pendingProps; - const handlerOnStack = suspenseHandlerStackCursor.current; + // TODO: Pass as argument + const current = handler.alternate; + const props: SuspenseProps = handler.pendingProps; + + // Experimental feature: Some Suspense boundaries are marked as having an + // undesirable fallback state. These have special behavior where we only + // activate the fallback if there's no other boundary on the stack that we can + // use instead. if ( enableSuspenseAvoidThisFallback && props.unstable_avoidThisFallback === true && - handlerOnStack !== null && - !shouldAvoidedBoundaryCapture(handler, handlerOnStack, props) + // If an avoided boundary is already visible, it behaves identically to + // a regular Suspense boundary. + (current === null || isCurrentTreeHidden()) ) { - // This boundary should not capture if something suspends. Reuse the - // existing handler on the stack. - push(suspenseHandlerStackCursor, handlerOnStack, handler); - } else { - // Push this handler onto the stack. - push(suspenseHandlerStackCursor, handler, handler); + if (shellBoundary === null) { + // We're rendering in the shell. There's no parent Suspense boundary that + // can provide a desirable fallback state. We'll use this boundary. + push(suspenseHandlerStackCursor, handler, handler); + + // However, because this is not a desirable fallback, the children are + // still considered part of the shell. So we intentionally don't assign + // to `shellBoundary`. + } else { + // There's already a parent Suspense boundary that can provide a desirable + // fallback state. Prefer that one. + const handlerOnStack = suspenseHandlerStackCursor.current; + push(suspenseHandlerStackCursor, handlerOnStack, handler); + } + return; + } + + // TODO: If the parent Suspense handler already suspended, there's no reason + // to push a nested Suspense handler, because it will get replaced by the + // outer fallback, anyway. Consider this as a future optimization. + push(suspenseHandlerStackCursor, handler, handler); + if (shellBoundary === null) { + if (current === null || isCurrentTreeHidden()) { + // This boundary is not visible in the current UI. + shellBoundary = handler; + } else { + const prevState: SuspenseState = current.memoizedState; + if (prevState !== null) { + // This boundary is showing a fallback in the current UI. + shellBoundary = handler; + } + } } } @@ -110,6 +102,20 @@ export function pushFallbackTreeSuspenseHandler(fiber: Fiber): void { export function pushOffscreenSuspenseHandler(fiber: Fiber): void { if (fiber.tag === OffscreenComponent) { push(suspenseHandlerStackCursor, fiber, fiber); + if (shellBoundary !== null) { + // A parent boundary is showing a fallback, so we've already rendered + // deeper than the shell. + } else { + const current = fiber.alternate; + if (current !== null) { + const prevState: OffscreenState = current.memoizedState; + if (prevState !== null) { + // This is the first boundary in the stack that's already showing + // a fallback. So everything outside is considered the shell. + shellBoundary = fiber; + } + } + } } else { // This is a LegacyHidden component. reuseSuspenseHandlerOnStack(fiber); @@ -126,6 +132,10 @@ export function getSuspenseHandler(): Fiber | null { export function popSuspenseHandler(fiber: Fiber): void { pop(suspenseHandlerStackCursor, fiber); + if (shellBoundary === fiber) { + // Popping back into the shell. + shellBoundary = null; + } } // SuspenseList context diff --git a/packages/react-reconciler/src/ReactFiberThrow.js b/packages/react-reconciler/src/ReactFiberThrow.js index d425175eabb55..7390a25e72d79 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.js +++ b/packages/react-reconciler/src/ReactFiberThrow.js @@ -49,7 +49,10 @@ import { enqueueUpdate, } from './ReactFiberClassUpdateQueue'; import {markFailedErrorBoundaryForHotReloading} from './ReactFiberHotReloading'; -import {getSuspenseHandler} from './ReactFiberSuspenseContext'; +import { + getShellBoundary, + getSuspenseHandler, +} from './ReactFiberSuspenseContext'; import { renderDidError, renderDidSuspendDelayIfPossible, @@ -58,6 +61,7 @@ import { isAlreadyFailedLegacyErrorBoundary, attachPingListener, restorePendingUpdaters, + renderDidSuspend, } from './ReactFiberWorkLoop'; import {propagateParentContextChangesToDeferredTree} from './ReactFiberNewContext'; import {logCapturedError} from './ReactFiberErrorLogger'; @@ -349,11 +353,46 @@ function throwException( } } - // Schedule the nearest Suspense to re-render the timed out view. + // Mark the nearest Suspense boundary to switch to rendering a fallback. const suspenseBoundary = getSuspenseHandler(); if (suspenseBoundary !== null) { switch (suspenseBoundary.tag) { case SuspenseComponent: { + // If this suspense boundary is not already showing a fallback, mark + // the in-progress render as suspended. We try to perform this logic + // as soon as soon as possible during the render phase, so the work + // loop can know things like whether it's OK to switch to other tasks, + // or whether it can wait for data to resolve before continuing. + // TODO: Most of these checks are already performed when entering a + // Suspense boundary. We should track the information on the stack so + // we don't have to recompute it on demand. This would also allow us + // to unify with `use` which needs to perform this logic even sooner, + // before `throwException` is called. + if (sourceFiber.mode & ConcurrentMode) { + if (getShellBoundary() === null) { + // Suspended in the "shell" of the app. This is an undesirable + // loading state. We should avoid committing this tree. + renderDidSuspendDelayIfPossible(); + } else { + // If we suspended deeper than the shell, we don't need to delay + // the commmit. However, we still call renderDidSuspend if this is + // a new boundary, to tell the work loop that a new fallback has + // appeared during this render. + // TODO: Theoretically we should be able to delete this branch. + // It's currently used for two things: 1) to throttle the + // appearance of successive loading states, and 2) in + // SuspenseList, to determine whether the children include any + // pending fallbacks. For 1, we should apply throttling to all + // retries, not just ones that render an additional fallback. For + // 2, we should check subtreeFlags instead. Then we can delete + // this branch. + const current = suspenseBoundary.alternate; + if (current === null) { + renderDidSuspend(); + } + } + } + suspenseBoundary.flags &= ~ForceClientRender; markSuspenseBoundaryShouldCapture( suspenseBoundary, diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 74ad3150258ed..b6efb85e19728 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -12,7 +12,7 @@ import {REACT_STRICT_MODE_TYPE} from 'shared/ReactSymbols'; import type {Wakeable, Thenable} from 'shared/ReactTypes'; import type {Fiber, FiberRoot} from './ReactInternalTypes'; import type {Lanes, Lane} from './ReactFiberLane'; -import type {SuspenseProps, SuspenseState} from './ReactFiberSuspenseComponent'; +import type {SuspenseState} from './ReactFiberSuspenseComponent'; import type {FunctionComponentUpdateQueue} from './ReactFiberHooks'; import type {EventPriority} from './ReactEventPriorities'; import type { @@ -276,7 +276,7 @@ import { import {schedulePostPaintCallback} from './ReactPostPaintCallback'; import { getSuspenseHandler, - isBadSuspenseFallback, + getShellBoundary, } from './ReactFiberSuspenseContext'; import {resolveDefaultProps} from './ReactFiberLazyComponent'; @@ -1859,18 +1859,11 @@ function handleThrow(root, thrownValue): void { } function shouldAttemptToSuspendUntilDataResolves() { - // TODO: We should be able to move the - // renderDidSuspend/renderDidSuspendDelayIfPossible logic into this function, - // instead of repeating it in the complete phase. Or something to that effect. - - if (includesOnlyRetries(workInProgressRootRenderLanes)) { - // We can always wait during a retry. - return true; - } - // Check if there are other pending updates that might possibly unblock this // component from suspending. This mirrors the check in // renderDidSuspendDelayIfPossible. We should attempt to unify them somehow. + // TODO: Consider unwinding immediately, using the + // SuspendedOnHydration mechanism. if ( includesNonIdleWork(workInProgressRootSkippedLanes) || includesNonIdleWork(workInProgressRootInterleavedUpdatedLanes) @@ -1883,27 +1876,24 @@ function shouldAttemptToSuspendUntilDataResolves() { // TODO: We should be able to remove the equivalent check in // finishConcurrentRender, and rely just on this one. if (includesOnlyTransitions(workInProgressRootRenderLanes)) { - const suspenseHandler = getSuspenseHandler(); - if (suspenseHandler !== null && suspenseHandler.tag === SuspenseComponent) { - const currentSuspenseHandler = suspenseHandler.alternate; - const nextProps: SuspenseProps = suspenseHandler.memoizedProps; - if (isBadSuspenseFallback(currentSuspenseHandler, nextProps)) { - // The nearest Suspense boundary is already showing content. We should - // avoid replacing it with a fallback, and instead wait until the - // data finishes loading. - return true; - } else { - // This is not a bad fallback condition. We should show a fallback - // immediately instead of waiting for the data to resolve. This includes - // when suspending inside new trees. - return false; - } - } + // If we're rendering inside the "shell" of the app, it's better to suspend + // rendering and wait for the data to resolve. Otherwise, we should switch + // to a fallback and continue rendering. + return getShellBoundary() === null; + } - // During a transition, if there is no Suspense boundary (i.e. suspending in - // the "shell" of an application), or if we're inside a hidden tree, then - // we should wait until the data finishes loading. - return true; + const handler = getSuspenseHandler(); + if (handler === null) { + // TODO: We should support suspending in the case where there's no + // parent Suspense boundary, even outside a transition. Somehow. Otherwise, + // an uncached promise can fall into an infinite loop. + } else { + if (includesOnlyRetries(workInProgressRootRenderLanes)) { + // During a retry, we can suspend rendering if the nearest Suspense boundary + // is the boundary of the "shell", because we're guaranteed not to block + // any new content from appearing. + return handler === getShellBoundary(); + } } // For all other Lanes besides Transitions and Retries, we should not wait @@ -1981,6 +1971,8 @@ export function renderDidSuspendDelayIfPossible(): void { // (inside this function), since by suspending at the end of the render // phase introduces a potential mistake where we suspend lanes that were // pinged or updated while we were rendering. + // TODO: Consider unwinding immediately, using the + // SuspendedOnHydration mechanism. markRootSuspended(workInProgressRoot, workInProgressRootRenderLanes); } } @@ -2198,6 +2190,10 @@ function renderRootConcurrent(root: FiberRoot, lanes: Lanes) { } // The work loop is suspended on data. We should wait for it to // resolve before continuing to render. + // TODO: Handle the case where the promise resolves synchronously. + // Usually this is handled when we instrument the promise to add a + // `status` field, but if the promise already has a status, we won't + // have added a listener until right here. const onResolution = () => { ensureRootIsScheduled(root, now()); }; diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js index bb4ea103ff124..9d95a57572f50 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js @@ -995,14 +995,10 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(Scheduler).toFlushAndYieldThrough(['Suspend! [Async]', 'Sibling']); await resolveText('Async'); - expect(Scheduler).toFlushAndYield([ - // We've now pinged the boundary but we don't know if we should restart yet, - // because we haven't completed the suspense boundary. - 'Loading...', - // Once we've completed the boundary we restarted. - 'Async', - 'Sibling', - ]); + + // Because we're already showing a fallback, interrupt the current render + // and restart immediately. + expect(Scheduler).toFlushAndYield(['Async', 'Sibling']); expect(root).toMatchRenderedOutput( <> diff --git a/packages/react-reconciler/src/__tests__/ReactThenable-test.js b/packages/react-reconciler/src/__tests__/ReactThenable-test.js index 0af8ccafd381d..965d69b7c2aa0 100644 --- a/packages/react-reconciler/src/__tests__/ReactThenable-test.js +++ b/packages/react-reconciler/src/__tests__/ReactThenable-test.js @@ -9,6 +9,7 @@ let useState; let useMemo; let Suspense; let startTransition; +let cache; let pendingTextRequests; describe('ReactThenable', () => { @@ -24,6 +25,7 @@ describe('ReactThenable', () => { useMemo = React.useMemo; Suspense = React.Suspense; startTransition = React.startTransition; + cache = React.cache; pendingTextRequests = new Map(); }); @@ -876,4 +878,147 @@ describe('ReactThenable', () => { ]); }, ); + + // @gate enableUseHook + test('load multiple nested Suspense boundaries', async () => { + const getCachedAsyncText = cache(getAsyncText); + + function AsyncText({text}) { + return ; + } + + const root = ReactNoop.createRoot(); + await act(async () => { + root.render( + }> + + }> + + }> + + + + , + ); + }); + expect(Scheduler).toHaveYielded([ + 'Async text requested [A]', + 'Async text requested [B]', + 'Async text requested [C]', + '(Loading C...)', + '(Loading B...)', + '(Loading A...)', + ]); + expect(root).toMatchRenderedOutput('(Loading A...)'); + + await act(async () => { + resolveTextRequests('A'); + }); + expect(Scheduler).toHaveYielded(['A', '(Loading C...)', '(Loading B...)']); + expect(root).toMatchRenderedOutput('A(Loading B...)'); + + await act(async () => { + resolveTextRequests('B'); + }); + expect(Scheduler).toHaveYielded(['B', '(Loading C...)']); + expect(root).toMatchRenderedOutput('AB(Loading C...)'); + + await act(async () => { + resolveTextRequests('C'); + }); + expect(Scheduler).toHaveYielded(['C']); + expect(root).toMatchRenderedOutput('ABC'); + }); + + // @gate enableUseHook + test('load multiple nested Suspense boundaries (uncached requests)', async () => { + // This the same as the previous test, except the requests are not cached. + // The tree should still eventually resolve, despite the + // duplicate requests. + function AsyncText({text}) { + // This initiates a new request on each render. + return ; + } + + const root = ReactNoop.createRoot(); + await act(async () => { + root.render( + }> + + }> + + }> + + + + , + ); + }); + expect(Scheduler).toHaveYielded([ + 'Async text requested [A]', + 'Async text requested [B]', + 'Async text requested [C]', + '(Loading C...)', + '(Loading B...)', + '(Loading A...)', + ]); + expect(root).toMatchRenderedOutput('(Loading A...)'); + + await act(async () => { + resolveTextRequests('A'); + }); + expect(Scheduler).toHaveYielded(['Async text requested [A]']); + expect(root).toMatchRenderedOutput('(Loading A...)'); + + await act(async () => { + resolveTextRequests('A'); + }); + expect(Scheduler).toHaveYielded([ + // React suspends until A finishes loading. + 'Async text requested [A]', + 'A', + + // Now React can continue rendering the rest of the tree. + + // React does not suspend on the inner requests, because that would + // block A from appearing. Instead it shows a fallback. + 'Async text requested [B]', + 'Async text requested [C]', + '(Loading C...)', + '(Loading B...)', + ]); + expect(root).toMatchRenderedOutput('A(Loading B...)'); + + await act(async () => { + resolveTextRequests('B'); + }); + expect(Scheduler).toHaveYielded(['Async text requested [B]']); + expect(root).toMatchRenderedOutput('A(Loading B...)'); + + await act(async () => { + resolveTextRequests('B'); + }); + expect(Scheduler).toHaveYielded([ + // React suspends until B finishes loading. + 'Async text requested [B]', + 'B', + + // React does not suspend on C, because that would block B from appearing. + 'Async text requested [C]', + '(Loading C...)', + ]); + expect(root).toMatchRenderedOutput('AB(Loading C...)'); + + await act(async () => { + resolveTextRequests('C'); + }); + expect(Scheduler).toHaveYielded(['Async text requested [C]']); + expect(root).toMatchRenderedOutput('AB(Loading C...)'); + + await act(async () => { + resolveTextRequests('C'); + }); + expect(Scheduler).toHaveYielded(['Async text requested [C]', 'C']); + expect(root).toMatchRenderedOutput('ABC'); + }); });