From 8d5047e0119da638c1821281e301b01351968ed6 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Sun, 26 Mar 2023 17:33:24 -0400 Subject: [PATCH 1/4] Fix: Insert stylesheets that previously suspended Follow up to #26450. In `acquireResource`, if a stylesheet resource already has an instance, we need to confirm that it was inserted into the document, because it may have been suspended. This happens because stylesheet resources are assigned a resource before committing, inside `suspendResource`. Probably a factoring a smell. As currently implemented, the idea is that `suspendResource` does all the same stuff as `acquireResource` except for the insertion. --- .../src/client/ReactDOMHostConfig.js | 33 +++++++++++++++++-- .../src/forks/ReactFiberHostConfig.custom.js | 1 + 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/packages/react-dom-bindings/src/client/ReactDOMHostConfig.js b/packages/react-dom-bindings/src/client/ReactDOMHostConfig.js index df1714f881520..ed8d0732825aa 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom-bindings/src/client/ReactDOMHostConfig.js @@ -1788,7 +1788,7 @@ type StyleTagResource = TResource<'style', null>; type StyleResource = StyleTagResource | StylesheetResource; type ScriptResource = TResource<'script', null>; type VoidResource = TResource<'void', null>; -type Resource = StyleResource | ScriptResource | VoidResource; +export type Resource = StyleResource | ScriptResource | VoidResource; type LoadingState = number; const NotLoaded = /* */ 0b000; @@ -2170,6 +2170,7 @@ function preinit(href: string, options: PreinitOptions) { state.loading |= Errored; }); + state.loading |= Inserted; insertStylesheet(instance, precedence, resourceRoot); } @@ -2518,6 +2519,11 @@ export function acquireResource( markNodeAsHoistable(instance); setInitialProperties(instance, 'style', styleProps); + + // TODO: `style` does not have loading state for tracking insertions. I + // guess because these aren't suspensey? Not sure whether this is a + // factoring smell. + // resource.state.loading |= Inserted; insertStylesheet(instance, qualifiedProps.precedence, hoistableRoot); resource.instance = instance; @@ -2556,6 +2562,7 @@ export function acquireResource( linkInstance.onerror = reject; }); setInitialProperties(instance, 'link', stylesheetProps); + resource.state.loading |= Inserted; insertStylesheet(instance, qualifiedProps.precedence, hoistableRoot); resource.instance = instance; @@ -2604,6 +2611,28 @@ export function acquireResource( ); } } + } else { + // In the case of stylesheets, they might have already been assigned an + // instance during `suspendResource`. But that doesn't mean they were + // inserted, because the commit might have been interrupted. So we need to + // check now. + // + // The other resource types are unaffected because they are not + // yet suspensey. + // + // TODO: This is a bit of a code smell. Consider refactoring how + // `suspendResource` and `acquireResource` work together. The idea is that + // `suspendResource` does all the same stuff as `acquireResource` except + // for the insertion. + if ( + resource.type === 'stylesheet' && + (resource.state.loading & Inserted) === NotLoaded + ) { + const qualifiedProps: StylesheetQualifyingProps = props; + const instance: Instance = resource.instance; + resource.state.loading |= Inserted; + insertStylesheet(instance, qualifiedProps.precedence, hoistableRoot); + } } return resource.instance; } @@ -2613,7 +2642,7 @@ export function releaseResource(resource: Resource): void { } function insertStylesheet( - instance: HTMLElement, + instance: Element, precedence: string, root: HoistableRoot, ): void { diff --git a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js index 938b83080431a..995da3a931243 100644 --- a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js +++ b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js @@ -208,6 +208,7 @@ export const errorHydratingContainer = $$$hostConfig.errorHydratingContainer; // (optional) // ------------------- export type HoistableRoot = mixed; +export type Resource = mixed; // eslint-disable-line no-undef export const supportsResources = $$$hostConfig.supportsResources; export const isHostHoistableType = $$$hostConfig.isHostHoistableType; export const getHoistableRoot = $$$hostConfig.getHoistableRoot; From 01910075b0f03af7b330ea658f462e5db4e7ca4d Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Sun, 26 Mar 2023 17:46:04 -0400 Subject: [PATCH 2/4] Fix: HostHoistable should call suspendResource Follow up to #26450. In the complete phase of HostHoistable, it should call suspendResource instead of suspenseInstance. I refactored the code a bit to make the branching more clear. There's a test case that covers this but it's currently gated behind a TODO because of another issue, which I'll fix in the next step. --- .../src/ReactFiberCompleteWork.js | 211 +++++++++++++----- .../ReactFiberHostConfigWithNoResources.js | 1 + 2 files changed, 150 insertions(+), 62 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index d2d72562183dc..9aacd549f0a82 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -17,6 +17,7 @@ import type { Props, Container, ChildSet, + Resource, } from './ReactFiberHostConfig'; import type { SuspenseState, @@ -112,6 +113,7 @@ import { maySuspendCommit, mayResourceSuspendCommit, preloadInstance, + preloadResource, } from './ReactFiberHostConfig'; import { getRootHostContainer, @@ -511,6 +513,10 @@ function updateHostComponent( } } +// This function must be called at the very end of the complete phase, because +// it might throw to suspend, and if the resource immediately loads, the work +// loop will resume rendering as if the work-in-progress completed. So it must +// fully complete. // TODO: This should ideally move to begin phase, but currently the instance is // not created until the complete phase. For our existing use cases, host nodes // that suspend don't have children, so it doesn't matter. But that might not @@ -521,13 +527,35 @@ function preloadInstanceAndSuspendIfNeeded( props: Props, renderLanes: Lanes, ) { + if (!maySuspendCommit(type, props)) { + // If this flag was set previously, we can remove it. The flag + // represents whether this particular set of props might ever need to + // suspend. The safest thing to do is for maySuspendCommit to always + // return true, but if the renderer is reasonably confident that the + // underlying resource won't be evicted, it can return false as a + // performance optimization. + workInProgress.flags &= ~SuspenseyCommit; + return; + } + + // Mark this fiber with a flag. This gets set on all host instances + // that might possibly suspend, even if they don't need to suspend + // currently. We use this when revealing a prerendered tree, because + // even though the tree has "mounted", its resources might not have + // loaded yet. workInProgress.flags |= SuspenseyCommit; + // Check if we're rendering at a "non-urgent" priority. This is the same // check that `useDeferredValue` does to determine whether it needs to // defer. This is partly for gradual adoption purposes (i.e. shouldn't start // suspending until you opt in with startTransition or Suspense) but it // also happens to be the desired behavior for the concrete use cases we've // thought of so far, like CSS loading, fonts, images, etc. + // + // We check the "root" render lanes here rather than the "subtree" render + // because during a retry or offscreen prerender, the "subtree" render + // lanes may include additional "base" lanes that were deferred during + // a previous render. // TODO: We may decide to expose a way to force a fallback even during a // sync update. if (!includesOnlyNonUrgentLanes(renderLanes)) { @@ -553,6 +581,35 @@ function preloadInstanceAndSuspendIfNeeded( } } +function preloadResourceAndSuspendIfNeeded( + workInProgress: Fiber, + resource: Resource, + type: Type, + props: Props, + renderLanes: Lanes, +) { + // This is a fork of preloadInstanceAndSuspendIfNeeded, but for resources. + if (!mayResourceSuspendCommit(resource)) { + workInProgress.flags &= ~SuspenseyCommit; + return; + } + + workInProgress.flags |= SuspenseyCommit; + + if (!includesOnlyNonUrgentLanes(renderLanes)) { + // This is an urgent render. Don't suspend or show a fallback. + } else { + const isReady = preloadResource(resource); + if (!isReady) { + if (shouldRemainOnPreviousScreen()) { + // It's OK to suspend. Continue rendering. + } else { + suspendCommit(); + } + } + } +} + function scheduleRetryEffect( workInProgress: Fiber, retryQueue: RetryQueue | null, @@ -1015,64 +1072,99 @@ function completeWork( } case HostHoistable: { if (enableFloat && supportsResources) { - const currentRef = current ? current.ref : null; - if (currentRef !== workInProgress.ref) { - markRef(workInProgress); - } - - let maySuspend = false; + // The branching here is more complicated than you might expect because + // a HostHoistable sometimes corresponds to a Resource and sometimes + // corresponds to an Instance. It can also switch during an update. - // @TODO refactor this block to create the instance here in complete phase if we - // are not hydrating. - if ( + const type = workInProgress.type; + const nextResource: Resource | null = workInProgress.memoizedState; + if (current === null) { // We are mounting and must Update this Hoistable in this commit - current === null || - // We are transitioning to, from, or between Hoistable Resources - // and require an update - current.memoizedState !== workInProgress.memoizedState - ) { - if (workInProgress.memoizedState !== null) { - maySuspend = mayResourceSuspendCommit(workInProgress.memoizedState); + // @TODO refactor this block to create the instance here in complete + // phase if we are not hydrating. + markUpdate(workInProgress); + if (workInProgress.ref !== null) { + markRef(workInProgress); + } + if (nextResource !== null) { + // This is a Hoistable Resource + + // This must come at the very end of the complete phase. + bubbleProperties(workInProgress); + preloadResourceAndSuspendIfNeeded( + workInProgress, + nextResource, + type, + newProps, + renderLanes, + ); + return null; } else { - maySuspend = maySuspendCommit( - workInProgress.type, - workInProgress.pendingProps, + // This is a Hoistable Instance + + // This must come at the very end of the complete phase. + bubbleProperties(workInProgress); + preloadInstanceAndSuspendIfNeeded( + workInProgress, + type, + newProps, + renderLanes, ); + return null; } - markUpdate(workInProgress); - } else if (workInProgress.memoizedState === null) { - maySuspend = maySuspendCommit( - workInProgress.type, - workInProgress.pendingProps, - ); - // We may have props to update on the Hoistable instance. We use the - // updateHostComponent path becuase it produces the update queue - // we need for Hoistables - updateHostComponent( - current, - workInProgress, - workInProgress.type, - workInProgress.pendingProps, - renderLanes, - ); - } - bubbleProperties(workInProgress); - - // This must come at the very end of the complete phase, because it might - // throw to suspend, and if the resource immediately loads, the work loop - // will resume rendering as if the work-in-progress completed. So it must - // fully complete. - if (maySuspend) { - preloadInstanceAndSuspendIfNeeded( - workInProgress, - workInProgress.type, - workInProgress.pendingProps, - renderLanes, - ); } else { - workInProgress.flags &= ~SuspenseyCommit; + // We are updating. + const currentResource = current.memoizedState; + if (nextResource !== currentResource) { + // We are transitioning to, from, or between Hoistable Resources + // and require an update + markUpdate(workInProgress); + } + if (current.ref !== workInProgress.ref) { + markRef(workInProgress); + } + if (nextResource !== null) { + // This is a Hoistable Resource + // This must come at the very end of the complete phase. + + bubbleProperties(workInProgress); + if (nextResource === currentResource) { + workInProgress.flags &= ~SuspenseyCommit; + } else { + preloadResourceAndSuspendIfNeeded( + workInProgress, + nextResource, + type, + newProps, + renderLanes, + ); + } + return null; + } else { + // This is a Hoistable Instance + // + // We may have props to update on the Hoistable instance. We use the + // updateHostComponent path becuase it produces the update queue + // we need for Hoistables. + updateHostComponent( + current, + workInProgress, + type, + newProps, + renderLanes, + ); + + // This must come at the very end of the complete phase. + bubbleProperties(workInProgress); + preloadInstanceAndSuspendIfNeeded( + workInProgress, + type, + newProps, + renderLanes, + ); + return null; + } } - return null; } } // eslint-disable-next-line-no-fallthrough @@ -1141,7 +1233,6 @@ function completeWork( case HostComponent: { popHostContext(workInProgress); const type = workInProgress.type; - const maySuspend = maySuspendCommit(type, newProps); if (current !== null && workInProgress.stateNode != null) { updateHostComponent( current, @@ -1222,16 +1313,12 @@ function completeWork( // throw to suspend, and if the resource immediately loads, the work loop // will resume rendering as if the work-in-progress completed. So it must // fully complete. - if (maySuspend) { - preloadInstanceAndSuspendIfNeeded( - workInProgress, - type, - newProps, - renderLanes, - ); - } else { - workInProgress.flags &= ~SuspenseyCommit; - } + preloadInstanceAndSuspendIfNeeded( + workInProgress, + workInProgress.type, + workInProgress.pendingProps, + renderLanes, + ); return null; } case HostText: { diff --git a/packages/react-reconciler/src/ReactFiberHostConfigWithNoResources.js b/packages/react-reconciler/src/ReactFiberHostConfigWithNoResources.js index 9680d45dc1616..0ab629a20ee44 100644 --- a/packages/react-reconciler/src/ReactFiberHostConfigWithNoResources.js +++ b/packages/react-reconciler/src/ReactFiberHostConfigWithNoResources.js @@ -19,6 +19,7 @@ function shim(...args: any): empty { } export type HoistableRoot = mixed; +export type Resource = mixed; // Resources (when unsupported) export const supportsResources = false; From ea26d2bcee7558018858edcd4dd8ede7e2eaad9e Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 20 Mar 2023 00:54:43 -0400 Subject: [PATCH 3/4] Suspensey commits in prerendered trees Prerendering a tree (i.e. with Offscreen) should not suspend the commit phase, because the content is not yet visible. However, when revealing a prerendered tree, we should suspend the commit phase if resources in the prerendered tree haven't finished loading yet. To do this properly, we need to visit all the visible nodes in the tree that might possibly suspend. This includes nodes in the current tree, because even though they were already "mounted", the resources might not have loaded yet, because we didn't suspend when it was prerendered. We will need to add this capability to the Offscreen component's "manual" mode, too. Something like a `ready()` method that returns a promise that resolves when the tree has fully loaded. --- .../src/ReactFiberCommitWork.js | 47 +++++++++++-- .../src/ReactFiberCompleteWork.js | 26 ++++--- .../react-reconciler/src/ReactFiberFlags.js | 7 +- .../src/ReactFiberWorkLoop.js | 16 ++++- .../ReactSuspenseyCommitPhase-test.js | 68 ++++++++++++++++++- 5 files changed, 142 insertions(+), 22 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index af086895f3559..18d36674bc27c 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -94,7 +94,8 @@ import { LayoutMask, PassiveMask, Visibility, - SuspenseyCommit, + ShouldSuspendCommit, + MaySuspendCommit, } from './ReactFiberFlags'; import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber'; import { @@ -4065,12 +4066,23 @@ export function commitPassiveUnmountEffects(finishedWork: Fiber): void { resetCurrentDebugFiberInDEV(); } +// If we're inside a brand new tree, or a tree that was already visible, then we +// should only suspend host components that have a ShouldSuspendCommit flag. +// Components without it haven't changed since the last commit, so we can skip +// over those. +// +// When we enter a tree that is being revealed (going from hidden -> visible), +// we need to suspend _any_ component that _may_ suspend. Even if they're +// already in the "current" tree. Because their visibility has changed, the +// browser may not have prerendered them yet. So we check the MaySuspendCommit +// flag instead. +let suspenseyCommitFlag = ShouldSuspendCommit; export function accumulateSuspenseyCommit(finishedWork: Fiber): void { accumulateSuspenseyCommitOnFiber(finishedWork); } function recursivelyAccumulateSuspenseyCommit(parentFiber: Fiber): void { - if (parentFiber.subtreeFlags & SuspenseyCommit) { + if (parentFiber.subtreeFlags & suspenseyCommitFlag) { let child = parentFiber.child; while (child !== null) { accumulateSuspenseyCommitOnFiber(child); @@ -4083,7 +4095,7 @@ function accumulateSuspenseyCommitOnFiber(fiber: Fiber) { switch (fiber.tag) { case HostHoistable: { recursivelyAccumulateSuspenseyCommit(fiber); - if (fiber.flags & SuspenseyCommit) { + if (fiber.flags & suspenseyCommitFlag) { if (fiber.memoizedState !== null) { suspendResource( // This should always be set by visiting HostRoot first @@ -4101,7 +4113,7 @@ function accumulateSuspenseyCommitOnFiber(fiber: Fiber) { } case HostComponent: { recursivelyAccumulateSuspenseyCommit(fiber); - if (fiber.flags & SuspenseyCommit) { + if (fiber.flags & suspenseyCommitFlag) { const type = fiber.type; const props = fiber.memoizedProps; suspendInstance(type, props); @@ -4117,10 +4129,33 @@ function accumulateSuspenseyCommitOnFiber(fiber: Fiber) { recursivelyAccumulateSuspenseyCommit(fiber); currentHoistableRoot = previousHoistableRoot; - break; + } else { + recursivelyAccumulateSuspenseyCommit(fiber); } + break; + } + case OffscreenComponent: { + const isHidden = (fiber.memoizedState: OffscreenState | null) !== null; + if (isHidden) { + // Don't suspend in hidden trees + } else { + const current = fiber.alternate; + const wasHidden = + current !== null && + (current.memoizedState: OffscreenState | null) !== null; + if (wasHidden) { + // This tree is being revealed. Visit all newly visible suspensey + // instances, even if they're in the current tree. + const prevFlags = suspenseyCommitFlag; + suspenseyCommitFlag = MaySuspendCommit; + recursivelyAccumulateSuspenseyCommit(fiber); + suspenseyCommitFlag = prevFlags; + } else { + recursivelyAccumulateSuspenseyCommit(fiber); + } + } + break; } - // eslint-disable-next-line-no-fallthrough default: { recursivelyAccumulateSuspenseyCommit(fiber); } diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index 9aacd549f0a82..1ca6af9b9b800 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -87,8 +87,9 @@ import { MutationMask, Passive, ForceClientRender, - SuspenseyCommit, + MaySuspendCommit, ScheduleRetry, + ShouldSuspendCommit, } from './ReactFiberFlags'; import { @@ -154,6 +155,7 @@ import { getRenderTargetTime, getWorkInProgressTransitions, shouldRemainOnPreviousScreen, + getWorkInProgressRootRenderLanes, } from './ReactFiberWorkLoop'; import { OffscreenLane, @@ -534,7 +536,7 @@ function preloadInstanceAndSuspendIfNeeded( // return true, but if the renderer is reasonably confident that the // underlying resource won't be evicted, it can return false as a // performance optimization. - workInProgress.flags &= ~SuspenseyCommit; + workInProgress.flags &= ~MaySuspendCommit; return; } @@ -543,7 +545,7 @@ function preloadInstanceAndSuspendIfNeeded( // currently. We use this when revealing a prerendered tree, because // even though the tree has "mounted", its resources might not have // loaded yet. - workInProgress.flags |= SuspenseyCommit; + workInProgress.flags |= MaySuspendCommit; // Check if we're rendering at a "non-urgent" priority. This is the same // check that `useDeferredValue` does to determine whether it needs to @@ -558,7 +560,8 @@ function preloadInstanceAndSuspendIfNeeded( // a previous render. // TODO: We may decide to expose a way to force a fallback even during a // sync update. - if (!includesOnlyNonUrgentLanes(renderLanes)) { + const rootRenderLanes = getWorkInProgressRootRenderLanes(); + if (!includesOnlyNonUrgentLanes(rootRenderLanes)) { // This is an urgent render. Don't suspend or show a fallback. Also, // there's no need to preload, because we're going to commit this // synchronously anyway. @@ -572,7 +575,9 @@ function preloadInstanceAndSuspendIfNeeded( const isReady = preloadInstance(type, props); if (!isReady) { if (shouldRemainOnPreviousScreen()) { - // It's OK to suspend. Continue rendering. + // It's OK to suspend. Mark the fiber so we know to suspend before the + // commit phase. Then continue rendering. + workInProgress.flags |= ShouldSuspendCommit; } else { // Trigger a fallback rather than block the render. suspendCommit(); @@ -590,19 +595,20 @@ function preloadResourceAndSuspendIfNeeded( ) { // This is a fork of preloadInstanceAndSuspendIfNeeded, but for resources. if (!mayResourceSuspendCommit(resource)) { - workInProgress.flags &= ~SuspenseyCommit; + workInProgress.flags &= ~MaySuspendCommit; return; } - workInProgress.flags |= SuspenseyCommit; + workInProgress.flags |= MaySuspendCommit; - if (!includesOnlyNonUrgentLanes(renderLanes)) { + const rootRenderLanes = getWorkInProgressRootRenderLanes(); + if (!includesOnlyNonUrgentLanes(rootRenderLanes)) { // This is an urgent render. Don't suspend or show a fallback. } else { const isReady = preloadResource(resource); if (!isReady) { if (shouldRemainOnPreviousScreen()) { - // It's OK to suspend. Continue rendering. + workInProgress.flags |= ShouldSuspendCommit; } else { suspendCommit(); } @@ -1129,7 +1135,7 @@ function completeWork( bubbleProperties(workInProgress); if (nextResource === currentResource) { - workInProgress.flags &= ~SuspenseyCommit; + workInProgress.flags &= ~MaySuspendCommit; } else { preloadResourceAndSuspendIfNeeded( workInProgress, diff --git a/packages/react-reconciler/src/ReactFiberFlags.js b/packages/react-reconciler/src/ReactFiberFlags.js index c37a648973ef0..5febfa3d528dd 100644 --- a/packages/react-reconciler/src/ReactFiberFlags.js +++ b/packages/react-reconciler/src/ReactFiberFlags.js @@ -36,10 +36,11 @@ export const Passive = /* */ 0b0000000000000000100000000000 export const Visibility = /* */ 0b0000000000000010000000000000; export const StoreConsistency = /* */ 0b0000000000000100000000000000; -// It's OK to reuse this bit because these flags are mutually exclusive for +// It's OK to reuse these bits because these flags are mutually exclusive for // different fiber types. We should really be doing this for as many flags as // possible, because we're about to run out of bits. export const ScheduleRetry = StoreConsistency; +export const ShouldSuspendCommit = Visibility; export const LifecycleEffectMask = Passive | Update | Callback | Ref | Snapshot | StoreConsistency; @@ -63,7 +64,7 @@ export const Forked = /* */ 0b0000000100000000000000000000 export const RefStatic = /* */ 0b0000001000000000000000000000; export const LayoutStatic = /* */ 0b0000010000000000000000000000; export const PassiveStatic = /* */ 0b0000100000000000000000000000; -export const SuspenseyCommit = /* */ 0b0001000000000000000000000000; +export const MaySuspendCommit = /* */ 0b0001000000000000000000000000; // Flag used to identify newly inserted fibers. It isn't reset after commit unlike `Placement`. export const PlacementDEV = /* */ 0b0010000000000000000000000000; @@ -103,4 +104,4 @@ export const PassiveMask = Passive | Visibility | ChildDeletion; // This allows certain concepts to persist without recalculating them, // e.g. whether a subtree contains passive effects or portals. export const StaticMask = - LayoutStatic | PassiveStatic | RefStatic | SuspenseyCommit; + LayoutStatic | PassiveStatic | RefStatic | MaySuspendCommit; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index e00d63769062e..14a1d9a4ddf1a 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -169,6 +169,8 @@ import { addTransitionToLanesMap, getTransitionsForLanes, includesOnlyNonUrgentLanes, + includesSomeLane, + OffscreenLane, } from './ReactFiberLane'; import { DiscreteEventPriority, @@ -1997,17 +1999,27 @@ export function shouldRemainOnPreviousScreen(): boolean { // parent Suspense boundary, even outside a transition. Somehow. Otherwise, // an uncached promise can fall into an infinite loop. } else { - if (includesOnlyRetries(workInProgressRootRenderLanes)) { + if ( + includesOnlyRetries(workInProgressRootRenderLanes) || + // In this context, an OffscreenLane counts as a Retry + // TODO: It's become increasingly clear that Retries and Offscreen are + // deeply connected. They probably can be unified further. + includesSomeLane(workInProgressRootRenderLanes, OffscreenLane) + ) { // 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. + // + // The reason we must check if this is a retry is because it guarantees + // that suspending the work loop won't block an actual update, because + // retries don't "update" anything; they fill in fallbacks that were left + // behind by a previous transition. return handler === getShellBoundary(); } } // For all other Lanes besides Transitions and Retries, we should not wait // for the data to load. - // TODO: We should wait during Offscreen prerendering, too. return false; } diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseyCommitPhase-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseyCommitPhase-test.js index 4acdc339e04c8..bbd2ae8c2edcb 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseyCommitPhase-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseyCommitPhase-test.js @@ -4,7 +4,9 @@ let ReactNoop; let resolveSuspenseyThing; let getSuspenseyThingStatus; let Suspense; +let Offscreen; let SuspenseList; +let useMemo; let Scheduler; let act; let assertLog; @@ -18,10 +20,11 @@ describe('ReactSuspenseyCommitPhase', () => { ReactNoop = require('react-noop-renderer'); Scheduler = require('scheduler'); Suspense = React.Suspense; - SuspenseList = React.SuspenseList; if (gate(flags => flags.enableSuspenseList)) { SuspenseList = React.SuspenseList; } + Offscreen = React.unstable_Offscreen; + useMemo = React.useMemo; startTransition = React.startTransition; resolveSuspenseyThing = ReactNoop.resolveSuspenseyThing; getSuspenseyThingStatus = ReactNoop.getSuspenseyThingStatus; @@ -279,4 +282,67 @@ describe('ReactSuspenseyCommitPhase', () => { , ); }); + + // @gate enableOffscreen + test("host instances don't suspend during prerendering, but do suspend when they are revealed", async () => { + function More() { + Scheduler.log('More'); + return ; + } + + function Details({showMore}) { + Scheduler.log('Details'); + const more = useMemo(() => , []); + return ( + <> +
Main Content
+ {more} + + ); + } + + const root = ReactNoop.createRoot(); + await act(async () => { + root.render(
); + // First render the outer component, without the hidden content + await waitForPaint(['Details']); + expect(root).toMatchRenderedOutput(
Main Content
); + }); + // Then prerender the hidden content. + assertLog(['More', 'Image requested [More]']); + // The prerender should commit even though the image is still loading, + // because it's hidden. + expect(root).toMatchRenderedOutput( + <> +
Main Content
+