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-dom/src/__tests__/ReactDOMFloat-test.js b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js index 7c9e3e75dc533..5bdca5d43f1f8 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js @@ -2939,7 +2939,6 @@ body { ); }); - // @gate TODO it('can interrupt a suspended commit with a new update', async () => { function App({children}) { return ( @@ -2949,9 +2948,13 @@ body { ); } const root = ReactDOMClient.createRoot(document); + + // Do an initial render. This means subsequent insertions will suspend, + // unless they are wrapped inside a fresh Suspense boundary. root.render(); await waitForAll([]); + // Insert a stylesheet. This will suspend because it's a transition. React.startTransition(() => { root.render( @@ -2961,6 +2964,7 @@ body { ); }); await waitForAll([]); + // Although the commit suspended, a preload was inserted. expect(getMeaningfulChildren(document)).toEqual( @@ -2970,6 +2974,9 @@ body { , ); + // Before the stylesheet has loaded, do an urgent update. This will insert a + // different stylesheet, and cancel the first one. This stylesheet will not + // suspend, even though it hasn't loaded, because it's an urgent update. root.render( hello2 @@ -2978,6 +2985,9 @@ body { , ); await waitForAll([]); + + // The bar stylesheet was inserted. There's still a "foo" preload, even + // though that update was superseded. expect(getMeaningfulChildren(document)).toEqual( @@ -2989,9 +2999,10 @@ body { , ); - // Even though foo was preloaded we don't see the stylesheet insert because the commit was cancelled. - // If we do a followup render that tries to recommit that resource it will insert right away because - // the preload is already loaded + // When "foo" finishes loading, nothing happens, because "foo" was not + // included in the last root update. However, if we insert "foo" again + // later, it should immediately commit without suspending, because it's + // been preloaded. loadPreloads(['foo']); assertLog(['load preload: foo']); expect(getMeaningfulChildren(document)).toEqual( @@ -3005,6 +3016,7 @@ body { , ); + // Now insert "foo" again. React.startTransition(() => { root.render( @@ -3015,6 +3027,7 @@ body { ); }); await waitForAll([]); + // Commits without suspending because "foo" was preloaded. expect(getMeaningfulChildren(document)).toEqual( @@ -3023,7 +3036,7 @@ body { - hello2 + hello3 , ); 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 d2d72562183dc..1ca6af9b9b800 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, @@ -86,8 +87,9 @@ import { MutationMask, Passive, ForceClientRender, - SuspenseyCommit, + MaySuspendCommit, ScheduleRetry, + ShouldSuspendCommit, } from './ReactFiberFlags'; import { @@ -112,6 +114,7 @@ import { maySuspendCommit, mayResourceSuspendCommit, preloadInstance, + preloadResource, } from './ReactFiberHostConfig'; import { getRootHostContainer, @@ -152,6 +155,7 @@ import { getRenderTargetTime, getWorkInProgressTransitions, shouldRemainOnPreviousScreen, + getWorkInProgressRootRenderLanes, } from './ReactFiberWorkLoop'; import { OffscreenLane, @@ -511,6 +515,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,16 +529,39 @@ function preloadInstanceAndSuspendIfNeeded( props: Props, renderLanes: Lanes, ) { - workInProgress.flags |= SuspenseyCommit; + 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 &= ~MaySuspendCommit; + 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 |= 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 // 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)) { + 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. @@ -544,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(); @@ -553,6 +586,36 @@ 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 &= ~MaySuspendCommit; + return; + } + + workInProgress.flags |= MaySuspendCommit; + + 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()) { + workInProgress.flags |= ShouldSuspendCommit; + } else { + suspendCommit(); + } + } + } +} + function scheduleRetryEffect( workInProgress: Fiber, retryQueue: RetryQueue | null, @@ -1015,64 +1078,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 &= ~MaySuspendCommit; + } 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 +1239,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 +1319,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/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/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; 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
+