Repository: facebook/react
Pull request: Double invoked effects on suspended children in StrictMode

There is another special case I missed when reworking StrictMode for Offscreen.
When Offscreen becomes visible, its children may be suspended. If that happens, once the children are unsuspended (what's the term for this?), StrictMode needs to make sure to double invoke the children. In the previous implementation, that was not the case because we only looked if Offscreen became visible or not.

Changed files:
filename: packages/react-reconciler/src/ReactFiberWorkLoop.new.js
patch: @@ -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);
   }
 }
 

filename: packages/react-reconciler/src/ReactFiberWorkLoop.old.js
patch: @@ -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);
   }
 }
 

filename: packages/react-reconciler/src/__tests__/ReactOffscreenStrictMode-test.js
patch: @@ -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',
+    ]);
+  });
 });