Call cleanup of insertion effects when hidden

Insertion effects do not unmount when a subtree goes offscreen, this means we still have to traverse the subtree in that case. We could potentially use a new subtree bit flag to avoid the traversal if there's no insertion effects, but I'm not sure that's worth the tradeoff of using a new flag? Likely also fixes facebook#26670
kassens · Sep 12, 2024 · 4b19a8e · 4b19a8e
1 parent 206df66
commit 4b19a8e
Show file tree

Hide file tree

Showing 2 changed files with 69 additions and 43 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js
@@ -1324,58 +1324,56 @@ function commitDeletionEffectsOnFiber(
     case ForwardRef:
     case MemoComponent:
     case SimpleMemoComponent: {
-      if (!offscreenSubtreeWasHidden) {
-        const updateQueue: FunctionComponentUpdateQueue | null =
-          (deletedFiber.updateQueue: any);
-        if (updateQueue !== null) {
-          const lastEffect = updateQueue.lastEffect;
-          if (lastEffect !== null) {
-            const firstEffect = lastEffect.next;
-
-            let effect = firstEffect;
-            do {
-              const tag = effect.tag;
-              const inst = effect.inst;
-              const destroy = inst.destroy;
-              if (destroy !== undefined) {
-                if ((tag & HookInsertion) !== NoHookEffect) {
+      const updateQueue: FunctionComponentUpdateQueue | null =
+        (deletedFiber.updateQueue: any);
+      if (updateQueue !== null) {
+        const lastEffect = updateQueue.lastEffect;
+        if (lastEffect !== null) {
+          const firstEffect = lastEffect.next;
+
+          let effect = firstEffect;
+          do {
+            const tag = effect.tag;
+            const inst = effect.inst;
+            const destroy = inst.destroy;
+            if (destroy !== undefined) {
+              if ((tag & HookInsertion) !== NoHookEffect) {
+                inst.destroy = undefined;
+                safelyCallDestroy(
+                  deletedFiber,
+                  nearestMountedAncestor,
+                  destroy,
+                );
+              } else if ((tag & HookLayout) !== NoHookEffect) {
+                if (enableSchedulingProfiler) {
+                  markComponentLayoutEffectUnmountStarted(deletedFiber);
+                }
+
+                if (shouldProfile(deletedFiber)) {
+                  startLayoutEffectTimer();
                   inst.destroy = undefined;
                   safelyCallDestroy(
                     deletedFiber,
                     nearestMountedAncestor,
                     destroy,
                   );
-                } else if ((tag & HookLayout) !== NoHookEffect) {
-                  if (enableSchedulingProfiler) {
-                    markComponentLayoutEffectUnmountStarted(deletedFiber);
-                  }
-
-                  if (shouldProfile(deletedFiber)) {
-                    startLayoutEffectTimer();
-                    inst.destroy = undefined;
-                    safelyCallDestroy(
-                      deletedFiber,
-                      nearestMountedAncestor,
-                      destroy,
-                    );
-                    recordLayoutEffectDuration(deletedFiber);
-                  } else {
-                    inst.destroy = undefined;
-                    safelyCallDestroy(
-                      deletedFiber,
-                      nearestMountedAncestor,
-                      destroy,
-                    );
-                  }
+                  recordLayoutEffectDuration(deletedFiber);
+                } else {
+                  inst.destroy = undefined;
+                  safelyCallDestroy(
+                    deletedFiber,
+                    nearestMountedAncestor,
+                    destroy,
+                  );
+                }
 
-                  if (enableSchedulingProfiler) {
-                    markComponentLayoutEffectUnmountStopped();
-                  }
+                if (enableSchedulingProfiler) {
+                  markComponentLayoutEffectUnmountStopped();
                 }
               }
-              effect = effect.next;
-            } while (effect !== firstEffect);
-          }
+            }
+            effect = effect.next;
+          } while (effect !== firstEffect);
         }
       }
 

diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
@@ -2872,6 +2872,34 @@ describe('ReactHooksWithNoopRenderer', () => {
       });
     });
 
+    // @gate enableActivity
+    it('runs insertion effect cleanup when unmounting in Offscreen state', async () => {
+      function Logger(props) {
+        useInsertionEffect(() => {
+          Scheduler.log(`create`);
+          return () => {
+            Scheduler.log(`destroy`);
+          };
+        }, []);
+        return null;
+      }
+
+      const Activity = React.unstable_Activity;
+      await act(async () => {
+        ReactNoop.render(
+          <Activity mode="hidden">
+            <Logger name="hidden" />
+          </Activity>,
+        );
+        await waitForAll(['create']);
+      });
+
+      await act(async () => {
+        ReactNoop.render(null);
+        await waitForAll(['destroy']);
+      });
+    });
+
     it('assumes insertion effect destroy function is either a function or undefined', async () => {
       function App(props) {
         useInsertionEffect(() => {