From fab3639ca9ade3cf748002e59e5b4694ecbabd3a Mon Sep 17 00:00:00 2001
From: Andrew Clark <git@andrewclark.io>
Date: Mon, 31 Aug 2020 12:38:52 -0500
Subject: [PATCH 1/2] Bug: Effect clean up when deleting suspended tree

Adds a failing unit test.
---
 .../ReactSuspenseWithNoopRenderer-test.js     | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js
index c10772eaa16e5..48d25ecd9aa5f 100644
--- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js
+++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js
@@ -3956,4 +3956,53 @@ 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 flushed the placeholder
+    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']);
+  });
 });

From 30741ae583c3bbbffb24808ad5d304ed51e80bb3 Mon Sep 17 00:00:00 2001
From: Andrew Clark <git@andrewclark.io>
Date: Fri, 4 Sep 2020 17:34:12 -0500
Subject: [PATCH 2/2] 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.
---
 .../src/ReactFiberBeginWork.new.js            |  7 +++
 .../ReactSuspenseWithNoopRenderer-test.js     | 50 ++++++++++++++++++-
 2 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js
index 2d3841d2e0e43..fa27d3dcad74e 100644
--- 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
index 48d25ecd9aa5f..b6ed0d2be4d40 100644
--- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js
+++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js
@@ -3990,7 +3990,8 @@ describe('ReactSuspenseWithNoopRenderer', () => {
     await ReactNoop.act(async () => {
       root.render(<App show={true} />);
     });
-    // TODO: `act` should have flushed the placeholder
+    // 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(
@@ -4005,4 +4006,51 @@ describe('ReactSuspenseWithNoopRenderer', () => {
     });
     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']);
+  });
 });