Strict mode fix offscreen edge case

packages/react-reconciler/src/ReactChildFiber.new.js

@@ -17,7 +17,7 @@ import {
   Placement,
   ChildDeletion,
   Forked,
-  PlacementDEV,
+  NeedsDoubleInvokedEffectsDEV,
 } from './ReactFiberFlags';
 import {
   getIteratorFn,
@@ -349,15 +349,15 @@ function ChildReconciler(shouldTrackSideEffects) {
       const oldIndex = current.index;
       if (oldIndex < lastPlacedIndex) {
         // This is a move.
-        newFiber.flags |= Placement | PlacementDEV;
+        newFiber.flags |= Placement | NeedsDoubleInvokedEffectsDEV;
         return lastPlacedIndex;
       } else {
         // This item can stay in place.
         return oldIndex;
       }
     } else {
       // This is an insertion.
-      newFiber.flags |= Placement | PlacementDEV;
+      newFiber.flags |= Placement | NeedsDoubleInvokedEffectsDEV;
       return lastPlacedIndex;
     }
   }
@@ -366,7 +366,7 @@ function ChildReconciler(shouldTrackSideEffects) {
     // This is simpler for the single child case. We only need to do a
     // placement for inserting new children.
     if (shouldTrackSideEffects && newFiber.alternate === null) {
-      newFiber.flags |= Placement | PlacementDEV;
+      newFiber.flags |= Placement | NeedsDoubleInvokedEffectsDEV;
     }
     return newFiber;
   }

packages/react-reconciler/src/ReactChildFiber.old.js

@@ -17,7 +17,7 @@ import {
   Placement,
   ChildDeletion,
   Forked,
-  PlacementDEV,
+  NeedsDoubleInvokedEffectsDEV,
 } from './ReactFiberFlags';
 import {
   getIteratorFn,
@@ -349,15 +349,15 @@ function ChildReconciler(shouldTrackSideEffects) {
       const oldIndex = current.index;
       if (oldIndex < lastPlacedIndex) {
         // This is a move.
-        newFiber.flags |= Placement | PlacementDEV;
+        newFiber.flags |= Placement | NeedsDoubleInvokedEffectsDEV;
         return lastPlacedIndex;
       } else {
         // This item can stay in place.
         return oldIndex;
       }
     } else {
       // This is an insertion.
-      newFiber.flags |= Placement | PlacementDEV;
+      newFiber.flags |= Placement | NeedsDoubleInvokedEffectsDEV;
       return lastPlacedIndex;
     }
   }
@@ -366,7 +366,7 @@ function ChildReconciler(shouldTrackSideEffects) {
     // This is simpler for the single child case. We only need to do a
     // placement for inserting new children.
     if (shouldTrackSideEffects && newFiber.alternate === null) {
-      newFiber.flags |= Placement | PlacementDEV;
+      newFiber.flags |= Placement | NeedsDoubleInvokedEffectsDEV;
     }
     return newFiber;
   }

packages/react-reconciler/src/ReactFiber.new.js

@@ -33,7 +33,12 @@ import {
   enableTransitionTracing,
   enableDebugTracing,
 } from 'shared/ReactFeatureFlags';
-import {NoFlags, Placement, StaticMask} from './ReactFiberFlags';
+import {
+  NoFlags,
+  Placement,
+  StaticMask,
+  NeedsDoubleInvokedEffectsDEV,
+} from './ReactFiberFlags';
 import {ConcurrentRoot} from './ReactRootTags';
 import {
   IndeterminateComponent,
@@ -300,9 +305,12 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber {
     }
   }
 
-  // Reset all effects except static ones.
+  // Reset all effects except static ones NeedsDoubleInvokedEffectsDEV.
   // Static effects are not specific to a render.
-  workInProgress.flags = current.flags & StaticMask;
+  // NeedsDoubleInvokedEffectsDEV is cleared when component is double invoked
+  // in debugging. It is only used with StrictMode enabled.
+  workInProgress.flags =
+    current.flags & (StaticMask | NeedsDoubleInvokedEffectsDEV);
   workInProgress.childLanes = current.childLanes;
   workInProgress.lanes = current.lanes;
 
