Fix: Stylesheet in error UI suspends indefinitely (#27265)
This fixes the regression test added in the previous commit. The "Suspensey commit" implementation relies on the `shouldRemainOnPreviousScreen` function to determine whether to 1) suspend the commit 2) activate a parent fallback and schedule a retry. The issue was that we were sometimes attempting option 2 even when there was no parent fallback. Part of the reason this bug landed is due to how `throwException` is structured. In the case of Suspensey commits, we pass a special "noop" thenable to `throwException` as a way to trigger the Suspense path. This special thenable must never have a listener attached to it. This is not a great way to structure the logic, it's just a consequence of how the code evolved over time. We should refactor it into multiple functions so we can trigger a fallback directly without having to check the type. In the meantime, I added an internal warning to help detect similar mistakes in the future.
Andrew Clark committed
Aug 22, 2023 at 11:22 UTC
dd480ef923930c8906a02664b01bcdea50707b5d
4 files changed
+51
-40
packages/react-dom/src/__tests__/ReactDOMFloat-test.js
-1
@@ -3385,7 +3385,6 @@ body {
3385
);
3386
});
3387
3388
- // @gate FIXME
3388
it('loading a stylesheet as part of an error boundary UI, during initial render', async () => {
3389
class ErrorBoundary extends React.Component {
3390
state = {error: null};
packages/react-reconciler/src/ReactFiberThenable.js
+10
-1
@@ -42,7 +42,16 @@ export const SuspenseyCommitException: mixed = new Error(
42
// TODO: It would be better to refactor throwException into multiple functions
43
// so we can trigger a fallback directly without having to check the type. But
44
// for now this will do.
45
-export const noopSuspenseyCommitThenable = {then() {}};
45
+export const noopSuspenseyCommitThenable = {
46
+ then() {
47
+ if (__DEV__) {
48
+ console.error(
49
+ 'Internal React error: A listener was unexpectedly attached to a ' +
50
+ '"noop" thenable. This is a bug in React. Please file an issue.',
51
+ );
52
+ }
53
+ },
54
+};
55
56
export function createThenableState(): ThenableState {
57
// The ThenableState is created the first time a component suspends. If it
packages/react-reconciler/src/ReactFiberThrow.js
+15
-15
@@ -438,8 +438,15 @@ function throwException(
438
} else {
439
retryQueue.add(wakeable);
440
}
441
+
442
+ // We only attach ping listeners in concurrent mode. Legacy
443
+ // Suspense always commits fallbacks synchronously, so there are
444
+ // no pings.
445
+ if (suspenseBoundary.mode & ConcurrentMode) {
446
+ attachPingListener(root, wakeable, rootRenderLanes);
447
+ }
448
}
442
- break;
449
+ return;
450
}
451
case OffscreenComponent: {
452
if (suspenseBoundary.mode & ConcurrentMode) {
@@ -466,24 +473,17 @@ function throwException(
473
retryQueue.add(wakeable);
474
}
475
}
476
+
477
+ attachPingListener(root, wakeable, rootRenderLanes);
478
}
470
- break;
479
+ return;
480
}
472
- // Fall through
473
- }
474
- default: {
475
- throw new Error(
476
- `Unexpected Suspense handler tag (${suspenseBoundary.tag}). This ` +
477
- 'is a bug in React.',
478
- );
481
}
482
}
481
- // We only attach ping listeners in concurrent mode. Legacy Suspense always
482
- // commits fallbacks synchronously, so there are no pings.
483
- if (suspenseBoundary.mode & ConcurrentMode) {
484
- attachPingListener(root, wakeable, rootRenderLanes);
485
- }
486
- return;
483
+ throw new Error(
484
+ `Unexpected Suspense handler tag (${suspenseBoundary.tag}). This ` +
485
+ 'is a bug in React.',
486
+ );
487
} else {
488
// No boundary was found. Unless this is a sync update, this is OK.
489
// We can suspend and wait for more data to arrive.
packages/react-reconciler/src/ReactFiberWorkLoop.js
+26
-23
@@ -1687,6 +1687,16 @@ export function shouldRemainOnPreviousScreen(): boolean {
1687
// takes into account both the priority of render and also whether showing a
1688
// fallback would produce a desirable user experience.
1689
1690
+ const handler = getSuspenseHandler();
1691
+ if (handler === null) {
1692
+ // There's no Suspense boundary that can provide a fallback. We have no
1693
+ // choice but to remain on the previous screen.
1694
+ // NOTE: We do this even for sync updates, for lack of any better option. In
1695
+ // the future, we may change how we handle this, like by putting the whole
1696
+ // root into a "detached" mode.
1697
+ return true;
1698
+ }
1699
+
1700
// TODO: Once `use` has fully replaced the `throw promise` pattern, we should
1701
// be able to remove the equivalent check in finishConcurrentRender, and rely
1702
// just on this one.
@@ -1705,29 +1715,22 @@ export function shouldRemainOnPreviousScreen(): boolean {
1715
}
1716
}
1717
1708
- const handler = getSuspenseHandler();
1709
- if (handler === null) {
1710
- // TODO: We should support suspending in the case where there's no
1711
- // parent Suspense boundary, even outside a transition. Somehow. Otherwise,
1712
- // an uncached promise can fall into an infinite loop.
1713
- } else {
1714
- if (
1715
- includesOnlyRetries(workInProgressRootRenderLanes) ||
1716
- // In this context, an OffscreenLane counts as a Retry
1717
- // TODO: It's become increasingly clear that Retries and Offscreen are
1718
- // deeply connected. They probably can be unified further.
1719
- includesSomeLane(workInProgressRootRenderLanes, OffscreenLane)
1720
- ) {
1721
- // During a retry, we can suspend rendering if the nearest Suspense boundary
1722
- // is the boundary of the "shell", because we're guaranteed not to block
1723
- // any new content from appearing.
1724
- //
1725
- // The reason we must check if this is a retry is because it guarantees
1726
- // that suspending the work loop won't block an actual update, because
1727
- // retries don't "update" anything; they fill in fallbacks that were left
1728
- // behind by a previous transition.
1729
- return handler === getShellBoundary();
1730
- }
1718
+ if (
1719
+ includesOnlyRetries(workInProgressRootRenderLanes) ||
1720
+ // In this context, an OffscreenLane counts as a Retry
1721
+ // TODO: It's become increasingly clear that Retries and Offscreen are
1722
+ // deeply connected. They probably can be unified further.
1723
+ includesSomeLane(workInProgressRootRenderLanes, OffscreenLane)
1724
+ ) {
1725
+ // During a retry, we can suspend rendering if the nearest Suspense boundary
1726
+ // is the boundary of the "shell", because we're guaranteed not to block
1727
+ // any new content from appearing.
1728
+ //
1729
+ // The reason we must check if this is a retry is because it guarantees
1730
+ // that suspending the work loop won't block an actual update, because
1731
+ // retries don't "update" anything; they fill in fallbacks that were left
1732
+ // behind by a previous transition.
1733
+ return handler === getShellBoundary();
1734
}
1735
1736
// For all other Lanes besides Transitions and Retries, we should not wait