From 888874673f81c08d9c3cfd4a56e2e93fd728894c Mon Sep 17 00:00:00 2001
From: Andrew Clark
Date: Fri, 31 Mar 2023 15:35:48 -0400
Subject: [PATCH] Allow transitions to interrupt Suspensey commits (#26531)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
I originally made it so that a Suspensey commit — i.e. a commit that's
waiting for a stylesheet, image, or font to load before proceeding —
could not be interrupted by transitions. My reasoning was that Suspensey
commits always time out after a short interval, anyway, so if the
incoming update isn't urgent, it's better to wait to commit the current
frame instead of throwing it away.
I don't think this rationale was correct, for a few reasons. There are
some cases where we'll suspend for a longer duration, like stylesheets —
it's nearly always a bad idea to show content before its styles have
loaded, so we're going to be extend this timeout to be really long.
But even in the case where the timeout is shorter, like fonts, if you
get a new update, it's possible (even likely) that update will allow us
to avoid showing a fallback, like by navigating to a different page. So
we might as well try.
The behavior now matches our behavior for interrupting a suspended
render phase (i.e. `use`), which makes sense because they're not that
conceptually different.
---
.../src/__tests__/ReactDOMFloat-test.js | 67 +++++++------------
.../src/ReactFiberRootScheduler.js | 11 +--
.../src/ReactFiberWorkLoop.js | 7 +-
.../ReactSuspenseyCommitPhase-test.js | 20 +-----
4 files changed, 40 insertions(+), 65 deletions(-)
diff --git a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js
index 5bdca5d43f1f8..d089b25ffad03 100644
--- a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js
+++ b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js
@@ -3216,7 +3216,7 @@ body {
);
});
- it('can start a new suspended commit after a previous one finishes', async () => {
+ it('can interrupt a suspended commit with a new transition', async () => {
function App({children}) {
return (
@@ -3225,81 +3225,66 @@ body {
);
}
const root = ReactDOMClient.createRoot(document);
- root.render();
+ root.render((empty));
+
+ // Start a transition to "A"
React.startTransition(() => {
root.render(
- hello
-
+ A
+
,
);
});
await waitForAll([]);
+
+ // "A" hasn't loaded yet, so we remain on the initial UI. Its preload
+ // has been inserted into the head, though.
expect(getMeaningfulChildren(document)).toEqual(
-
+
-
+ (empty)
,
);
+ // Interrupt the "A" transition with a new one, "B"
React.startTransition(() => {
root.render(
- hello2
- {null}
-
+ B
+
,
);
});
await waitForAll([]);
- expect(getMeaningfulChildren(document)).toEqual(
-
-
-
-
-
- ,
- );
- loadPreloads();
- loadStylesheets();
- assertLog(['load preload: foo', 'load stylesheet: foo']);
+ // Still on the initial UI because "B" hasn't loaded, but its preload
+ // is now in the head, too.
expect(getMeaningfulChildren(document)).toEqual(
-
-
+
+
-
hello
+ (empty)
,
);
- // The second update should process now
- await waitForAll([]);
- expect(getMeaningfulChildren(document)).toEqual(
-
-
-
-
-
-
- hello
- ,
- );
+ // Finish loading
loadPreloads();
loadStylesheets();
- assertLog(['load preload: bar', 'load stylesheet: bar']);
+ assertLog(['load preload: A', 'load preload: B', 'load stylesheet: B']);
+ // The "B" transition has finished.
expect(getMeaningfulChildren(document)).toEqual(
-
-
-
-
+
+
+
- hello2
+ B
,
);
});
diff --git a/packages/react-reconciler/src/ReactFiberRootScheduler.js b/packages/react-reconciler/src/ReactFiberRootScheduler.js
index 5068194aa24cc..be14c4695089c 100644
--- a/packages/react-reconciler/src/ReactFiberRootScheduler.js
+++ b/packages/react-reconciler/src/ReactFiberRootScheduler.js
@@ -18,7 +18,6 @@ import {
SyncLane,
getHighestPriorityLane,
getNextLanes,
- includesOnlyNonUrgentLanes,
includesSyncLane,
markStarvedLanesAsExpired,
} from './ReactFiberLane';
@@ -292,14 +291,16 @@ function scheduleTaskForRootDuringMicrotask(
const existingCallbackNode = root.callbackNode;
if (
+ // Check if there's nothing to work on
nextLanes === NoLanes ||
// If this root is currently suspended and waiting for data to resolve, don't
// schedule a task to render it. We'll either wait for a ping, or wait to
// receive an update.
- (isWorkLoopSuspendedOnData() && root === workInProgressRoot) ||
- // We should only interrupt a pending commit if the new update
- // is urgent.
- (root.cancelPendingCommit !== null && includesOnlyNonUrgentLanes(nextLanes))
+ //
+ // Suspended render phase
+ (root === workInProgressRoot && isWorkLoopSuspendedOnData()) ||
+ // Suspended commit phase
+ root.cancelPendingCommit !== null
) {
// Fast path: There's nothing to work on.
if (existingCallbackNode !== null) {
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js
index 55c30ae81a896..71e6f4feabe41 100644
--- a/packages/react-reconciler/src/ReactFiberWorkLoop.js
+++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js
@@ -725,8 +725,11 @@ export function scheduleUpdateOnFiber(
// Check if the work loop is currently suspended and waiting for data to
// finish loading.
if (
- workInProgressSuspendedReason === SuspendedOnData &&
- root === workInProgressRoot
+ // Suspended render phase
+ (root === workInProgressRoot &&
+ workInProgressSuspendedReason === SuspendedOnData) ||
+ // Suspended commit phase
+ root.cancelPendingCommit !== null
) {
// The incoming update might unblock the current render. Interrupt the
// current attempt and restart from the top.
diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseyCommitPhase-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseyCommitPhase-test.js
index bbd2ae8c2edcb..2a558ba93a523 100644
--- a/packages/react-reconciler/src/__tests__/ReactSuspenseyCommitPhase-test.js
+++ b/packages/react-reconciler/src/__tests__/ReactSuspenseyCommitPhase-test.js
@@ -137,7 +137,7 @@ describe('ReactSuspenseyCommitPhase', () => {
// Nothing showing yet.
expect(root).toMatchRenderedOutput(null);
- // If there's an urgent update, it should interrupt the suspended commit.
+ // If there's an update, it should interrupt the suspended commit.
await act(() => {
root.render();
});
@@ -145,7 +145,7 @@ describe('ReactSuspenseyCommitPhase', () => {
expect(root).toMatchRenderedOutput('Something else');
});
- test('a non-urgent update does not interrupt a suspended commit', async () => {
+ test('a transition update interrupts a suspended commit', async () => {
const root = ReactNoop.createRoot();
// Mount an image. This transition will suspend because it's not inside a
@@ -159,26 +159,12 @@ describe('ReactSuspenseyCommitPhase', () => {
// Nothing showing yet.
expect(root).toMatchRenderedOutput(null);
- // If there's another transition update, it should not interrupt the
- // suspended commit.
+ // If there's an update, it should interrupt the suspended commit.
await act(() => {
startTransition(() => {
root.render();
});
});
- // Still suspended.
- expect(root).toMatchRenderedOutput(null);
-
- await act(() => {
- // Resolving the image should result in an immediate, synchronous commit.
- resolveSuspenseyThing('A');
- expect(root).toMatchRenderedOutput();
- });
- // Then the second transition is unblocked.
- // TODO: Right now the only way to unsuspend a commit early is to proceed
- // with the commit even if everything isn't ready. Maybe there should also
- // be a way to abort a commit so that it can be interrupted by
- // another transition.
assertLog(['Something else']);
expect(root).toMatchRenderedOutput('Something else');
});