Add random comment

packages/react-reconciler/src/ReactFiberOffscreenComponent.js

@@ -21,7 +21,7 @@ export type OffscreenProps = {
   // content without changing the layout.
   //
   // Default mode is visible. Kind of a weird default for a component
-  // called "Offscreen." Possible alt: <Visibility />?
+  // called "Offscreen." Possible alt: <Visibility />??
   mode?: OffscreenMode | null | void,
   children?: ReactNodeList,
 };

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

@@ -40,7 +40,6 @@ import {
   enableUpdaterTracking,
   enableCache,
   enableTransitionTracing,
-  useModernStrictMode,
 } from 'shared/ReactFeatureFlags';
 import ReactSharedInternals from 'shared/ReactSharedInternals';
 import is from 'shared/objectIs';
@@ -63,6 +62,7 @@ import {
   scheduleSyncCallback,
   scheduleLegacySyncCallback,
 } from './ReactFiberSyncTaskQueue.new';
+import {OffscreenPassiveEffectsConnected} from './ReactFiberOffscreenComponent';
 import {
   logCommitStarted,
   logCommitStopped,
@@ -116,7 +116,6 @@ import {
   Profiler,
 } from './ReactWorkTags';
 import {ConcurrentRoot, LegacyRoot} from './ReactRootTags';
-import type {Flags} from './ReactFiberFlags';
 import {
   NoFlags,
   Incomplete,
@@ -129,8 +128,6 @@ import {
   PassiveMask,
   PlacementDEV,
   Visibility,
-  MountPassiveDev,
-  MountLayoutDev,
 } from './ReactFiberFlags';
 import {
   NoLanes,
@@ -195,10 +192,6 @@ import {
   reappearLayoutEffects,
   disconnectPassiveEffect,
   reportUncaughtErrorInDEV,
-  invokeLayoutEffectMountInDEV,
-  invokePassiveEffectMountInDEV,
-  invokeLayoutEffectUnmountInDEV,
-  invokePassiveEffectUnmountInDEV,
 } from './ReactFiberCommitWork.new';
 import {enqueueUpdate} from './ReactFiberClassUpdateQueue.new';
 import {resetContextDependencies} from './ReactFiberNewContext.new';
@@ -3291,11 +3284,19 @@ function recursivelyTraverseAndDoubleInvokeEffectsInDEV(
 }
 
 // Unconditionally disconnects and connects passive and layout effects.
-function doubleInvokeEffectsOnFiber(root: FiberRoot, fiber: Fiber) {
+function doubleInvokeEffectsOnFiber(
+  root: FiberRoot,
+  fiber: Fiber,
+  includePassiveEffects: boolean,
+) {
   disappearLayoutEffects(fiber);
-  disconnectPassiveEffect(fiber);
+  if (includePassiveEffects) {
+    disconnectPassiveEffect(fiber);
+  }
   reappearLayoutEffects(root, fiber.alternate, fiber, false);
-  reconnectPassiveEffects(root, fiber, NoLanes, null, false);
+  if (includePassiveEffects) {
+    reconnectPassiveEffects(root, fiber, NoLanes, null, false);
+  }
 }
 
 function doubleInvokeEffectsInDEVIfNecessary(
@@ -3312,7 +3313,7 @@ function doubleInvokeEffectsInDEVIfNecessary(
     if (fiber.flags & PlacementDEV) {
       setCurrentDebugFiberInDEV(fiber);
       if (isInStrictMode) {
-        doubleInvokeEffectsOnFiber(root, fiber);
+        doubleInvokeEffectsOnFiber(root, fiber, true);
       }
       resetCurrentDebugFiberInDEV();
     } else {
@@ -3334,7 +3335,15 @@ function doubleInvokeEffectsInDEVIfNecessary(
     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);
+
+      // For backwards compatibility, we don't unmount passive effects when a tree suspends.
+      // StrictMode needs to align with this.
+      const includePassiveEffects: boolean =
+        (fiber.stateNode._visibility & OffscreenPassiveEffectsConnected) !==
+          0 &&
+        fiber.return !== null &&
+        fiber.return.tag !== SuspenseComponent;
+      doubleInvokeEffectsOnFiber(root, fiber, includePassiveEffects);
     } else if (fiber.subtreeFlags & PlacementDEV) {
       // Something in the subtree could have been suspended.
       // We need to continue traversal and find newly inserted fibers.
@@ -3353,76 +3362,22 @@ function commitDoubleInvokeEffectsInDEV(
   hasPassiveEffects: boolean,
 ) {
   if (__DEV__ && enableStrictEffects) {
-    if (useModernStrictMode) {
-      let doubleInvokeEffects = true;
+    let doubleInvokeEffects = true;
 
-      if (root.tag === LegacyRoot && !(root.current.mode & StrictLegacyMode)) {
-        doubleInvokeEffects = false;
-      }
-      if (
-        root.tag === ConcurrentRoot &&
-        !(root.current.mode & (StrictLegacyMode | StrictEffectsMode))
-      ) {
-        doubleInvokeEffects = false;
-      }
-      recursivelyTraverseAndDoubleInvokeEffectsInDEV(
-        root,
-        root.current,
-        doubleInvokeEffects,
-      );
-    } else {
-      legacyCommitDoubleInvokeEffectsInDEV(root.current, hasPassiveEffects);
+    if (root.tag === LegacyRoot && !(root.current.mode & StrictLegacyMode)) {
+      doubleInvokeEffects = false;
     }
-  }
-}
-
-function legacyCommitDoubleInvokeEffectsInDEV(
-  fiber: Fiber,
-  hasPassiveEffects: boolean,
-) {
-  // TODO (StrictEffects) Should we set a marker on the root if it contains strict effects
-  // so we don't traverse unnecessarily? similar to subtreeFlags but just at the root level.
-  // Maybe not a big deal since this is DEV only behavior.
-
-  setCurrentDebugFiberInDEV(fiber);
-  invokeEffectsInDev(fiber, MountLayoutDev, invokeLayoutEffectUnmountInDEV);
-  if (hasPassiveEffects) {
-    invokeEffectsInDev(fiber, MountPassiveDev, invokePassiveEffectUnmountInDEV);
-  }
-
-  invokeEffectsInDev(fiber, MountLayoutDev, invokeLayoutEffectMountInDEV);
-  if (hasPassiveEffects) {
-    invokeEffectsInDev(fiber, MountPassiveDev, invokePassiveEffectMountInDEV);
-  }
-  resetCurrentDebugFiberInDEV();
-}
-
-function invokeEffectsInDev(
-  firstChild: Fiber,
-  fiberFlags: Flags,
-  invokeEffectFn: (fiber: Fiber) => void,
-) {
-  let current: null | Fiber = firstChild;
-  let subtreeRoot = null;
-  while (current != null) {
-    const primarySubtreeFlag = current.subtreeFlags & fiberFlags;
     if (
-      current !== subtreeRoot &&
-      current.child != null &&
-      primarySubtreeFlag !== NoFlags
+      root.tag === ConcurrentRoot &&
+      !(root.current.mode & (StrictLegacyMode | StrictEffectsMode))
     ) {
-      current = current.child;
-    } else {
-      if ((current.flags & fiberFlags) !== NoFlags) {
-        invokeEffectFn(current);
-      }
-
-      if (current.sibling !== null) {
-        current = current.sibling;
-      } else {
-        current = subtreeRoot = current.return;
-      }
+      doubleInvokeEffects = false;
     }
+    recursivelyTraverseAndDoubleInvokeEffectsInDEV(
+      root,
+      root.current,
+      doubleInvokeEffects,
+    );
   }
 }
 

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

@@ -40,7 +40,6 @@ import {
   enableUpdaterTracking,
   enableCache,
   enableTransitionTracing,
-  useModernStrictMode,
 } from 'shared/ReactFeatureFlags';
 import ReactSharedInternals from 'shared/ReactSharedInternals';
 import is from 'shared/objectIs';
@@ -63,6 +62,7 @@ import {
   scheduleSyncCallback,
   scheduleLegacySyncCallback,
 } from './ReactFiberSyncTaskQueue.old';
+import {OffscreenPassiveEffectsConnected} from './ReactFiberOffscreenComponent';
 import {
   logCommitStarted,
   logCommitStopped,
@@ -116,7 +116,6 @@ import {
   Profiler,
 } from './ReactWorkTags';
 import {ConcurrentRoot, LegacyRoot} from './ReactRootTags';
-import type {Flags} from './ReactFiberFlags';
 import {
   NoFlags,
   Incomplete,
@@ -129,8 +128,6 @@ import {
   PassiveMask,
   PlacementDEV,
   Visibility,
-  MountPassiveDev,
-  MountLayoutDev,
 } from './ReactFiberFlags';
 import {
   NoLanes,
@@ -195,10 +192,6 @@ import {
   reappearLayoutEffects,
   disconnectPassiveEffect,
   reportUncaughtErrorInDEV,
-  invokeLayoutEffectMountInDEV,
-  invokePassiveEffectMountInDEV,
-  invokeLayoutEffectUnmountInDEV,
-  invokePassiveEffectUnmountInDEV,
 } from './ReactFiberCommitWork.old';
 import {enqueueUpdate} from './ReactFiberClassUpdateQueue.old';
 import {resetContextDependencies} from './ReactFiberNewContext.old';
@@ -3291,11 +3284,19 @@ function recursivelyTraverseAndDoubleInvokeEffectsInDEV(
 }
 
 // Unconditionally disconnects and connects passive and layout effects.
-function doubleInvokeEffectsOnFiber(root: FiberRoot, fiber: Fiber) {
+function doubleInvokeEffectsOnFiber(
+  root: FiberRoot,
+  fiber: Fiber,
+  includePassiveEffects: boolean,
+) {
   disappearLayoutEffects(fiber);
-  disconnectPassiveEffect(fiber);
+  if (includePassiveEffects) {
+    disconnectPassiveEffect(fiber);
+  }
   reappearLayoutEffects(root, fiber.alternate, fiber, false);
-  reconnectPassiveEffects(root, fiber, NoLanes, null, false);
+  if (includePassiveEffects) {
+    reconnectPassiveEffects(root, fiber, NoLanes, null, false);
+  }
 }
 
 function doubleInvokeEffectsInDEVIfNecessary(
@@ -3312,7 +3313,7 @@ function doubleInvokeEffectsInDEVIfNecessary(
     if (fiber.flags & PlacementDEV) {
       setCurrentDebugFiberInDEV(fiber);
       if (isInStrictMode) {
-        doubleInvokeEffectsOnFiber(root, fiber);
+        doubleInvokeEffectsOnFiber(root, fiber, true);
       }
       resetCurrentDebugFiberInDEV();
     } else {
@@ -3334,7 +3335,15 @@ function doubleInvokeEffectsInDEVIfNecessary(
     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);
+
+      // For backwards compatibility, we don't unmount passive effects when a tree suspends.
+      // StrictMode needs to align with this.
+      const includePassiveEffects: boolean =
+        (fiber.stateNode._visibility & OffscreenPassiveEffectsConnected) !==
+          0 &&
+        fiber.return !== null &&
+        fiber.return.tag !== SuspenseComponent;
+      doubleInvokeEffectsOnFiber(root, fiber, includePassiveEffects);
     } else if (fiber.subtreeFlags & PlacementDEV) {
       // Something in the subtree could have been suspended.
       // We need to continue traversal and find newly inserted fibers.
@@ -3353,76 +3362,22 @@ function commitDoubleInvokeEffectsInDEV(
   hasPassiveEffects: boolean,
 ) {
   if (__DEV__ && enableStrictEffects) {
-    if (useModernStrictMode) {
-      let doubleInvokeEffects = true;
+    let doubleInvokeEffects = true;
 
-      if (root.tag === LegacyRoot && !(root.current.mode & StrictLegacyMode)) {
-        doubleInvokeEffects = false;
-      }
-      if (
-        root.tag === ConcurrentRoot &&
-        !(root.current.mode & (StrictLegacyMode | StrictEffectsMode))
-      ) {
-        doubleInvokeEffects = false;
-      }
-      recursivelyTraverseAndDoubleInvokeEffectsInDEV(
-        root,
-        root.current,
-        doubleInvokeEffects,
-      );
-    } else {
-      legacyCommitDoubleInvokeEffectsInDEV(root.current, hasPassiveEffects);
+    if (root.tag === LegacyRoot && !(root.current.mode & StrictLegacyMode)) {
+      doubleInvokeEffects = false;
     }
-  }
-}
-
-function legacyCommitDoubleInvokeEffectsInDEV(
-  fiber: Fiber,
-  hasPassiveEffects: boolean,
-) {
-  // TODO (StrictEffects) Should we set a marker on the root if it contains strict effects
-  // so we don't traverse unnecessarily? similar to subtreeFlags but just at the root level.
-  // Maybe not a big deal since this is DEV only behavior.
-
-  setCurrentDebugFiberInDEV(fiber);
-  invokeEffectsInDev(fiber, MountLayoutDev, invokeLayoutEffectUnmountInDEV);
-  if (hasPassiveEffects) {
-    invokeEffectsInDev(fiber, MountPassiveDev, invokePassiveEffectUnmountInDEV);
-  }
-
-  invokeEffectsInDev(fiber, MountLayoutDev, invokeLayoutEffectMountInDEV);
-  if (hasPassiveEffects) {
-    invokeEffectsInDev(fiber, MountPassiveDev, invokePassiveEffectMountInDEV);
-  }
-  resetCurrentDebugFiberInDEV();
-}
-
-function invokeEffectsInDev(
-  firstChild: Fiber,
-  fiberFlags: Flags,
-  invokeEffectFn: (fiber: Fiber) => void,
-) {
-  let current: null | Fiber = firstChild;
-  let subtreeRoot = null;
-  while (current != null) {
-    const primarySubtreeFlag = current.subtreeFlags & fiberFlags;
     if (
-      current !== subtreeRoot &&
-      current.child != null &&
-      primarySubtreeFlag !== NoFlags
+      root.tag === ConcurrentRoot &&
+      !(root.current.mode & (StrictLegacyMode | StrictEffectsMode))
     ) {
-      current = current.child;
-    } else {
-      if ((current.flags & fiberFlags) !== NoFlags) {
-        invokeEffectFn(current);
-      }
-
-      if (current.sibling !== null) {
-        current = current.sibling;
-      } else {
-        current = subtreeRoot = current.return;
-      }
+      doubleInvokeEffects = false;
     }
+    recursivelyTraverseAndDoubleInvokeEffectsInDEV(
+      root,
+      root.current,
+      doubleInvokeEffects,
+    );
   }
 }
 

packages/react-reconciler/src/__tests__/ReactOffscreenStrictMode-test.js

@@ -203,13 +203,6 @@ describe('ReactOffscreenStrictMode', () => {
       );
     });
 
-    log.push('------------------------------');
-
-    await act(async () => {
-      resolve();
-      shouldSuspend = false;
-    });
-
     expect(log).toEqual([
       'Parent rendered',
       'Parent rendered',
@@ -218,12 +211,15 @@ describe('ReactOffscreenStrictMode', () => {
       'Parent mount',
       'Parent unmount',
       'Parent mount',
-      '------------------------------',
-      'Child rendered',
-      'Child rendered',
-      'Child mount',
-      'Child unmount',
-      'Child mount',
     ]);
+
+    log = [];
+
+    await act(async () => {
+      resolve();
+      shouldSuspend = false;
+    });
+
+    expect(log).toEqual(['Child rendered', 'Child rendered', 'Child mount']);
   });
 });

packages/react-reconciler/src/__tests__/ReactOffscreenSuspense-test.js

@@ -571,4 +571,75 @@ describe('ReactOffscreen', () => {
       </>,
     );
   });
+
+  // @gate __DEV__ && enableStrictEffects
+  it('suspending a mounted tree unmounts layout effects but not passive effects', async () => {
+    const root = ReactNoop.createRoot();
+    let shouldSuspend = false;
+
+    function Child({text}) {
+      useEffect(() => {
+        Scheduler.unstable_yieldValue(`${text} mounted`);
+        return () => {
+          Scheduler.unstable_yieldValue(`${text} unmounted`);
+        };
+      });
+
+      React.useLayoutEffect(() => {
+        Scheduler.unstable_yieldValue(`${text} layout mounted`);
+        return () => {
+          Scheduler.unstable_yieldValue(`${text} layout unmounted`);
+        };
+      });
+
+      if (shouldSuspend) {
+        Scheduler.unstable_yieldValue(`${text} suspended`);
+        throw new Promise(_resolve => {
+          // do nothing.
+        });
+      }
+
+      Scheduler.unstable_yieldValue(`${text} rendered`);
+      return <span>{text}</span>;
+    }
+
+    // Initial mount. Nothing suspends.
+    await act(async () => {
+      root.render(
+        <React.StrictMode>
+          <Suspense fallback={<Text text="Loading..." />}>
+            <Child text="A" />
+          </Suspense>
+        </React.StrictMode>,
+      );
+    });
+
+    expect(Scheduler).toHaveYielded([
+      'A rendered',
+      'A layout mounted',
+      'A mounted',
+      'A layout unmounted',
+      'A unmounted',
+      'A layout mounted',
+      'A mounted',
+    ]);
+
+    // Initial mount. Child suspends.
+    await act(async () => {
+      shouldSuspend = true;
+      root.render(
+        <React.StrictMode>
+          <Suspense fallback={<Text text="Loading..." />}>
+            <Child text="B" />
+          </Suspense>
+        </React.StrictMode>,
+      );
+    });
+
+    expect(Scheduler).toHaveYielded([
+      'B suspended',
+      'Loading...',
+      'A layout unmounted',
+    ]);
+  });
 });