Skip to content

Commit

Permalink
Regression test: SuspenseList causes lost unmount (#20433)
Browse files Browse the repository at this point in the history
@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.
  • Loading branch information
acdlite authored Dec 11, 2020
1 parent cdfde3a commit 3f9205c
Showing 1 changed file with 56 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -3416,4 +3416,60 @@ describe('ReactHooksWithNoopRenderer', () => {
'Unmount A',
]);
});

// @gate experimental
it('regression: SuspenseList causes unmounts to be dropped on deletion', async () => {
const SuspenseList = React.unstable_SuspenseList;

function Row({label}) {
useEffect(() => {
Scheduler.unstable_yieldValue('Mount ' + label);
return () => {
Scheduler.unstable_yieldValue('Unmount ' + label);
};
}, [label]);
return (
<Suspense fallback="Loading...">
<AsyncText text={label} />
</Suspense>
);
}

function App() {
return (
<SuspenseList revealOrder="together">
<Row label="A" />
<Row label="B" />
</SuspenseList>
);
}

const root = ReactNoop.createRoot();
await ReactNoop.act(async () => {
root.render(<App />);
});
expect(Scheduler).toHaveYielded([
'Suspend! [A]',
'Suspend! [B]',
'Mount A',
'Mount B',
]);

await ReactNoop.act(async () => {
await resolveText('A');
});
expect(Scheduler).toHaveYielded([
'Promise resolved [A]',
'A',
'Suspend! [B]',
]);

await ReactNoop.act(async () => {
root.render(null);
});
// In the regression, SuspenseList would cause the children to "forget" that
// it contains passive effects. Then when we deleted the tree, these unmount
// effects would not fire.
expect(Scheduler).toHaveYielded(['Unmount A', 'Unmount B']);
});
});

1 comment on commit 3f9205c

@steida
Copy link

@steida steida commented on 3f9205c Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't possible to prevent such bugs with the type system, I mean newtype (branded primitives in TS) or TaskEither (A promise which uses Either instead of an untyped throw). Both is possible with Flow as far I know.

Please sign in to comment.