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 fb60401095184..f3a447e0c2d20 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 e962039d9ca9c..4a93e4dc94d82 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 82ee6ee6bf1dc..8a020b4e73c87 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 c2ecca08c5182..708799636dae6 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 1a28f3c387dac..37cff9c313a68 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 8a020b4e73c87..7adc0345729fc 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');
});