Double invoked effects on suspended children (#25307)

Co-authored-by: Robert Balicki <robertbalicki@fb.com>
facebook · Sep 21, 2022 · 0cac4d5 · 0cac4d5
1 parent b274890
commit 0cac4d5
Show file tree

Hide file tree

Showing 3 changed files with 165 additions and 30 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -3211,35 +3211,66 @@ function recursivelyTraverseAndDoubleInvokeEffectsInDEV(
   }
   let child = parentFiber.child;
   while (child !== null) {
-    doubleInvokeEffectsInDEV(root, child, isInStrictMode);
+    doubleInvokeEffectsInDEVIfNecessary(root, child, isInStrictMode);
     child = child.sibling;
   }
 }
 
-function doubleInvokeEffectsInDEV(
+// Unconditionally disconnects and connects passive and layout effects.
+function doubleInvokeEffectsOnFiber(root: FiberRoot, fiber: Fiber) {
+  disappearLayoutEffects(fiber);
+  disconnectPassiveEffect(fiber);
+  reappearLayoutEffects(root, fiber.alternate, fiber, false);
+  reconnectPassiveEffects(root, fiber, NoLanes, null, false);
+}
+
+function doubleInvokeEffectsInDEVIfNecessary(
   root: FiberRoot,
   fiber: Fiber,
   parentIsInStrictMode: boolean,
 ) {
   const isStrictModeFiber = fiber.type === REACT_STRICT_MODE_TYPE;
   const isInStrictMode = parentIsInStrictMode || isStrictModeFiber;
 
-  if (fiber.flags & PlacementDEV || fiber.tag === OffscreenComponent) {
+  // First case: the fiber **is not** of type OffscreenComponent. No
+  // special rules apply to double invoking effects.
+  if (fiber.tag !== OffscreenComponent) {
+    if (fiber.flags & PlacementDEV) {
+      setCurrentDebugFiberInDEV(fiber);
+      if (isInStrictMode) {
+        doubleInvokeEffectsOnFiber(root, fiber);
+      }
+      resetCurrentDebugFiberInDEV();
+    } else {
+      recursivelyTraverseAndDoubleInvokeEffectsInDEV(
+        root,
+        fiber,
+        isInStrictMode,
+      );
+    }
+    return;
+  }
+
+  // Second case: the fiber **is** of type OffscreenComponent.
+  // This branch contains cases specific to Offscreen.
+  if (fiber.memoizedState === null) {
+    // Only consider Offscreen that is visible.
+    // TODO (Offscreen) Handle manual mode.
     setCurrentDebugFiberInDEV(fiber);
-    const isNotOffscreen = fiber.tag !== OffscreenComponent;
-    // Checks if Offscreen is being revealed. For all other components, evaluates to true.
-    const hasOffscreenBecomeVisible =
-      isNotOffscreen ||
-      (fiber.flags & Visibility && fiber.memoizedState === null);
-    if (isInStrictMode && hasOffscreenBecomeVisible) {
-      disappearLayoutEffects(fiber);
-      disconnectPassiveEffect(fiber);
-      reappearLayoutEffects(root, fiber.alternate, fiber, false);
-      reconnectPassiveEffects(root, fiber, NoLanes, null, false);
+    if (isInStrictMode && fiber.flags & Visibility) {
+      // Double invoke effects on Offscreen's subtree only
+      // if it is visible and its visibility has changed.
+      doubleInvokeEffectsOnFiber(root, fiber);
+    } else if (fiber.subtreeFlags & PlacementDEV) {
+      // Something in the subtree could have been suspended.
+      // We need to continue traversal and find newly inserted fibers.
+      recursivelyTraverseAndDoubleInvokeEffectsInDEV(
+        root,
+        fiber,
+        isInStrictMode,
+      );
     }
     resetCurrentDebugFiberInDEV();
-  } else {
-    recursivelyTraverseAndDoubleInvokeEffectsInDEV(root, fiber, isInStrictMode);
   }
 }
 

diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
@@ -3211,35 +3211,66 @@ function recursivelyTraverseAndDoubleInvokeEffectsInDEV(
   }
   let child = parentFiber.child;
   while (child !== null) {
-    doubleInvokeEffectsInDEV(root, child, isInStrictMode);
+    doubleInvokeEffectsInDEVIfNecessary(root, child, isInStrictMode);
     child = child.sibling;
   }
 }
 
-function doubleInvokeEffectsInDEV(
+// Unconditionally disconnects and connects passive and layout effects.
+function doubleInvokeEffectsOnFiber(root: FiberRoot, fiber: Fiber) {
+  disappearLayoutEffects(fiber);
+  disconnectPassiveEffect(fiber);
+  reappearLayoutEffects(root, fiber.alternate, fiber, false);
+  reconnectPassiveEffects(root, fiber, NoLanes, null, false);
+}
+
+function doubleInvokeEffectsInDEVIfNecessary(
   root: FiberRoot,
   fiber: Fiber,
   parentIsInStrictMode: boolean,
 ) {
   const isStrictModeFiber = fiber.type === REACT_STRICT_MODE_TYPE;
   const isInStrictMode = parentIsInStrictMode || isStrictModeFiber;
 
-  if (fiber.flags & PlacementDEV || fiber.tag === OffscreenComponent) {
+  // First case: the fiber **is not** of type OffscreenComponent. No
+  // special rules apply to double invoking effects.
+  if (fiber.tag !== OffscreenComponent) {
+    if (fiber.flags & PlacementDEV) {
+      setCurrentDebugFiberInDEV(fiber);
+      if (isInStrictMode) {
+        doubleInvokeEffectsOnFiber(root, fiber);
+      }
+      resetCurrentDebugFiberInDEV();
+    } else {
+      recursivelyTraverseAndDoubleInvokeEffectsInDEV(
+        root,
+        fiber,
+        isInStrictMode,
+      );
+    }
+    return;
+  }
+
+  // Second case: the fiber **is** of type OffscreenComponent.
+  // This branch contains cases specific to Offscreen.
+  if (fiber.memoizedState === null) {
+    // Only consider Offscreen that is visible.
+    // TODO (Offscreen) Handle manual mode.
     setCurrentDebugFiberInDEV(fiber);
-    const isNotOffscreen = fiber.tag !== OffscreenComponent;
-    // Checks if Offscreen is being revealed. For all other components, evaluates to true.
-    const hasOffscreenBecomeVisible =
-      isNotOffscreen ||
-      (fiber.flags & Visibility && fiber.memoizedState === null);
-    if (isInStrictMode && hasOffscreenBecomeVisible) {
-      disappearLayoutEffects(fiber);
-      disconnectPassiveEffect(fiber);
-      reappearLayoutEffects(root, fiber.alternate, fiber, false);
-      reconnectPassiveEffects(root, fiber, NoLanes, null, false);
+    if (isInStrictMode && fiber.flags & Visibility) {
+      // Double invoke effects on Offscreen's subtree only
+      // if it is visible and its visibility has changed.
+      doubleInvokeEffectsOnFiber(root, fiber);
+    } else if (fiber.subtreeFlags & PlacementDEV) {
+      // Something in the subtree could have been suspended.
+      // We need to continue traversal and find newly inserted fibers.
+      recursivelyTraverseAndDoubleInvokeEffectsInDEV(
+        root,
+        fiber,
+        isInStrictMode,
+      );
     }
     resetCurrentDebugFiberInDEV();
-  } else {
-    recursivelyTraverseAndDoubleInvokeEffectsInDEV(root, fiber, isInStrictMode);
   }
 }
 

diff --git a/packages/react-reconciler/src/__tests__/ReactOffscreenStrictMode-test.js b/packages/react-reconciler/src/__tests__/ReactOffscreenStrictMode-test.js
@@ -153,4 +153,77 @@ describe('ReactOffscreenStrictMode', () => {
       );
     });
   });
+
+  // @gate __DEV__ && enableStrictEffects && enableOffscreen
+  it('should double invoke effects on unsuspended child', async () => {
+    let shouldSuspend = true;
+    let resolve;
+    const suspensePromise = new Promise(_resolve => {
+      resolve = _resolve;
+    });
+
+    function Parent() {
+      log.push('Parent rendered');
+      React.useEffect(() => {
+        log.push('Parent mount');
+        return () => {
+          log.push('Parent unmount');
+        };
+      });
+
+      return (
+        <React.Suspense fallback="fallback">
+          <Child />
+        </React.Suspense>
+      );
+    }
+
+    function Child() {
+      log.push('Child rendered');
+      React.useEffect(() => {
+        log.push('Child mount');
+        return () => {
+          log.push('Child unmount');
+        };
+      });
+      if (shouldSuspend) {
+        log.push('Child suspended');
+        throw suspensePromise;
+      }
+      return null;
+    }
+
+    act(() => {
+      ReactNoop.render(
+        <React.StrictMode>
+          <Offscreen mode="visible">
+            <Parent />
+          </Offscreen>
+        </React.StrictMode>,
+      );
+    });
+
+    log.push('------------------------------');
+
+    await act(async () => {
+      resolve();
+      shouldSuspend = false;
+    });
+
+    expect(log).toEqual([
+      'Parent rendered',
+      'Parent rendered',
+      'Child rendered',
+      'Child suspended',
+      'Parent mount',
+      'Parent unmount',
+      'Parent mount',
+      '------------------------------',
+      'Child rendered',
+      'Child rendered',
+      'Child mount',
+      'Child unmount',
+      'Child mount',
+    ]);
+  });
 });