@@ -369,7 +377,9 @@ export function resetWorkInProgress(
 
   // Reset the effect flags but keep any Placement tags, since that's something
   // that child fiber is setting, not the reconciliation.
-  workInProgress.flags &= StaticMask | Placement;
+  // NeedsDoubleInvokedEffectsDEV is cleared when component is double invoked
+  // in debugging. It is only used with StrictMode enabled.
+  workInProgress.flags &= StaticMask | Placement | NeedsDoubleInvokedEffectsDEV;
 
   // The effects are no longer valid.
 

packages/react-reconciler/src/ReactFiber.old.js

@@ -33,7 +33,12 @@ import {
   enableTransitionTracing,
   enableDebugTracing,
 } from 'shared/ReactFeatureFlags';
-import {NoFlags, Placement, StaticMask} from './ReactFiberFlags';
+import {
+  NoFlags,
+  Placement,
+  StaticMask,
+  NeedsDoubleInvokedEffectsDEV,
+} from './ReactFiberFlags';
 import {ConcurrentRoot} from './ReactRootTags';
 import {
   IndeterminateComponent,
@@ -300,9 +305,12 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber {
     }
   }
 
-  // Reset all effects except static ones.
+  // Reset all effects except static ones NeedsDoubleInvokedEffectsDEV.
   // Static effects are not specific to a render.
-  workInProgress.flags = current.flags & StaticMask;
+  // NeedsDoubleInvokedEffectsDEV is cleared when component is double invoked
+  // in debugging. It is only used with StrictMode enabled.
+  workInProgress.flags =
+    current.flags & (StaticMask | NeedsDoubleInvokedEffectsDEV);
   workInProgress.childLanes = current.childLanes;
   workInProgress.lanes = current.lanes;
 
@@ -369,7 +377,9 @@ export function resetWorkInProgress(
 
   // Reset the effect flags but keep any Placement tags, since that's something
   // that child fiber is setting, not the reconciliation.
-  workInProgress.flags &= StaticMask | Placement;
+  // NeedsDoubleInvokedEffectsDEV is cleared when component is double invoked
+  // in debugging. It is only used with StrictMode enabled.
+  workInProgress.flags &= StaticMask | Placement | NeedsDoubleInvokedEffectsDEV;
 
   // The effects are no longer valid.
 

packages/react-reconciler/src/ReactFiberFlags.js

@@ -53,8 +53,8 @@ export const RefStatic = /*                    */ 0b000100000000000000000000;
 export const LayoutStatic = /*                 */ 0b001000000000000000000000;
 export const PassiveStatic = /*                */ 0b010000000000000000000000;
 
-// Flag used to identify newly inserted fibers. It isn't reset after commit unlike `Placement`.
-export const PlacementDEV = /*                 */ 0b100000000000000000000000;
+// Flag used to identify fibers that need to have effects double invoked.
+export const NeedsDoubleInvokedEffectsDEV = /* */ 0b100000000000000000000000;
 
 // Groups of flags that are used in the commit phase to skip over trees that
 // don't contain effects, by checking subtreeFlags.

packages/react-reconciler/src/ReactFiberWorkLoop.new.js

@@ -123,7 +123,7 @@ import {
   MutationMask,
   LayoutMask,
   PassiveMask,
-  PlacementDEV,
+  NeedsDoubleInvokedEffectsDEV,
   Visibility,
 } from './ReactFiberFlags';
 import {
@@ -3239,24 +3239,31 @@ function recursivelyTraverseAndDoubleInvokeEffectsInDEV(
   parentFiber: Fiber,
   isInStrictMode: boolean,
 ) {
-  if ((parentFiber.subtreeFlags & (PlacementDEV | Visibility)) === NoFlags) {
+  if (
+    (parentFiber.subtreeFlags & (NeedsDoubleInvokedEffectsDEV | Visibility)) ===
+    NoFlags
+  ) {
     // Parent's descendants have already had effects double invoked.
     // Early exit to avoid unnecessary tree traversal.
     return;
   }
+
   let child = parentFiber.child;
   while (child !== null) {
     doubleInvokeEffectsInDEVIfNecessary(root, child, isInStrictMode);
     child = child.sibling;
   }
 }
 
-// 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);
+// NeedsDoubleInvokedEffectsDEV needs to be unset once effects are double invoked.
+// This is to keep track of fibers which need to have their effects double invoked.
+function recursivelyUnsetNeedsDoubleInvokedEffectsDEV(parentFiber: Fiber) {
+  parentFiber.flags &= ~NeedsDoubleInvokedEffectsDEV;
+  let child = parentFiber.child;
+  while (child !== null) {
+    recursivelyUnsetNeedsDoubleInvokedEffectsDEV(child);
+    child = child.sibling;
+  }
 }
 
 function doubleInvokeEffectsInDEVIfNecessary(
@@ -3270,10 +3277,14 @@ function doubleInvokeEffectsInDEVIfNecessary(
   // 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) {
+    if (fiber.flags & NeedsDoubleInvokedEffectsDEV) {
       setCurrentDebugFiberInDEV(fiber);
       if (isInStrictMode) {
-        doubleInvokeEffectsOnFiber(root, fiber);
+        disappearLayoutEffects(fiber);
+        disconnectPassiveEffect(fiber);
+        reappearLayoutEffects(root, fiber.alternate, fiber, false);
+        reconnectPassiveEffects(root, fiber, NoLanes, null, false);
+        recursivelyUnsetNeedsDoubleInvokedEffectsDEV(fiber);
       }
       resetCurrentDebugFiberInDEV();
     } else {
@@ -3287,25 +3298,10 @@ function doubleInvokeEffectsInDEVIfNecessary(
   }
 
   // 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.
+    // Only continue traversal if Offscreen is visible.
     // TODO (Offscreen) Handle manual mode.
-    setCurrentDebugFiberInDEV(fiber);
-    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();
+    recursivelyTraverseAndDoubleInvokeEffectsInDEV(root, fiber, isInStrictMode);
   }
 }
 

packages/react-reconciler/src/ReactFiberWorkLoop.old.js

@@ -123,7 +123,7 @@ import {
   MutationMask,
   LayoutMask,
   PassiveMask,
-  PlacementDEV,
+  NeedsDoubleInvokedEffectsDEV,
   Visibility,
 } from './ReactFiberFlags';
 import {
@@ -3239,24 +3239,31 @@ function recursivelyTraverseAndDoubleInvokeEffectsInDEV(
   parentFiber: Fiber,
   isInStrictMode: boolean,
 ) {
-  if ((parentFiber.subtreeFlags & (PlacementDEV | Visibility)) === NoFlags) {
+  if (
+    (parentFiber.subtreeFlags & (NeedsDoubleInvokedEffectsDEV | Visibility)) ===
+    NoFlags
+  ) {
     // Parent's descendants have already had effects double invoked.
     // Early exit to avoid unnecessary tree traversal.
     return;
   }
+
   let child = parentFiber.child;
   while (child !== null) {
     doubleInvokeEffectsInDEVIfNecessary(root, child, isInStrictMode);
     child = child.sibling;
   }
 }
 
-// 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);
+// NeedsDoubleInvokedEffectsDEV needs to be unset once effects are double invoked.
+// This is to keep track of fibers which need to have their effects double invoked.
+function recursivelyUnsetNeedsDoubleInvokedEffectsDEV(parentFiber: Fiber) {
+  parentFiber.flags &= ~NeedsDoubleInvokedEffectsDEV;
+  let child = parentFiber.child;
+  while (child !== null) {
+    recursivelyUnsetNeedsDoubleInvokedEffectsDEV(child);
+    child = child.sibling;
+  }
 }
 
 function doubleInvokeEffectsInDEVIfNecessary(
@@ -3270,10 +3277,14 @@ function doubleInvokeEffectsInDEVIfNecessary(
   // 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) {
+    if (fiber.flags & NeedsDoubleInvokedEffectsDEV) {
       setCurrentDebugFiberInDEV(fiber);
       if (isInStrictMode) {
-        doubleInvokeEffectsOnFiber(root, fiber);
+        disappearLayoutEffects(fiber);
+        disconnectPassiveEffect(fiber);
+        reappearLayoutEffects(root, fiber.alternate, fiber, false);
+        reconnectPassiveEffects(root, fiber, NoLanes, null, false);
+        recursivelyUnsetNeedsDoubleInvokedEffectsDEV(fiber);
       }
       resetCurrentDebugFiberInDEV();
     } else {
@@ -3287,25 +3298,10 @@ function doubleInvokeEffectsInDEVIfNecessary(
   }
 
   // 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.
+    // Only continue traversal if Offscreen is visible.
     // TODO (Offscreen) Handle manual mode.
-    setCurrentDebugFiberInDEV(fiber);
-    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();
+    recursivelyTraverseAndDoubleInvokeEffectsInDEV(root, fiber, isInStrictMode);
   }
 }