From b1062aa4595338e89b9fc69777419c850bb2001e Mon Sep 17 00:00:00 2001 From: sunderls <2394070+sunderls@users.noreply.github.com> Date: Sat, 2 Apr 2022 00:13:34 +0100 Subject: [PATCH 1/3] fix suspense throttling --- .../src/ReactFiberCommitWork.new.js | 18 ++++--- .../src/ReactFiberCommitWork.old.js | 18 ++++--- .../__tests__/ReactSuspense-test.internal.js | 51 +++++++++++++++++++ 3 files changed, 71 insertions(+), 16 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index fb6040109518..f3a447e0c2d2 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -2250,22 +2250,25 @@ function commitMutationEffectsOnFiber( } } - if (flags & Visibility) { - switch (finishedWork.tag) { - case SuspenseComponent: { - const newState: OffscreenState | null = finishedWork.memoizedState; + switch (finishedWork.tag) { + case SuspenseComponent: { + const offscreenFiber: Fiber = (finishedWork.child: any); + if (offscreenFiber.flags & Visibility) { + const newState: OffscreenState | null = offscreenFiber.memoizedState; const isHidden = newState !== null; if (isHidden) { - const current = finishedWork.alternate; + const current = offscreenFiber.alternate; const wasHidden = current !== null && current.memoizedState !== null; if (!wasHidden) { // TODO: Move to passive phase markCommitTimeOfFallback(); } } - break; } - case OffscreenComponent: { + break; + } + case OffscreenComponent: { + if (flags & Visibility) { const newState: OffscreenState | null = finishedWork.memoizedState; const isHidden = newState !== null; const current = finishedWork.alternate; @@ -2301,7 +2304,6 @@ function commitMutationEffectsOnFiber( } } } - // The following switch statement is only concerned about placement, // updates, and deletions. To avoid needing to add a case for every possible // bitmap value, we remove the secondary effects from the effect tag and diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index e962039d9ca9..4a93e4dc94d8 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -2250,22 +2250,25 @@ function commitMutationEffectsOnFiber( } } - if (flags & Visibility) { - switch (finishedWork.tag) { - case SuspenseComponent: { - const newState: OffscreenState | null = finishedWork.memoizedState; + switch (finishedWork.tag) { + case SuspenseComponent: { + const offscreenFiber: Fiber = (finishedWork.child: any); + if (offscreenFiber.flags & Visibility) { + const newState: OffscreenState | null = offscreenFiber.memoizedState; const isHidden = newState !== null; if (isHidden) { - const current = finishedWork.alternate; + const current = offscreenFiber.alternate; const wasHidden = current !== null && current.memoizedState !== null; if (!wasHidden) { // TODO: Move to passive phase markCommitTimeOfFallback(); } } - break; } - case OffscreenComponent: { + break; + } + case OffscreenComponent: { + if (flags & Visibility) { const newState: OffscreenState | null = finishedWork.memoizedState; const isHidden = newState !== null; const current = finishedWork.alternate; @@ -2301,7 +2304,6 @@ function commitMutationEffectsOnFiber( } } } - // The following switch statement is only concerned about placement, // updates, and deletions. To avoid needing to add a case for every possible // bitmap value, we remove the secondary effects from the effect tag and diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index 82ee6ee6bf1d..8a020b4e73c8 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -285,6 +285,57 @@ describe('ReactSuspense', () => { expect(root).toMatchRenderedOutput('AsyncAfter SuspenseSibling'); }); + it('throttles fallback committing globally', () => { + function Foo() { + Scheduler.unstable_yieldValue('Foo'); + return ( + }> + + }> + + + + ); + } + + // throttling works on fallback committing + // here 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...']); + + // should still renders previous fallback + expect(root).toMatchRenderedOutput('Loading...'); + + // resolve B + jest.advanceTimersByTime(100); + Scheduler.unstable_advanceTime(100); + expect(Scheduler).toHaveYielded(['Promise resolved [B]']); + + // before commiting we still shows previous fallback + expect(root).toMatchRenderedOutput('Loading...'); + expect(Scheduler).toFlushAndYield(['A', 'B']); + expect(root).toMatchRenderedOutput('AB'); + }); + // @gate !enableSyncDefaultUpdates it( 'interrupts current render when something suspends with a ' + From d89c87159168c769ada29e865c88518fc20e2e5f Mon Sep 17 00:00:00 2001 From: sunderls <2394070+sunderls@users.noreply.github.com> Date: Sat, 9 Apr 2022 20:47:03 +0100 Subject: [PATCH 2/3] fix lint --- packages/react-reconciler/src/ReactFiberCommitWork.new.js | 5 +++-- packages/react-reconciler/src/ReactFiberCommitWork.old.js | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index c2ecca08c518..708799636dae 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -2215,8 +2215,9 @@ function commitMutationEffectsOnFiber( const newState: OffscreenState | null = offscreenFiber.memoizedState; const isHidden = newState !== null; if (isHidden) { - const current = offscreenFiber.alternate; - const wasHidden = current !== null && current.memoizedState !== null; + const wasHidden = + offscreenFiber.alternate !== null && + offscreenFiber.alternate.memoizedState !== null; if (!wasHidden) { // TODO: Move to passive phase markCommitTimeOfFallback(); diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 1a28f3c387da..37cff9c313a6 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -2215,8 +2215,9 @@ function commitMutationEffectsOnFiber( const newState: OffscreenState | null = offscreenFiber.memoizedState; const isHidden = newState !== null; if (isHidden) { - const current = offscreenFiber.alternate; - const wasHidden = current !== null && current.memoizedState !== null; + const wasHidden = + offscreenFiber.alternate !== null && + offscreenFiber.alternate.memoizedState !== null; if (!wasHidden) { // TODO: Move to passive phase markCommitTimeOfFallback(); From 8bd48ca14e3a17471c860a21e4688028962a606f Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 11 Apr 2022 21:09:33 +0100 Subject: [PATCH 3/3] Tweak tests + another test --- .../__tests__/ReactSuspense-test.internal.js | 72 +++++++++++++++++-- 1 file changed, 66 insertions(+), 6 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index 8a020b4e73c8..7adc0345729f 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -298,8 +298,8 @@ describe('ReactSuspense', () => { ); } - // throttling works on fallback committing - // here advance some time to skip the first threshold + // Committing fallbacks should be throttled. + // First, advance some time to skip the first threshold. jest.advanceTimersByTime(600); Scheduler.unstable_advanceTime(600); @@ -316,23 +316,83 @@ describe('ReactSuspense', () => { ]); expect(root).toMatchRenderedOutput('Loading...'); - // resolve A + // Resolve A. jest.advanceTimersByTime(200); Scheduler.unstable_advanceTime(200); expect(Scheduler).toHaveYielded(['Promise resolved [A]']); expect(Scheduler).toFlushAndYield(['A', 'Suspend! [B]', 'Loading more...']); - // should still renders previous fallback + // 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 + // Resolve B. jest.advanceTimersByTime(100); Scheduler.unstable_advanceTime(100); expect(Scheduler).toHaveYielded(['Promise resolved [B]']); - // before commiting we still shows previous fallback + // 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'); });