From c6cca648e628c79eca2edb59da48bdfd64bdc700 Mon Sep 17 00:00:00 2001 From: sunderls <2394070+sunderls@users.noreply.github.com> Date: Tue, 12 Apr 2022 15:42:05 +0100 Subject: [PATCH] Fix suspense fallback throttling (#24253) * fix suspense throttling * fix lint * Tweak tests + another test Co-authored-by: Dan Abramov --- .../src/ReactFiberCommitWork.new.js | 11 +- .../src/ReactFiberCommitWork.old.js | 11 +- .../__tests__/ReactSuspense-test.internal.js | 111 ++++++++++++++++++ 3 files changed, 127 insertions(+), 6 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index e829e0252cdd..9fb21574e58d 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -2221,17 +2221,22 @@ function commitMutationEffectsOnFiber( recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); - if (flags & Visibility) { - const newState: OffscreenState | null = finishedWork.memoizedState; + const offscreenFiber: Fiber = (finishedWork.child: any); + + if (offscreenFiber.flags & Visibility) { + const newState: OffscreenState | null = offscreenFiber.memoizedState; const isHidden = newState !== null; if (isHidden) { - const wasHidden = current !== null && current.memoizedState !== null; + const wasHidden = + offscreenFiber.alternate !== null && + offscreenFiber.alternate.memoizedState !== null; if (!wasHidden) { // TODO: Move to passive phase markCommitTimeOfFallback(); } } } + if (flags & Update) { try { commitSuspenseCallback(finishedWork); diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 719c3d40fb60..7b4554d41230 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -2221,17 +2221,22 @@ function commitMutationEffectsOnFiber( recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); - if (flags & Visibility) { - const newState: OffscreenState | null = finishedWork.memoizedState; + const offscreenFiber: Fiber = (finishedWork.child: any); + + if (offscreenFiber.flags & Visibility) { + const newState: OffscreenState | null = offscreenFiber.memoizedState; const isHidden = newState !== null; if (isHidden) { - const wasHidden = current !== null && current.memoizedState !== null; + const wasHidden = + offscreenFiber.alternate !== null && + offscreenFiber.alternate.memoizedState !== null; if (!wasHidden) { // TODO: Move to passive phase markCommitTimeOfFallback(); } } } + if (flags & Update) { try { commitSuspenseCallback(finishedWork); diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index 82ee6ee6bf1d..7adc0345729f 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -285,6 +285,117 @@ describe('ReactSuspense', () => { expect(root).toMatchRenderedOutput('AsyncAfter SuspenseSibling'); }); + it('throttles fallback committing globally', () => { + function Foo() { + Scheduler.unstable_yieldValue('Foo'); + return ( + }> + + }> + + + + ); + } + + // Committing fallbacks should be throttled. + // First, advance some time to skip the first threshold. + jest.advanceTimersByTime(600); + Scheduler.unstable_advanceTime(600); + + const root = ReactTestRenderer.create(, { + unstable_isConcurrent: true, + }); + + expect(Scheduler).toFlushAndYield([ + 'Foo', + 'Suspend! [A]', + 'Suspend! [B]', + 'Loading more...', + 'Loading...', + ]); + expect(root).toMatchRenderedOutput('Loading...'); + + // Resolve A. + jest.advanceTimersByTime(200); + Scheduler.unstable_advanceTime(200); + expect(Scheduler).toHaveYielded(['Promise resolved [A]']); + expect(Scheduler).toFlushAndYield(['A', 'Suspend! [B]', 'Loading more...']); + + // By this point, we have enough info to show "A" and "Loading more..." + // However, we've just shown the outer fallback. So we'll delay + // showing the inner fallback hoping that B will resolve soon enough. + expect(root).toMatchRenderedOutput('Loading...'); + + // Resolve B. + jest.advanceTimersByTime(100); + Scheduler.unstable_advanceTime(100); + expect(Scheduler).toHaveYielded(['Promise resolved [B]']); + + // By this point, B has resolved. + // We're still showing the outer fallback. + expect(root).toMatchRenderedOutput('Loading...'); + expect(Scheduler).toFlushAndYield(['A', 'B']); + // Then contents of both should pop in together. + expect(root).toMatchRenderedOutput('AB'); + }); + + it('does not throttle fallback committing for too long', () => { + function Foo() { + Scheduler.unstable_yieldValue('Foo'); + return ( + }> + + }> + + + + ); + } + + // Committing fallbacks should be throttled. + // First, advance some time to skip the first threshold. + jest.advanceTimersByTime(600); + Scheduler.unstable_advanceTime(600); + + const root = ReactTestRenderer.create(, { + unstable_isConcurrent: true, + }); + + expect(Scheduler).toFlushAndYield([ + 'Foo', + 'Suspend! [A]', + 'Suspend! [B]', + 'Loading more...', + 'Loading...', + ]); + expect(root).toMatchRenderedOutput('Loading...'); + + // Resolve A. + jest.advanceTimersByTime(200); + Scheduler.unstable_advanceTime(200); + expect(Scheduler).toHaveYielded(['Promise resolved [A]']); + expect(Scheduler).toFlushAndYield(['A', 'Suspend! [B]', 'Loading more...']); + + // By this point, we have enough info to show "A" and "Loading more..." + // However, we've just shown the outer fallback. So we'll delay + // showing the inner fallback hoping that B will resolve soon enough. + expect(root).toMatchRenderedOutput('Loading...'); + + // Wait some more. B is still not resolving. + jest.advanceTimersByTime(500); + Scheduler.unstable_advanceTime(500); + // Give up and render A with a spinner for B. + expect(root).toMatchRenderedOutput('ALoading more...'); + + // Resolve B. + jest.advanceTimersByTime(500); + Scheduler.unstable_advanceTime(500); + expect(Scheduler).toHaveYielded(['Promise resolved [B]']); + expect(Scheduler).toFlushAndYield(['B']); + expect(root).toMatchRenderedOutput('AB'); + }); + // @gate !enableSyncDefaultUpdates it( 'interrupts current render when something suspends with a ' +