From 3f9205c3331b80f13a0376826d1ef859786c086b Mon Sep 17 00:00:00 2001
From: Andrew Clark <git@andrewclark.io>
Date: Fri, 11 Dec 2020 10:38:49 -0600
Subject: [PATCH] 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.
---
 .../ReactHooksWithNoopRenderer-test.js        | 56 +++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
index 43ec803f11555..8d006b2f300bc 100644
--- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
+++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
@@ -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']);
+  });
 });