diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index b540929ce0352..1960a4707e244 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -55,6 +55,7 @@ import { enableLegacyHidden, enableHostSingletons, diffInCommitPhase, + alwaysThrottleRetries, } from 'shared/ReactFeatureFlags'; import { FunctionComponent, @@ -2905,17 +2906,35 @@ function commitMutationEffectsOnFiber( recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); + // TODO: We should mark a flag on the Suspense fiber itself, rather than + // relying on the Offscreen fiber having a flag also being marked. The + // reason is that this offscreen fiber might not be part of the work-in- + // progress tree! It could have been reused from a previous render. This + // doesn't lead to incorrect behavior because we don't rely on the flag + // check alone; we also compare the states explicitly below. But for + // modeling purposes, we _should_ be able to rely on the flag check alone. + // So this is a bit fragile. + // + // Also, all this logic could/should move to the passive phase so it + // doesn't block paint. const offscreenFiber: Fiber = (finishedWork.child: any); - if (offscreenFiber.flags & Visibility) { - const newState: OffscreenState | null = offscreenFiber.memoizedState; - const isHidden = newState !== null; - if (isHidden) { - const wasHidden = - offscreenFiber.alternate !== null && - offscreenFiber.alternate.memoizedState !== null; - if (!wasHidden) { - // TODO: Move to passive phase + // Throttle the appearance and disappearance of Suspense fallbacks. + const isShowingFallback = + (finishedWork.memoizedState: SuspenseState | null) !== null; + const wasShowingFallback = + current !== null && + (current.memoizedState: SuspenseState | null) !== null; + + if (alwaysThrottleRetries) { + if (isShowingFallback !== wasShowingFallback) { + // A fallback is either appearing or disappearing. + markCommitTimeOfFallback(); + } + } else { + if (isShowingFallback && !wasShowingFallback) { + // Old behavior. Only mark when a fallback appears, not when + // it disappears. markCommitTimeOfFallback(); } } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index f0a0d4e228160..f6d1d7f7a5849 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -370,8 +370,10 @@ let workInProgressRootConcurrentErrors: Array> | null = let workInProgressRootRecoverableErrors: Array> | null = null; -// The most recent time we committed a fallback. This lets us ensure a train -// model where we don't commit new loading states in too quick succession. +// The most recent time we either committed a fallback, or when a fallback was +// filled in with the resolved UI. This lets us throttle the appearance of new +// content as it streams in, to minimize jank. +// TODO: Think of a better name for this variable? let globalMostRecentFallbackTime: number = 0; const FALLBACK_THROTTLE_MS: number = 500; diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js index e6019ffd40583..fc1aa38708b8d 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js @@ -1811,6 +1811,102 @@ describe('ReactSuspenseWithNoopRenderer', () => { ); }); + // @gate enableLegacyCache + it('throttles content from appearing if a fallback was filled in recently', async () => { + function Foo() { + Scheduler.log('Foo'); + return ( + <> + }> + + + }> + + + + ); + } + + ReactNoop.render(); + // Start rendering + await waitForAll([ + 'Foo', + 'Suspend! [A]', + 'Loading A...', + 'Suspend! [B]', + 'Loading B...', + ]); + expect(ReactNoop).toMatchRenderedOutput( + <> + + + , + ); + + // Resolve only A. B will still be loading. + await act(async () => { + await resolveText('A'); + + // If we didn't advance the time here, A would not commit; it would + // be throttled because the fallback would have appeared too recently. + Scheduler.unstable_advanceTime(10000); + jest.advanceTimersByTime(10000); + await waitForPaint(['A']); + expect(ReactNoop).toMatchRenderedOutput( + <> + + + , + ); + }); + + // Advance by a small amount of time. For testing purposes, this is meant + // to be just under the throttling interval. It's a heurstic, though, so + // if we adjust the heuristic we might have to update this test, too. + Scheduler.unstable_advanceTime(400); + jest.advanceTimersByTime(400); + + // Now resolve B. + await act(async () => { + await resolveText('B'); + await waitForPaint(['B']); + + if (gate(flags => flags.alwaysThrottleRetries)) { + // B should not commit yet. Even though it's been a long time since its + // fallback was shown, it hasn't been long since A appeared. So B's + // appearance is throttled to reduce jank. + expect(ReactNoop).toMatchRenderedOutput( + <> + + + , + ); + + // Advance time a little bit more. Now it commits because enough time + // has passed. + Scheduler.unstable_advanceTime(100); + jest.advanceTimersByTime(100); + await waitForAll([]); + expect(ReactNoop).toMatchRenderedOutput( + <> + + + , + ); + } else { + // Old behavior, gated until this rolls out at Meta: + // + // B appears immediately, without being throttled. + expect(ReactNoop).toMatchRenderedOutput( + <> + + + , + ); + } + }); + }); + // TODO: flip to "warns" when this is implemented again. // @gate enableLegacyCache it('does not warn when a low priority update suspends inside a high priority update for functional components', async () => {