Regression test: SuspenseList causes lost unmount (#20433)

acdlite · web-flow · commit 3f9205c3331b · 2020-12-11T08:38:49.000-08:00
@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.
diff --git 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']);
+  });
 });
