From 5f1890f12b4ea900a82853c528c0fe040760857c Mon Sep 17 00:00:00 2001
From: Andrew Clark <git@andrewclark.io>
Date: Mon, 3 Aug 2020 10:57:41 -0500
Subject: [PATCH] Bugfix: Don't unmount siblings of deleted node (#19516)

* Test: Don't unmount siblings of deleted node

Adds a failing regression test. Will fix in the next commit.

* Refactor to accept deleted fiber, not child list

A deleted fiber is passed to
flushPassiveUnmountEffectsInsideOfDeletedTree, but the code is written
as if it accepts the first node of a child list. This is likely because
the function was based on similar functions like
`flushPassiveUnmountEffects`, which do accept a child list.

Unfortunately, types don't help here because we use the first node
in the list to represent the whole list, so in both cases, the type
is Fiber.

Might be worth changing the other functions to also accept individual
fibers instead of a child list, to help avoid confusion.

* Add layout effect to regression test, just in case
---
 .../src/ReactFiberWorkLoop.new.js             | 52 +++++++------------
 .../ReactHooksWithNoopRenderer-test.js        | 52 +++++++++++++++++++
 2 files changed, 71 insertions(+), 33 deletions(-)

diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
index d8dd1c74f3f0..6b362c4173d0 100644
--- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -2790,16 +2790,7 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void {
     if (deletions !== null) {
       for (let i = 0; i < deletions.length; i++) {
         const fiberToDelete = deletions[i];
-        // If this fiber (or anything below it) has passive effects then traverse the subtree.
-        const primaryEffectTag = fiberToDelete.effectTag & PassiveStatic;
-        const primarySubtreeTag =
-          fiberToDelete.subtreeTag & PassiveStaticSubtreeTag;
-        if (
-          primarySubtreeTag !== NoSubtreeTag ||
-          primaryEffectTag !== NoEffect
-        ) {
-          flushPassiveUnmountEffectsInsideOfDeletedTree(fiberToDelete);
-        }
+        flushPassiveUnmountEffectsInsideOfDeletedTree(fiberToDelete);
 
         // Now that passive effects have been processed, it's safe to detach lingering pointers.
         detachFiberAfterEffects(fiberToDelete);
@@ -2835,34 +2826,29 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void {
 }
 
 function flushPassiveUnmountEffectsInsideOfDeletedTree(
-  firstChild: Fiber,
+  fiberToDelete: Fiber,
 ): void {
-  let fiber = firstChild;
-  while (fiber !== null) {
-    const child = fiber.child;
-    if (child !== null) {
-      // If any children have passive effects then traverse the subtree.
-      // Note that this requires checking subtreeTag of the current Fiber,
-      // rather than the subtreeTag/effectsTag of the first child,
-      // since that would not cover passive effects in siblings.
-      const primarySubtreeTag = fiber.subtreeTag & PassiveStaticSubtreeTag;
-      if (primarySubtreeTag !== NoSubtreeTag) {
-        flushPassiveUnmountEffectsInsideOfDeletedTree(child);
-      }
+  if ((fiberToDelete.subtreeTag & PassiveStaticSubtreeTag) !== NoSubtreeTag) {
+    // If any children have passive effects then traverse the subtree.
+    // Note that this requires checking subtreeTag of the current Fiber,
+    // rather than the subtreeTag/effectsTag of the first child,
+    // since that would not cover passive effects in siblings.
+    let child = fiberToDelete.child;
+    while (child !== null) {
+      flushPassiveUnmountEffectsInsideOfDeletedTree(child);
+      child = child.sibling;
     }
+  }
 
-    if ((fiber.effectTag & PassiveStatic) !== NoEffect) {
-      switch (fiber.tag) {
-        case FunctionComponent:
-        case ForwardRef:
-        case SimpleMemoComponent:
-        case Block: {
-          flushPassiveUnmountEffectsImpl(fiber, HookPassive);
-        }
+  if ((fiberToDelete.effectTag & PassiveStatic) !== NoEffect) {
+    switch (fiberToDelete.tag) {
+      case FunctionComponent:
+      case ForwardRef:
+      case SimpleMemoComponent:
+      case Block: {
+        flushPassiveUnmountEffectsImpl(fiberToDelete, HookPassive);
       }
     }
-
-    fiber = fiber.sibling;
   }
 }
 
diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
index 33c710016d84..362bb26b25e3 100644
--- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
+++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
@@ -3314,4 +3314,56 @@ describe('ReactHooksWithNoopRenderer', () => {
     });
     expect(ReactNoop).toMatchRenderedOutput('ABC');
   });
+
+  it("regression test: don't unmount effects on siblings of deleted nodes", async () => {
+    const root = ReactNoop.createRoot();
+
+    function Child({label}) {
+      useLayoutEffect(() => {
+        Scheduler.unstable_yieldValue('Mount layout ' + label);
+        return () => {
+          Scheduler.unstable_yieldValue('Unmount layout ' + label);
+        };
+      }, [label]);
+      useEffect(() => {
+        Scheduler.unstable_yieldValue('Mount passive ' + label);
+        return () => {
+          Scheduler.unstable_yieldValue('Unmount passive ' + label);
+        };
+      }, [label]);
+      return label;
+    }
+
+    await act(async () => {
+      root.render(
+        <>
+          <Child key="A" label="A" />
+          <Child key="B" label="B" />
+        </>,
+      );
+    });
+    expect(Scheduler).toHaveYielded([
+      'Mount layout A',
+      'Mount layout B',
+      'Mount passive A',
+      'Mount passive B',
+    ]);
+
+    // Delete A. This should only unmount the effect on A. In the regression,
+    // B's effect would also unmount.
+    await act(async () => {
+      root.render(
+        <>
+          <Child key="B" label="B" />
+        </>,
+      );
+    });
+    expect(Scheduler).toHaveYielded(['Unmount layout A', 'Unmount passive A']);
+
+    // Now delete and unmount B.
+    await act(async () => {
+      root.render(null);
+    });
+    expect(Scheduler).toHaveYielded(['Unmount layout B', 'Unmount passive B']);
+  });
 });