Only double invoke newly added components

sammy-SC · rbalicki2 · sammy-SC · commit 9a30e80d4d30 · 2022-09-25T19:23:28.000+01:00
We need to keep track of fibers that have passed double invocation.
To achieve this, this commit repurposes flag PlacementDEV to
`NeedsDoubleInvokedEffectsDEV`.
Previous approach with Offscreen was not enough. When previously visible
Offscreen is hidden, it can have descendants added. Once Offscreen
becomes visible, only newly added components have to have their effects
double invoked. Not the previously existing components.

Co-authored-by: Robert Balicki &lt;robertbalicki@fb.com&gt;
diff --git a/packages/react-reconciler/src/ReactChildFiber.new.js b/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;
   }
diff --git a/packages/react-reconciler/src/ReactChildFiber.old.js b/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;
   }
diff --git a/packages/react-reconciler/src/ReactFiber.new.js b/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.
 
diff --git a/packages/react-reconciler/src/ReactFiber.old.js b/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.
 
diff --git a/packages/react-reconciler/src/ReactFiberFlags.js b/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.
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -123,7 +123,7 @@ import {
   MutationMask,
   LayoutMask,
   PassiveMask,
-  PlacementDEV,
+  NeedsDoubleInvokedEffectsDEV,
   Visibility,
 } from './ReactFiberFlags';
 import {
@@ -3239,24 +3239,33 @@ 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) {
+  if (parentFiber.subtreeFlags & NeedsDoubleInvokedEffectsDEV) {
+    parentFiber.flags &= ~NeedsDoubleInvokedEffectsDEV;
+    let child = parentFiber.child;
+    while (child !== null) {
+      recursivelyUnsetNeedsDoubleInvokedEffectsDEV(child);
+      child = child.sibling;
+    }
+  }
 }
 
 function doubleInvokeEffectsInDEVIfNecessary(
@@ -3270,10 +3279,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 +3300,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);
   }
 }
 
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
@@ -123,7 +123,7 @@ import {
   MutationMask,
   LayoutMask,
   PassiveMask,
-  PlacementDEV,
+  NeedsDoubleInvokedEffectsDEV,
   Visibility,
 } from './ReactFiberFlags';
 import {
@@ -3239,24 +3239,33 @@ 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) {
+  if (parentFiber.subtreeFlags & NeedsDoubleInvokedEffectsDEV) {
+    parentFiber.flags &= ~NeedsDoubleInvokedEffectsDEV;
+    let child = parentFiber.child;
+    while (child !== null) {
+      recursivelyUnsetNeedsDoubleInvokedEffectsDEV(child);
+      child = child.sibling;
+    }
+  }
 }
 
 function doubleInvokeEffectsInDEVIfNecessary(
@@ -3270,10 +3279,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 +3300,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);
   }
 }
 
diff --git a/packages/react-reconciler/src/__tests__/ReactOffscreenStrictMode-test.js b/packages/react-reconciler/src/__tests__/ReactOffscreenStrictMode-test.js
