Skip to content

Commit 54ddad8

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 54ddad8

File tree

1 file changed

+56
-0
lines changed

1 file changed

+56
-0
lines changed

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

+56
Original file line numberDiff line numberDiff line change
@@ -3416,4 +3416,60 @@ describe('ReactHooksWithNoopRenderer', () => {
34163416
'Unmount A',
34173417
]);
34183418
});
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+
});
34193475
});

0 commit comments

Comments
 (0)