Bugfix: Effect clean up when deleting suspended tree (#19752)

acdlite · web-flow · commit 4f3f7eeb7f46 · 2020-09-04T15:46:44.000-07:00
* Bug: Effect clean up when deleting suspended tree

Adds a failing unit test.

* Re-use static flags from suspended primary tree

When switching to a Suspense boundary's fallback, we need to be sure
to preserve static subtree flags from the primary tree.
diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js
@@ -64,6 +64,7 @@ import {
   Ref,
   Deletion,
   ForceUpdateForLegacySuspense,
+  StaticMask,
 } from './ReactFiberFlags';
 import ReactSharedInternals from 'shared/ReactSharedInternals';
 import {
@@ -2131,6 +2132,12 @@ function updateSuspenseFallbackChildren(
       currentPrimaryChildFragment,
       primaryChildProps,
     );
+
+    // Since we're reusing a current tree, we need to reuse the flags, too.
+    // (We don't do this in legacy mode, because in legacy mode we don't re-use
+    // the current tree; see previous branch.)
+    primaryChildFragment.subtreeFlags =
+      currentPrimaryChildFragment.subtreeFlags & StaticMask;
   }
   let fallbackChildFragment;
   if (currentFallbackChildFragment !== null) {
diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js
@@ -3956,4 +3956,101 @@ describe('ReactSuspenseWithNoopRenderer', () => {
       </>,
     );
   });
+
+  it('should fire effect clean-up when deleting suspended tree', async () => {
+    const {useEffect} = React;
+
+    function App({show}) {
+      return (
+        <Suspense fallback={<Text text="Loading..." />}>
+          <Child />
+          {show && <AsyncText text="Async" />}
+        </Suspense>
+      );
+    }
+
+    function Child() {
+      useEffect(() => {
+        Scheduler.unstable_yieldValue('Mount Child');
+        return () => {
+          Scheduler.unstable_yieldValue('Unmount Child');
+        };
+      }, []);
+      return <span prop="Child" />;
+    }
+
+    const root = ReactNoop.createRoot();
+
+    await ReactNoop.act(async () => {
+      root.render(<App show={false} />);
+    });
+    expect(Scheduler).toHaveYielded(['Mount Child']);
+    expect(root).toMatchRenderedOutput(<span prop="Child" />);
+
+    await ReactNoop.act(async () => {
+      root.render(<App show={true} />);
+    });
+    // TODO: `act` should have already flushed the placeholder, so this
+    // runAllTimers call should be unnecessary.
+    jest.runAllTimers();
+    expect(Scheduler).toHaveYielded(['Suspend! [Async]', 'Loading...']);
+    expect(root).toMatchRenderedOutput(
+      <>
+        <span hidden={true} prop="Child" />
+        <span prop="Loading..." />
+      </>,
+    );
+
+    await ReactNoop.act(async () => {
+      root.render(null);
+    });
+    expect(Scheduler).toHaveYielded(['Unmount Child']);
+  });
+
+  it('should fire effect clean-up when deleting suspended tree (legacy)', async () => {
+    const {useEffect} = React;
+
+    function App({show}) {
+      return (
+        <Suspense fallback={<Text text="Loading..." />}>
+          <Child />
+          {show && <AsyncText text="Async" />}
+        </Suspense>
+      );
+    }
+
+    function Child() {
+      useEffect(() => {
+        Scheduler.unstable_yieldValue('Mount Child');
+        return () => {
+          Scheduler.unstable_yieldValue('Unmount Child');
+        };
+      }, []);
+      return <span prop="Child" />;
+    }
+
+    const root = ReactNoop.createLegacyRoot();
+
+    await ReactNoop.act(async () => {
+      root.render(<App show={false} />);
+    });
+    expect(Scheduler).toHaveYielded(['Mount Child']);
+    expect(root).toMatchRenderedOutput(<span prop="Child" />);
+
+    await ReactNoop.act(async () => {
+      root.render(<App show={true} />);
+    });
+    expect(Scheduler).toHaveYielded(['Suspend! [Async]', 'Loading...']);
+    expect(root).toMatchRenderedOutput(
+      <>
+        <span hidden={true} prop="Child" />
+        <span prop="Loading..." />
+      </>,
+    );
+
+    await ReactNoop.act(async () => {
+      root.render(null);
+    });
+    expect(Scheduler).toHaveYielded(['Unmount Child']);
+  });
 });
