Fix: Don't read primaryChild.childExpirationTime (#18457)
This is a variant of the fix in 5a0f1d. We can't rely on the primary fiber's `childExpirationTime` field to be correct. In this case, we can read from the Suspense boundary fiber instead. This will include updates that exist in the fallback fiber, but that's not a big deal; the important thing is that we don't drop updates.
Andrew Clark committed
Apr 1, 2020 at 14:36 UTC
58afba06636300fd652924e3033916fc96142e13
2 files changed
+85
-11
packages/react-reconciler/src/ReactFiberBeginWork.js
+1
-11
@@ -1646,16 +1646,9 @@ function shouldRemainOnFallback(
1646
function getRemainingWorkInPrimaryTree(
1647
current: Fiber,
1648
workInProgress: Fiber,
1649
- currentPrimaryChildFragment: Fiber | null,
1649
renderExpirationTime,
1650
) {
1652
- const currentParentOfPrimaryChildren =
1653
- currentPrimaryChildFragment !== null
1654
- ? currentPrimaryChildFragment
1655
- : current;
1656
- const currentChildExpirationTime =
1657
- currentParentOfPrimaryChildren.childExpirationTime;
1658
-
1651
+ const currentChildExpirationTime = current.childExpirationTime;
1652
const currentSuspenseState: SuspenseState = current.memoizedState;
1653
if (currentSuspenseState !== null) {
1654
// This boundary already timed out. Check if this render includes the level
@@ -1945,7 +1938,6 @@ function updateSuspenseComponent(
1938
primaryChildFragment.childExpirationTime = getRemainingWorkInPrimaryTree(
1939
current,
1940
workInProgress,
1948
- null,
1941
renderExpirationTime,
1942
);
1943
workInProgress.memoizedState = updateSuspenseState(
@@ -2016,7 +2008,6 @@ function updateSuspenseComponent(
2008
primaryChildFragment.childExpirationTime = getRemainingWorkInPrimaryTree(
2009
current,
2010
workInProgress,
2019
- currentPrimaryChildFragment,
2011
renderExpirationTime,
2012
);
2013
// Skip the primary children, and continue working on the
@@ -2118,7 +2109,6 @@ function updateSuspenseComponent(
2109
primaryChildFragment.childExpirationTime = getRemainingWorkInPrimaryTree(
2110
current,
2111
workInProgress,
2121
- null,
2112
renderExpirationTime,
2113
);
2114
// Skip the primary children, and continue working on the
packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js
+84
@@ -3266,6 +3266,90 @@ describe('ReactSuspenseWithNoopRenderer', () => {
3266
},
3267
);
3268
3269
+ it(
3270
+ 'regression: primary fragment fiber is not always part of setState ' +
3271
+ 'return path (another case)',
3272
+ async () => {
3273
+ // Reproduces a bug where updates inside a suspended tree are dropped
3274
+ // because the fragment fiber we insert to wrap the hidden children is not
3275
+ // part of the return path, so it doesn't get marked during setState.
3276
+ const {useState} = React;
3277
+ const root = ReactNoop.createRoot();
3278
+
3279
+ function Parent() {
3280
+ return (
3281
+ <Suspense fallback={<Text text="Loading..." />}>
3282
+ <Child />
3283
+ </Suspense>
3284
+ );
3285
+ }
3286
+
3287
+ let setText;
3288
+ function Child() {
3289
+ const [text, _setText] = useState('A');
3290
+ setText = _setText;
3291
+ return <AsyncText text={text} />;
3292
+ }
3293
+
3294
+ // Mount an initial tree. Resolve A so that it doesn't suspend.
3295
+ await resolveText('A');
3296
+ await ReactNoop.act(async () => {
3297
+ root.render(<Parent />);
3298
+ });
3299
+ expect(Scheduler).toHaveYielded(['A']);
3300
+ // At this point, the setState return path follows current fiber.
3301
+ expect(root).toMatchRenderedOutput(<span prop="A" />);
3302
+
3303
+ // Schedule another update. This will "flip" the alternate pairs.
3304
+ await resolveText('B');
3305
+ await ReactNoop.act(async () => {
3306
+ setText('B');
3307
+ });
3308
+ expect(Scheduler).toHaveYielded(['B']);
3309
+ // Now the setState return path follows the *alternate* fiber.
3310
+ expect(root).toMatchRenderedOutput(<span prop="B" />);
3311
+
3312
+ // Schedule another update. This time, we'll suspend.
3313
+ await ReactNoop.act(async () => {
3314
+ setText('C');
3315
+ });
3316
+ expect(Scheduler).toHaveYielded(['Suspend! [C]', 'Loading...']);
3317
+
3318
+ // Commit. This will insert a fragment fiber to wrap around the component
3319
+ // that triggered the update.
3320
+ await ReactNoop.act(async () => {
3321
+ await advanceTimers(250);
3322
+ });
3323
+ // The fragment fiber is part of the current tree, but the setState return
3324
+ // path still follows the alternate path. That means the fragment fiber is
3325
+ // not part of the return path.
3326
+ expect(root).toMatchRenderedOutput(
3327
+ <>
3328
+ <span hidden={true} prop="B" />
3329
+ <span prop="Loading..." />
3330
+ </>,
3331
+ );
3332
+
3333
+ await ReactNoop.act(async () => {
3334
+ // Schedule a normal pri update. This will suspend again.
3335
+ setText('D');
3336
+
3337
+ // And another update at lower priority. This will unblock.
3338
+ await resolveText('E');
3339
+ Scheduler.unstable_runWithPriority(
3340
+ Scheduler.unstable_IdlePriority,
3341
+ () => {
3342
+ setText('E');
3343
+ },
3344
+ );
3345
+ });
3346
+ // Even though the fragment fiber is not part of the return path, we should
3347
+ // be able to finish rendering.
3348
+ expect(Scheduler).toHaveYielded(['Suspend! [D]', 'E']);
3349
+ expect(root).toMatchRenderedOutput(<span prop="E" />);
3350
+ },
3351
+ );
3352
+
3353
it(
3354
'after showing fallback, should not flip back to primary content until ' +
3355
'the update that suspended finishes',