@samitouri / QOS-React-2 / commits / 3f9205c333

Regression test: SuspenseList causes lost unmount (#20433)

@sebmarkbage reminded me that the complete phase of SuspenseList will sometimes enter the begin phase of the children without calling `createWorkInProgress` again, instead calling `resetWorkInProgress`. This was raised in the context of considering whether #20398 might have accidentally caused a SuspenseList bug. (I did look at this code at the time, but considering how complicated SuspenseList is it's not hard to imagine that I made a mistake.) Anyway, I think that PR is fine; however, reviewing it again did lead me to find a different bug. This new bug is actually a variant of the bug fixed by #20398. `resetWorkInProgress` clears a fiber's static flags. That's wrong, since static flags -- like PassiveStatic -- are meant to last the lifetime of the component. In more practical terms, what happens is that if a direct child of SuspenseList contains a `useEffect`, then SuspenseList will cause the child to "forget" that it contains passive effects. When the child is deleted, its unmount effects are not fired :O This is the second of this type of bug I've found, which indicates to me that it's too easy to accidentally clear static flags. Maybe we should only update the `flags` field using helper functions, like we do with `lanes`. Or perhaps we add an internal warning somewhere that detects when a fiber has different static flags than its alternate.

Andrew Clark committed Dec 11, 2020 at 10:38 UTC 3f9205c3331b80f13a0376826d1ef859786c086b

1 file changed +56

packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js

+56

@@ -3416,4 +3416,60 @@ describe('ReactHooksWithNoopRenderer', () => {

3416 'Unmount A',

3417 ]);

3418 });

3419 +

3420 + // @gate experimental

3421 + it('regression: SuspenseList causes unmounts to be dropped on deletion', async () => {

3422 + const SuspenseList = React.unstable_SuspenseList;

3423 +

3424 + function Row({label}) {

3425 + useEffect(() => {

3426 + Scheduler.unstable_yieldValue('Mount ' + label);

3427 + return () => {

3428 + Scheduler.unstable_yieldValue('Unmount ' + label);

3429 + };

3430 + }, [label]);

3431 + return (

3432 + <Suspense fallback="Loading...">

3433 + <AsyncText text={label} />

3434 + </Suspense>

3435 + );

3436 + }

3437 +

3438 + function App() {

3439 + return (

3440 + <SuspenseList revealOrder="together">

3441 + <Row label="A" />

3442 + <Row label="B" />

3443 + </SuspenseList>

3444 + );

3445 + }

3446 +

3447 + const root = ReactNoop.createRoot();

3448 + await ReactNoop.act(async () => {

3449 + root.render(<App />);

3450 + });

3451 + expect(Scheduler).toHaveYielded([

3452 + 'Suspend! [A]',

3453 + 'Suspend! [B]',

3454 + 'Mount A',

3455 + 'Mount B',

3456 + ]);

3457 +

3458 + await ReactNoop.act(async () => {

3459 + await resolveText('A');

3460 + });

3461 + expect(Scheduler).toHaveYielded([

3462 + 'Promise resolved [A]',

3463 + 'A',

3464 + 'Suspend! [B]',

3465 + ]);

3466 +

3467 + await ReactNoop.act(async () => {

3468 + root.render(null);

3469 + });

3470 + // In the regression, SuspenseList would cause the children to "forget" that

3471 + // it contains passive effects. Then when we deleted the tree, these unmount

3472 + // effects would not fire.

3473 + expect(Scheduler).toHaveYielded(['Unmount A', 'Unmount B']);

3474 + });

3475 });