Skip to content

Commit a1ad17c

Browse files
committed
Regression test: SuspenseList causes lost unmount
@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 facebook#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 facebook#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.
1 parent cdfde3a commit a1ad17c

File tree

1 file changed

+55
-0
lines changed

1 file changed

+55
-0
lines changed

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

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3416,4 +3416,59 @@ describe('ReactHooksWithNoopRenderer', () => {
34163416
'Unmount A',
34173417
]);
34183418
});
3419+
3420+
it('regression: SuspenseList causes unmounts to be dropped on deletion', async () => {
3421+
const {SuspenseList} = React;
3422+
3423+
function Row({label}) {
3424+
useEffect(() => {
3425+
Scheduler.unstable_yieldValue('Mount ' + label);
3426+
return () => {
3427+
Scheduler.unstable_yieldValue('Unmount ' + label);
3428+
};
3429+
}, [label]);
3430+
return (
3431+
<Suspense fallback="Loading...">
3432+
<AsyncText text={label} />
3433+
</Suspense>
3434+
);
3435+
}
3436+
3437+
function App() {
3438+
return (
3439+
<SuspenseList revealOrder="together">
3440+
<Row label="A" />
3441+
<Row label="B" />
3442+
</SuspenseList>
3443+
);
3444+
}
3445+
3446+
const root = ReactNoop.createRoot();
3447+
await ReactNoop.act(async () => {
3448+
root.render(<App />);
3449+
});
3450+
expect(Scheduler).toHaveYielded([
3451+
'Suspend! [A]',
3452+
'Suspend! [B]',
3453+
'Mount A',
3454+
'Mount B',
3455+
]);
3456+
3457+
await ReactNoop.act(async () => {
3458+
await resolveText('A');
3459+
});
3460+
expect(Scheduler).toHaveYielded([
3461+
'Promise resolved [A]',
3462+
'A',
3463+
'Suspend! [B]',
3464+
]);
3465+
3466+
await ReactNoop.act(async () => {
3467+
root.render(null);
3468+
});
3469+
// In the regression, the reorder would cause the children to "forget" that
3470+
// it contains passive effects. Then when we deleted the tree, A's unmount
3471+
// effect would not fire.
3472+
expect(Scheduler).toHaveYielded(['Unmount A', 'Unmount B']);
3473+
});
34193474
});

0 commit comments

Comments
 (0)