Bugfix: SuspenseList incorrectly forces a fallback (#26453)
Fixes a bug in SuspenseList that @kassens found when deploying React to Meta. In some scenarios, SuspenseList would force the fallback of a deeply nested Suspense boundary into fallback mode, which should never happen under any circumstances — SuspenseList should only affect the nearest descendent Suspense boundaries, without going deeper. The cause was that the internal ForceSuspenseFallback context flag was not being properly reset when it reached the nearest Suspense boundary. It should only be propagated shallowly. We didn't discover this earlier because the scenario where it happens is not that common. To trigger the bug, you need to insert a new Suspense boundary into an already-mounted row of the list. But often when a new Suspense boundary is rendered, it suspends and shows a fallback, anyway, because its content hasn't loaded yet. Another reason we didn't discover this earlier is because there was another bug that was accidentally masking it, which was fixed by #25922. When that fix landed, it revealed this bug. The SuspenseList implementation is complicated but I'm not too concerned with the current messiness. It's an experimental API, and we intend to release it soon, but there are some known flaws and missing features that we need to address first regardless. We'll likely end up rewriting most of it. Co-authored-by: Jan Kassens <jkassens@meta.com>
Andrew Clark committed
Mar 22, 2023 at 13:33 UTC
51a7c45f8799cab903693fcfdd305ce84ba15273
3 files changed
+121
packages/react-reconciler/src/ReactFiberBeginWork.js
+2
@@ -2167,6 +2167,8 @@ function shouldRemainOnFallback(
2167
// If we're already showing a fallback, there are cases where we need to
2168
// remain on that fallback regardless of whether the content has resolved.
2169
// For example, SuspenseList coordinates when nested content appears.
2170
+ // TODO: For compatibility with offscreen prerendering, this should also check
2171
+ // whether the current fiber (if it exists) was visible in the previous tree.
2172
if (current !== null) {
2173
const suspenseState: SuspenseState = current.memoizedState;
2174
if (suspenseState === null) {
packages/react-reconciler/src/ReactFiberSuspenseContext.js
+15
@@ -45,6 +45,14 @@ export function pushPrimaryTreeSuspenseHandler(handler: Fiber): void {
45
const current = handler.alternate;
46
const props: SuspenseProps = handler.pendingProps;
47
48
+ // Shallow Suspense context fields, like ForceSuspenseFallback, should only be
49
+ // propagated a single level. For example, when ForceSuspenseFallback is set,
50
+ // it should only force the nearest Suspense boundary into fallback mode.
51
+ pushSuspenseListContext(
52
+ handler,
53
+ setDefaultShallowSuspenseListContext(suspenseStackCursor.current),
54
+ );
55
+
56
// Experimental feature: Some Suspense boundaries are marked as having an
57
// undesirable fallback state. These have special behavior where we only
58
// activate the fallback if there's no other boundary on the stack that we can
@@ -100,6 +108,11 @@ export function pushFallbackTreeSuspenseHandler(fiber: Fiber): void {
108
109
export function pushOffscreenSuspenseHandler(fiber: Fiber): void {
110
if (fiber.tag === OffscreenComponent) {
111
+ // A SuspenseList context is only pushed here to avoid a push/pop mismatch.
112
+ // Reuse the current value on the stack.
113
+ // TODO: We can avoid needing to push here by by forking popSuspenseHandler
114
+ // into separate functions for Suspense and Offscreen.
115
+ pushSuspenseListContext(fiber, suspenseStackCursor.current);
116
push(suspenseHandlerStackCursor, fiber, fiber);
117
if (shellBoundary !== null) {
118
// A parent boundary is showing a fallback, so we've already rendered
@@ -122,6 +135,7 @@ export function pushOffscreenSuspenseHandler(fiber: Fiber): void {
135
}
136
137
export function reuseSuspenseHandlerOnStack(fiber: Fiber) {
138
+ pushSuspenseListContext(fiber, suspenseStackCursor.current);
139
push(suspenseHandlerStackCursor, getSuspenseHandler(), fiber);
140
}
141
@@ -135,6 +149,7 @@ export function popSuspenseHandler(fiber: Fiber): void {
149
// Popping back into the shell.
150
shellBoundary = null;
151
}
152
+ popSuspenseListContext(fiber);
153
}
154
155
// SuspenseList context
packages/react-reconciler/src/__tests__/ReactSuspenseList-test.js
+104
@@ -3003,4 +3003,108 @@ describe('ReactSuspenseList', () => {
3003
</>,
3004
);
3005
});
3006
+
3007
+ // @gate enableSuspenseList
3008
+ it(
3009
+ 'regression test: SuspenseList should never force boundaries deeper than ' +
3010
+ 'a single level into fallback mode',
3011
+ async () => {
3012
+ const A = createAsyncText('A');
3013
+
3014
+ function UnreachableFallback() {
3015
+ throw new Error('Should never be thrown!');
3016
+ }
3017
+
3018
+ function Repro({update}) {
3019
+ return (
3020
+ <SuspenseList revealOrder="forwards">
3021
+ {update && (
3022
+ <Suspense fallback={<Text text="Loading A..." />}>
3023
+ <A />
3024
+ </Suspense>
3025
+ )}
3026
+ <Suspense fallback={<Text text="Loading B..." />}>
3027
+ {update ? (
3028
+ <Suspense fallback={<UnreachableFallback />}>
3029
+ <Text text="B2" />
3030
+ </Suspense>
3031
+ ) : (
3032
+ <Text text="B1" />
3033
+ )}
3034
+ </Suspense>
3035
+ <Suspense fallback={<Text text="Loading C..." />}>
3036
+ <Text text="C" />
3037
+ </Suspense>
3038
+ {update && (
3039
+ <Suspense fallback={<Text text="Loading D..." />}>
3040
+ <Text text="D" />
3041
+ </Suspense>
3042
+ )}
3043
+ </SuspenseList>
3044
+ );
3045
+ }
3046
+
3047
+ // Initial mount. Only two rows are mounted, B and C.
3048
+ const root = ReactNoop.createRoot();
3049
+ await act(() => root.render(<Repro update={false} />));
3050
+ assertLog(['B1', 'C']);
3051
+ expect(root).toMatchRenderedOutput(
3052
+ <>
3053
+ <span>B1</span>
3054
+ <span>C</span>
3055
+ </>,
3056
+ );
3057
+
3058
+ // During the update, a few things happen simultaneously:
3059
+ // - A new row, A, is inserted into the head. This row suspends.
3060
+ // - The context in row B is replaced. The new content contains a nested
3061
+ // Suspense boundary.
3062
+ // - A new row, D, is inserted into the tail.
3063
+ await act(() => root.render(<Repro update={true} />));
3064
+ assertLog([
3065
+ // During the first pass, the new row, A, suspends. This means any new
3066
+ // rows in the tail should be forced into fallback mode.
3067
+ 'Suspend! [A]',
3068
+ 'Loading A...',
3069
+ 'B2',
3070
+ 'C',
3071
+
3072
+ // A second pass is used to render the fallbacks in the tail.
3073
+ //
3074
+ // Rows B and C were already mounted, so they should not be forced into
3075
+ // fallback mode.
3076
+ //
3077
+ // In the regression that this test was written for, the inner
3078
+ // Suspense boundary around B2 was incorrectly activated. Only the
3079
+ // nearest fallbacks per row should be activated, and only if they
3080
+ // haven't already mounted.
3081
+ 'Loading A...',
3082
+ 'B2',
3083
+ 'C',
3084
+
3085
+ // D is part of the tail, so it should show a fallback.
3086
+ 'Loading D...',
3087
+ ]);
3088
+ expect(root).toMatchRenderedOutput(
3089
+ <>
3090
+ <span>Loading A...</span>
3091
+ <span>B2</span>
3092
+ <span>C</span>
3093
+ <span>Loading D...</span>
3094
+ </>,
3095
+ );
3096
+
3097
+ // Now finish loading A.
3098
+ await act(() => A.resolve());
3099
+ assertLog(['A', 'D']);
3100
+ expect(root).toMatchRenderedOutput(
3101
+ <>
3102
+ <span>A</span>
3103
+ <span>B2</span>
3104
+ <span>C</span>
3105
+ <span>D</span>
3106
+ </>,
3107
+ );
3108
+ },
3109
+ );
3110
});