Allow transitions to interrupt Suspensey commits (#26531)
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.
Andrew Clark committed
Mar 31, 2023 at 15:35 UTC
888874673f81c08d9c3cfd4a56e2e93fd728894c
4 files changed
+40
-65
packages/react-dom/src/__tests__/ReactDOMFloat-test.js
+26
-41
@@ -3216,7 +3216,7 @@ body {
3216
);
3217
});
3218
3219
- it('can start a new suspended commit after a previous one finishes', async () => {
3219
+ it('can interrupt a suspended commit with a new transition', async () => {
3220
function App({children}) {
3221
return (
3222
<html>
@@ -3225,81 +3225,66 @@ body {
3225
);
3226
}
3227
const root = ReactDOMClient.createRoot(document);
3228
- root.render(<App />);
3228
+ root.render(<App>(empty)</App>);
3229
+
3230
+ // Start a transition to "A"
3231
React.startTransition(() => {
3232
root.render(
3233
<App>
3232
- hello
3233
- <link rel="stylesheet" href="foo" precedence="default" />
3234
+ A
3235
+ <link rel="stylesheet" href="A" precedence="default" />
3236
</App>,
3237
);
3238
});
3239
await waitForAll([]);
3240
+
3241
+ // "A" hasn't loaded yet, so we remain on the initial UI. Its preload
3242
+ // has been inserted into the head, though.
3243
expect(getMeaningfulChildren(document)).toEqual(
3244
<html>
3245
<head>
3241
- <link rel="preload" href="foo" as="style" />
3246
+ <link rel="preload" href="A" as="style" />
3247
</head>
3243
- <body />
3248
+ <body>(empty)</body>
3249
</html>,
3250
);
3251
3252
+ // Interrupt the "A" transition with a new one, "B"
3253
React.startTransition(() => {
3254
root.render(
3255
<App>
3250
- hello2
3251
- {null}
3252
- <link rel="stylesheet" href="bar" precedence="default" />
3256
+ B
3257
+ <link rel="stylesheet" href="B" precedence="default" />
3258
</App>,
3259
);
3260
});
3261
await waitForAll([]);
3257
- expect(getMeaningfulChildren(document)).toEqual(
3258
- <html>
3259
- <head>
3260
- <link rel="preload" href="foo" as="style" />
3261
- </head>
3262
- <body />
3263
- </html>,
3264
- );
3262
3266
- loadPreloads();
3267
- loadStylesheets();
3268
- assertLog(['load preload: foo', 'load stylesheet: foo']);
3263
+ // Still on the initial UI because "B" hasn't loaded, but its preload
3264
+ // is now in the head, too.
3265
expect(getMeaningfulChildren(document)).toEqual(
3266
<html>
3267
<head>
3272
- <link rel="stylesheet" href="foo" data-precedence="default" />
3273
- <link rel="preload" href="foo" as="style" />
3268
+ <link rel="preload" href="A" as="style" />
3269
+ <link rel="preload" href="B" as="style" />
3270
</head>
3275
- <body>hello</body>
3271
+ <body>(empty)</body>
3272
</html>,
3273
);
3274
3279
- // The second update should process now
3280
- await waitForAll([]);
3281
- expect(getMeaningfulChildren(document)).toEqual(
3282
- <html>
3283
- <head>
3284
- <link rel="stylesheet" href="foo" data-precedence="default" />
3285
- <link rel="preload" href="foo" as="style" />
3286
- <link rel="preload" href="bar" as="style" />
3287
- </head>
3288
- <body>hello</body>
3289
- </html>,
3290
- );
3275
+ // Finish loading
3276
loadPreloads();
3277
loadStylesheets();
3293
- assertLog(['load preload: bar', 'load stylesheet: bar']);
3278
+ assertLog(['load preload: A', 'load preload: B', 'load stylesheet: B']);
3279
+ // The "B" transition has finished.
3280
expect(getMeaningfulChildren(document)).toEqual(
3281
<html>
3282
<head>
3297
- <link rel="stylesheet" href="foo" data-precedence="default" />
3298
- <link rel="stylesheet" href="bar" data-precedence="default" />
3299
- <link rel="preload" href="foo" as="style" />
3300
- <link rel="preload" href="bar" as="style" />
3283
+ <link rel="stylesheet" href="B" data-precedence="default" />
3284
+ <link rel="preload" href="A" as="style" />
3285
+ <link rel="preload" href="B" as="style" />
3286
</head>
3302
- <body>hello2</body>
3287
+ <body>B</body>
3288
</html>,
3289
);
3290
});
packages/react-reconciler/src/ReactFiberRootScheduler.js
+6
-5
@@ -18,7 +18,6 @@ import {
18
SyncLane,
19
getHighestPriorityLane,
20
getNextLanes,
21
- includesOnlyNonUrgentLanes,
21
includesSyncLane,
22
markStarvedLanesAsExpired,
23
} from './ReactFiberLane';
@@ -292,14 +291,16 @@ function scheduleTaskForRootDuringMicrotask(
291
292
const existingCallbackNode = root.callbackNode;
293
if (
294
+ // Check if there's nothing to work on
295
nextLanes === NoLanes ||
296
// If this root is currently suspended and waiting for data to resolve, don't
297
// schedule a task to render it. We'll either wait for a ping, or wait to
298
// receive an update.
299
- (isWorkLoopSuspendedOnData() && root === workInProgressRoot) ||
300
- // We should only interrupt a pending commit if the new update
301
- // is urgent.
302
- (root.cancelPendingCommit !== null && includesOnlyNonUrgentLanes(nextLanes))
299
+ //
300
+ // Suspended render phase
301
+ (root === workInProgressRoot && isWorkLoopSuspendedOnData()) ||
302
+ // Suspended commit phase
303
+ root.cancelPendingCommit !== null
304
) {
305
// Fast path: There's nothing to work on.
306
if (existingCallbackNode !== null) {
packages/react-reconciler/src/ReactFiberWorkLoop.js
+5
-2
@@ -725,8 +725,11 @@ export function scheduleUpdateOnFiber(
725
// Check if the work loop is currently suspended and waiting for data to
726
// finish loading.
727
if (
728
- workInProgressSuspendedReason === SuspendedOnData &&
729
- root === workInProgressRoot
728
+ // Suspended render phase
729
+ (root === workInProgressRoot &&
730
+ workInProgressSuspendedReason === SuspendedOnData) ||
731
+ // Suspended commit phase
732
+ root.cancelPendingCommit !== null
733
) {
734
// The incoming update might unblock the current render. Interrupt the
735
// current attempt and restart from the top.
packages/react-reconciler/src/__tests__/ReactSuspenseyCommitPhase-test.js
+3
-17
@@ -137,7 +137,7 @@ describe('ReactSuspenseyCommitPhase', () => {
137
// Nothing showing yet.
138
expect(root).toMatchRenderedOutput(null);
139
140
- // If there's an urgent update, it should interrupt the suspended commit.
140
+ // If there's an update, it should interrupt the suspended commit.
141
await act(() => {
142
root.render(<Text text="Something else" />);
143
});
@@ -145,7 +145,7 @@ describe('ReactSuspenseyCommitPhase', () => {
145
expect(root).toMatchRenderedOutput('Something else');
146
});
147
148
- test('a non-urgent update does not interrupt a suspended commit', async () => {
148
+ test('a transition update interrupts a suspended commit', async () => {
149
const root = ReactNoop.createRoot();
150
151
// Mount an image. This transition will suspend because it's not inside a
@@ -159,26 +159,12 @@ describe('ReactSuspenseyCommitPhase', () => {
159
// Nothing showing yet.
160
expect(root).toMatchRenderedOutput(null);
161
162
- // If there's another transition update, it should not interrupt the
163
- // suspended commit.
162
+ // If there's an update, it should interrupt the suspended commit.
163
await act(() => {
164
startTransition(() => {
165
root.render(<Text text="Something else" />);
166
});
167
});
169
- // Still suspended.
170
- expect(root).toMatchRenderedOutput(null);
171
-
172
- await act(() => {
173
- // Resolving the image should result in an immediate, synchronous commit.
174
- resolveSuspenseyThing('A');
175
- expect(root).toMatchRenderedOutput(<suspensey-thing src="A" />);
176
- });
177
- // Then the second transition is unblocked.
178
- // TODO: Right now the only way to unsuspend a commit early is to proceed
179
- // with the commit even if everything isn't ready. Maybe there should also
180
- // be a way to abort a commit so that it can be interrupted by
181
- // another transition.
168
assertLog(['Something else']);
169
expect(root).toMatchRenderedOutput('Something else');
170
});