Bugfix: In legacy mode, call suspended tree's unmount effects when it…

… is deleted (#24400) * Bug: Missing unmount when suspended tree deleted When a suspended tree switches to a fallback, we unmount the effects. If the suspended tree is then deleted, there's a guard to prevent us from unmounting the effects again. However, in legacy mode, we don't unmount effects when a tree suspends. So if the suspended tree is then deleted, we do need to unmount the effects. We're missing a check for legacy/concurrent mode. * Fix: Unmount suspended tree when it is deleted
facebook · Apr 19, 2022 · ab9cdd3 · ab9cdd3
1 parent 2bf5eba
commit ab9cdd3
Show file tree

Hide file tree

Showing 3 changed files with 139 additions and 104 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
@@ -1804,24 +1804,36 @@ function commitDeletionEffectsOnFiber(
       return;
     }
     case OffscreenComponent: {
-      // If this offscreen component is hidden, we already unmounted it. Before
-      // deleting the children, track that it's already unmounted so that we
-      // don't attempt to unmount the effects again.
-      // TODO: If the tree is hidden, in most cases we should be able to skip
-      // over the nested children entirely. An exception is we haven't yet found
-      // the topmost host node to delete, which we already track on the stack.
-      // But the other case is portals, which need to be detached no matter how
-      // deeply they are nested. We should use a subtree flag to track whether a
-      // subtree includes a nested portal.
-      const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
-      offscreenSubtreeWasHidden =
-        prevOffscreenSubtreeWasHidden || deletedFiber.memoizedState !== null;
-      recursivelyTraverseDeletionEffects(
-        finishedRoot,
-        nearestMountedAncestor,
-        deletedFiber,
-      );
-      offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
+      if (
+        // TODO: Remove this dead flag
+        enableSuspenseLayoutEffectSemantics &&
+        deletedFiber.mode & ConcurrentMode
+      ) {
+        // If this offscreen component is hidden, we already unmounted it. Before
+        // deleting the children, track that it's already unmounted so that we
+        // don't attempt to unmount the effects again.
+        // TODO: If the tree is hidden, in most cases we should be able to skip
+        // over the nested children entirely. An exception is we haven't yet found
+        // the topmost host node to delete, which we already track on the stack.
+        // But the other case is portals, which need to be detached no matter how
+        // deeply they are nested. We should use a subtree flag to track whether a
+        // subtree includes a nested portal.
+        const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
+        offscreenSubtreeWasHidden =
+          prevOffscreenSubtreeWasHidden || deletedFiber.memoizedState !== null;
+        recursivelyTraverseDeletionEffects(
+          finishedRoot,
+          nearestMountedAncestor,
+          deletedFiber,
+        );
+        offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
+      } else {
+        recursivelyTraverseDeletionEffects(
+          finishedRoot,
+          nearestMountedAncestor,
+          deletedFiber,
+        );
+      }
       break;
     }
     default: {
@@ -2238,13 +2250,21 @@ function commitMutationEffectsOnFiber(
     case OffscreenComponent: {
       const wasHidden = current !== null && current.memoizedState !== null;
 
-      // Before committing the children, track on the stack whether this
-      // offscreen subtree was already hidden, so that we don't unmount the
-      // effects again.
-      const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
-      offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden;
-      recursivelyTraverseMutationEffects(root, finishedWork, lanes);
-      offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
+      if (
+        // TODO: Remove this dead flag
+        enableSuspenseLayoutEffectSemantics &&
+        finishedWork.mode & ConcurrentMode
+      ) {
+        // Before committing the children, track on the stack whether this
+        // offscreen subtree was already hidden, so that we don't unmount the
+        // effects again.
+        const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
+        offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden;
+        recursivelyTraverseMutationEffects(root, finishedWork, lanes);
+        offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
+      } else {
+        recursivelyTraverseMutationEffects(root, finishedWork, lanes);
+      }
 
       commitReconciliationEffects(finishedWork);
 

diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js
@@ -1804,24 +1804,36 @@ function commitDeletionEffectsOnFiber(
       return;
     }
     case OffscreenComponent: {
-      // If this offscreen component is hidden, we already unmounted it. Before
-      // deleting the children, track that it's already unmounted so that we
-      // don't attempt to unmount the effects again.
-      // TODO: If the tree is hidden, in most cases we should be able to skip
-      // over the nested children entirely. An exception is we haven't yet found
-      // the topmost host node to delete, which we already track on the stack.
-      // But the other case is portals, which need to be detached no matter how
-      // deeply they are nested. We should use a subtree flag to track whether a
-      // subtree includes a nested portal.
-      const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
-      offscreenSubtreeWasHidden =
-        prevOffscreenSubtreeWasHidden || deletedFiber.memoizedState !== null;
-      recursivelyTraverseDeletionEffects(
-        finishedRoot,
-        nearestMountedAncestor,
-        deletedFiber,
-      );
-      offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
+      if (
+        // TODO: Remove this dead flag
+        enableSuspenseLayoutEffectSemantics &&
+        deletedFiber.mode & ConcurrentMode
+      ) {
+        // If this offscreen component is hidden, we already unmounted it. Before
+        // deleting the children, track that it's already unmounted so that we
+        // don't attempt to unmount the effects again.
+        // TODO: If the tree is hidden, in most cases we should be able to skip
+        // over the nested children entirely. An exception is we haven't yet found
+        // the topmost host node to delete, which we already track on the stack.
+        // But the other case is portals, which need to be detached no matter how
+        // deeply they are nested. We should use a subtree flag to track whether a
+        // subtree includes a nested portal.
+        const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
+        offscreenSubtreeWasHidden =
+          prevOffscreenSubtreeWasHidden || deletedFiber.memoizedState !== null;
+        recursivelyTraverseDeletionEffects(
+          finishedRoot,
+          nearestMountedAncestor,
+          deletedFiber,
+        );
+        offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
+      } else {
+        recursivelyTraverseDeletionEffects(
+          finishedRoot,
+          nearestMountedAncestor,
+          deletedFiber,
+        );
+      }
       break;
     }
     default: {
@@ -2238,13 +2250,21 @@ function commitMutationEffectsOnFiber(
     case OffscreenComponent: {
       const wasHidden = current !== null && current.memoizedState !== null;
 
-      // Before committing the children, track on the stack whether this
-      // offscreen subtree was already hidden, so that we don't unmount the
-      // effects again.
-      const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
-      offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden;
-      recursivelyTraverseMutationEffects(root, finishedWork, lanes);
-      offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
+      if (
+        // TODO: Remove this dead flag
+        enableSuspenseLayoutEffectSemantics &&
+        finishedWork.mode & ConcurrentMode
+      ) {
+        // Before committing the children, track on the stack whether this
+        // offscreen subtree was already hidden, so that we don't unmount the
+        // effects again.
+        const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
+        offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden;
+        recursivelyTraverseMutationEffects(root, finishedWork, lanes);
+        offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
+      } else {
+        recursivelyTraverseMutationEffects(root, finishedWork, lanes);
+      }
 
       commitReconciliationEffects(finishedWork);
 

diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js
@@ -93,60 +93,6 @@ describe('ReactSuspenseEffectsSemanticsDOM', () => {
     });
   });
 
-  it('does not destroy layout effects twice when hidden child is removed', async () => {
-    function ChildA({label}) {
-      React.useLayoutEffect(() => {
-        Scheduler.unstable_yieldValue('Did mount: ' + label);
-        return () => {
-          Scheduler.unstable_yieldValue('Will unmount: ' + label);
-        };
-      }, []);
-      return <Text text={label} />;
-    }
-
-    function ChildB({label}) {
-      React.useLayoutEffect(() => {
-        Scheduler.unstable_yieldValue('Did mount: ' + label);
-        return () => {
-          Scheduler.unstable_yieldValue('Will unmount: ' + label);
-        };
-      }, []);
-      return <Text text={label} />;
-    }
-
-    const LazyChildA = React.lazy(() => fakeImport(ChildA));
-    const LazyChildB = React.lazy(() => fakeImport(ChildB));
-
-    function Parent({swap}) {
-      return (
-        <React.Suspense fallback={<Text text="Loading..." />}>
-          {swap ? <LazyChildB label="B" /> : <LazyChildA label="A" />}
-        </React.Suspense>
-      );
-    }
-
-    const root = ReactDOMClient.createRoot(container);
-    act(() => {
-      root.render(<Parent swap={false} />);
-    });
-    expect(Scheduler).toHaveYielded(['Loading...']);
-
-    await LazyChildA;
-    expect(Scheduler).toFlushAndYield(['A', 'Did mount: A']);
-    expect(container.innerHTML).toBe('A');
-
-    // Swap the position of A and B
-    ReactDOM.flushSync(() => {
-      root.render(<Parent swap={true} />);
-    });
-    expect(Scheduler).toHaveYielded(['Loading...', 'Will unmount: A']);
-    expect(container.innerHTML).toBe('Loading...');
-
-    await LazyChildB;
-    expect(Scheduler).toFlushAndYield(['B', 'Did mount: B']);
-    expect(container.innerHTML).toBe('B');
-  });
-
   it('does not destroy ref cleanup twice when hidden child is removed', async () => {
     function ChildA({label}) {
       return (
@@ -455,4 +401,53 @@ describe('ReactSuspenseEffectsSemanticsDOM', () => {
     expect(Scheduler).toFlushAndYield([]);
     expect(container.innerHTML).toBe('<h1>Hello</h1>');
   });
+
+  it('regression: unmount hidden tree, in legacy mode', async () => {
+    // In legacy mode, when a tree suspends and switches to a fallback, the
+    // effects are not unmounted. So we have to unmount them during a deletion.
+
+    function Child() {
+      React.useLayoutEffect(() => {
+        Scheduler.unstable_yieldValue('Mount');
+        return () => {
+          Scheduler.unstable_yieldValue('Unmount');
+        };
+      }, []);
+      return <Text text="Child" />;
+    }
+
+    function Sibling() {
+      return <Text text="Sibling" />;
+    }
+    const LazySibling = React.lazy(() => fakeImport(Sibling));
+
+    function App({showMore}) {
+      return (
+        <React.Suspense fallback={<Text text="Loading..." />}>
+          <Child />
+          {showMore ? <LazySibling /> : null}
+        </React.Suspense>
+      );
+    }
+
+    // Initial render
+    ReactDOM.render(<App showMore={false} />, container);
+    expect(Scheduler).toHaveYielded(['Child', 'Mount']);
+
+    // Update that suspends, causing the existing tree to switches it to
+    // a fallback.
+    ReactDOM.render(<App showMore={true} />, container);
+    expect(Scheduler).toHaveYielded([
+      'Child',
+      'Loading...',
+
+      // In a concurrent root, the effect would unmount here. But this is legacy
+      // mode, so it doesn't.
+      // Unmount
+    ]);
+
+    // Delete the tree and unmount the effect
+    ReactDOM.render(null, container);
+    expect(Scheduler).toHaveYielded(['Unmount']);
+  });
 });