Fix suspense fallback throttling (#24253)
* fix suspense throttling * fix lint * Tweak tests + another test Co-authored-by: Dan Abramov <dan.abramov@gmail.com>
sunderls committed
Apr 12, 2022 at 15:42 UTC
4fc394bbecdf55e46d192536e1a545376f90d84b
3 files changed
+127
-6
packages/react-reconciler/src/ReactFiberCommitWork.new.js
+8
-3
@@ -2209,17 +2209,22 @@ function commitMutationEffectsOnFiber(
2209
recursivelyTraverseMutationEffects(root, finishedWork, lanes);
2210
commitReconciliationEffects(finishedWork);
2211
2212
- if (flags & Visibility) {
2213
- const newState: OffscreenState | null = finishedWork.memoizedState;
2212
+ const offscreenFiber: Fiber = (finishedWork.child: any);
2213
+
2214
+ if (offscreenFiber.flags & Visibility) {
2215
+ const newState: OffscreenState | null = offscreenFiber.memoizedState;
2216
const isHidden = newState !== null;
2217
if (isHidden) {
2216
- const wasHidden = current !== null && current.memoizedState !== null;
2218
+ const wasHidden =
2219
+ offscreenFiber.alternate !== null &&
2220
+ offscreenFiber.alternate.memoizedState !== null;
2221
if (!wasHidden) {
2222
// TODO: Move to passive phase
2223
markCommitTimeOfFallback();
2224
}
2225
}
2226
}
2227
+
2228
if (flags & Update) {
2229
try {
2230
commitSuspenseCallback(finishedWork);
packages/react-reconciler/src/ReactFiberCommitWork.old.js
+8
-3
@@ -2209,17 +2209,22 @@ function commitMutationEffectsOnFiber(
2209
recursivelyTraverseMutationEffects(root, finishedWork, lanes);
2210
commitReconciliationEffects(finishedWork);
2211
2212
- if (flags & Visibility) {
2213
- const newState: OffscreenState | null = finishedWork.memoizedState;
2212
+ const offscreenFiber: Fiber = (finishedWork.child: any);
2213
+
2214
+ if (offscreenFiber.flags & Visibility) {
2215
+ const newState: OffscreenState | null = offscreenFiber.memoizedState;
2216
const isHidden = newState !== null;
2217
if (isHidden) {
2216
- const wasHidden = current !== null && current.memoizedState !== null;
2218
+ const wasHidden =
2219
+ offscreenFiber.alternate !== null &&
2220
+ offscreenFiber.alternate.memoizedState !== null;
2221
if (!wasHidden) {
2222
// TODO: Move to passive phase
2223
markCommitTimeOfFallback();
2224
}
2225
}
2226
}
2227
+
2228
if (flags & Update) {
2229
try {
2230
commitSuspenseCallback(finishedWork);
packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js
+111
@@ -285,6 +285,117 @@ describe('ReactSuspense', () => {
285
expect(root).toMatchRenderedOutput('AsyncAfter SuspenseSibling');
286
});
287
288
+ it('throttles fallback committing globally', () => {
289
+ function Foo() {
290
+ Scheduler.unstable_yieldValue('Foo');
291
+ return (
292
+ <Suspense fallback={<Text text="Loading..." />}>
293
+ <AsyncText text="A" ms={200} />
294
+ <Suspense fallback={<Text text="Loading more..." />}>
295
+ <AsyncText text="B" ms={300} />
296
+ </Suspense>
297
+ </Suspense>
298
+ );
299
+ }
300
+
301
+ // Committing fallbacks should be throttled.
302
+ // First, advance some time to skip the first threshold.
303
+ jest.advanceTimersByTime(600);
304
+ Scheduler.unstable_advanceTime(600);
305
+
306
+ const root = ReactTestRenderer.create(<Foo />, {
307
+ unstable_isConcurrent: true,
308
+ });
309
+
310
+ expect(Scheduler).toFlushAndYield([
311
+ 'Foo',
312
+ 'Suspend! [A]',
313
+ 'Suspend! [B]',
314
+ 'Loading more...',
315
+ 'Loading...',
316
+ ]);
317
+ expect(root).toMatchRenderedOutput('Loading...');
318
+
319
+ // Resolve A.
320
+ jest.advanceTimersByTime(200);
321
+ Scheduler.unstable_advanceTime(200);
322
+ expect(Scheduler).toHaveYielded(['Promise resolved [A]']);
323
+ expect(Scheduler).toFlushAndYield(['A', 'Suspend! [B]', 'Loading more...']);
324
+
325
+ // By this point, we have enough info to show "A" and "Loading more..."
326
+ // However, we've just shown the outer fallback. So we'll delay
327
+ // showing the inner fallback hoping that B will resolve soon enough.
328
+ expect(root).toMatchRenderedOutput('Loading...');
329
+
330
+ // Resolve B.
331
+ jest.advanceTimersByTime(100);
332
+ Scheduler.unstable_advanceTime(100);
333
+ expect(Scheduler).toHaveYielded(['Promise resolved [B]']);
334
+
335
+ // By this point, B has resolved.
336
+ // We're still showing the outer fallback.
337
+ expect(root).toMatchRenderedOutput('Loading...');
338
+ expect(Scheduler).toFlushAndYield(['A', 'B']);
339
+ // Then contents of both should pop in together.
340
+ expect(root).toMatchRenderedOutput('AB');
341
+ });
342
+
343
+ it('does not throttle fallback committing for too long', () => {
344
+ function Foo() {
345
+ Scheduler.unstable_yieldValue('Foo');
346
+ return (
347
+ <Suspense fallback={<Text text="Loading..." />}>
348
+ <AsyncText text="A" ms={200} />
349
+ <Suspense fallback={<Text text="Loading more..." />}>
350
+ <AsyncText text="B" ms={1200} />
351
+ </Suspense>
352
+ </Suspense>
353
+ );
354
+ }
355
+
356
+ // Committing fallbacks should be throttled.
357
+ // First, advance some time to skip the first threshold.
358
+ jest.advanceTimersByTime(600);
359
+ Scheduler.unstable_advanceTime(600);
360
+
361
+ const root = ReactTestRenderer.create(<Foo />, {
362
+ unstable_isConcurrent: true,
363
+ });
364
+
365
+ expect(Scheduler).toFlushAndYield([
366
+ 'Foo',
367
+ 'Suspend! [A]',
368
+ 'Suspend! [B]',
369
+ 'Loading more...',
370
+ 'Loading...',
371
+ ]);
372
+ expect(root).toMatchRenderedOutput('Loading...');
373
+
374
+ // Resolve A.
375
+ jest.advanceTimersByTime(200);
376
+ Scheduler.unstable_advanceTime(200);
377
+ expect(Scheduler).toHaveYielded(['Promise resolved [A]']);
378
+ expect(Scheduler).toFlushAndYield(['A', 'Suspend! [B]', 'Loading more...']);
379
+
380
+ // By this point, we have enough info to show "A" and "Loading more..."
381
+ // However, we've just shown the outer fallback. So we'll delay
382
+ // showing the inner fallback hoping that B will resolve soon enough.
383
+ expect(root).toMatchRenderedOutput('Loading...');
384
+
385
+ // Wait some more. B is still not resolving.
386
+ jest.advanceTimersByTime(500);
387
+ Scheduler.unstable_advanceTime(500);
388
+ // Give up and render A with a spinner for B.
389
+ expect(root).toMatchRenderedOutput('ALoading more...');
390
+
391
+ // Resolve B.
392
+ jest.advanceTimersByTime(500);
393
+ Scheduler.unstable_advanceTime(500);
394
+ expect(Scheduler).toHaveYielded(['Promise resolved [B]']);
395
+ expect(Scheduler).toFlushAndYield(['B']);
396
+ expect(root).toMatchRenderedOutput('AB');
397
+ });
398
+
399
// @gate !enableSyncDefaultUpdates
400
it(
401
'interrupts current render when something suspends with a ' +