Bugfix: Don't unmount siblings of deleted node (facebook#19516)

acdlite · web-flow · commit 5f1890f12b4e · 2020-08-03T08:57:41.000-07:00
* 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
diff --git 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
@@ -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']);
+  });
 });
