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. Likely also fixes #26670
facebook · Sep 13, 2024 · 91bf47c · 91bf47c
1 parent 206df66
commit 91bf47c
Show file tree

Hide file tree

Showing 11 changed files with 50 additions and 1 deletion.
diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js
@@ -40,6 +40,7 @@ import type {
 import {
   alwaysThrottleRetries,
   enableCreateEventHandleAPI,
+  enableHiddenSubtreeInsertionEffectCleanup,
   enablePersistedModeClonedFlag,
   enableProfilerTimer,
   enableProfilerCommitHooks,
@@ -1324,7 +1325,10 @@ function commitDeletionEffectsOnFiber(
     case ForwardRef:
     case MemoComponent:
     case SimpleMemoComponent: {
-      if (!offscreenSubtreeWasHidden) {
+      if (
+        enableHiddenSubtreeInsertionEffectCleanup ||
+        !offscreenSubtreeWasHidden
+      ) {
         const updateQueue: FunctionComponentUpdateQueue | null =
           (deletedFiber.updateQueue: any);
         if (updateQueue !== null) {

diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
@@ -2872,6 +2872,38 @@ 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(
+          gate(flags => flags.enableHiddenSubtreeInsertionEffectCleanup)
+            ? ['destroy']
+            : [],
+        );
+      });
+    });
+
     it('assumes insertion effect destroy function is either a function or undefined', async () => {
       function App(props) {
         useInsertionEffect(() => {

diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js
@@ -172,6 +172,11 @@ export const transitionLaneExpirationMs = 5000;
 // Renames the internal symbol for elements since they have changed signature/constructor
 export const renameElementSymbol = true;
 
+/**
+ * Enables a fix to run insertion effect cleanup on hidden subtrees.
+ */
+export const enableHiddenSubtreeInsertionEffectCleanup = true;
+
 /**
  * Removes legacy style context defined using static `contextTypes` and consumed with static `childContextTypes`.
  */

diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js b/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js
@@ -20,6 +20,7 @@
 export const alwaysThrottleRetries = __VARIANT__;
 export const enableAddPropertiesFastPath = __VARIANT__;
 export const enableObjectFiber = __VARIANT__;
+export const enableHiddenSubtreeInsertionEffectCleanup = __VARIANT__;
 export const enablePersistedModeClonedFlag = __VARIANT__;
 export const enableShallowPropDiffing = __VARIANT__;
 export const passChildrenWhenCloningPersistedNodes = __VARIANT__;

diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js
@@ -22,6 +22,7 @@ export const {
   alwaysThrottleRetries,
   enableAddPropertiesFastPath,
   enableFabricCompleteRootInCommitPhase,
+  enableHiddenSubtreeInsertionEffectCleanup,
   enableObjectFiber,
   enablePersistedModeClonedFlag,
   enableShallowPropDiffing,

diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js
@@ -50,6 +50,7 @@ export const enableFizzExternalRuntime = true;
 export const enableFlightReadableStream = true;
 export const enableGetInspectorDataForInstanceInProduction = false;
 export const enableHalt = false;
+export const enableHiddenSubtreeInsertionEffectCleanup = false;
 export const enableInfiniteRenderLoopDetection = true;
 export const enableLazyContextPropagation = false;
 export const enableContextProfiling = false;

diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js
@@ -46,6 +46,7 @@ export const enableLegacyFBSupport = false;
 export const enableFilterEmptyStringAttributesDOM = true;
 export const enableGetInspectorDataForInstanceInProduction = false;
 export const enableFabricCompleteRootInCommitPhase = false;
+export const enableHiddenSubtreeInsertionEffectCleanup = false;
 
 export const enableRetryLaneExpiration = false;
 export const retryLaneExpirationMs = 5000;

diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js
@@ -44,6 +44,7 @@ export const enableHalt = false;
 export const enableInfiniteRenderLoopDetection = true;
 export const enableLazyContextPropagation = false;
 export const enableContextProfiling = false;
+export const enableHiddenSubtreeInsertionEffectCleanup = true;
 export const enableLegacyCache = false;
 export const enableLegacyFBSupport = false;
 export const enableLegacyHidden = false;

diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
@@ -49,6 +49,7 @@ export const enableFilterEmptyStringAttributesDOM = true;
 export const enableGetInspectorDataForInstanceInProduction = false;
 export const enableRenderableContext = false;
 export const enableFabricCompleteRootInCommitPhase = false;
+export const enableHiddenSubtreeInsertionEffectCleanup = true;
 
 export const enableRetryLaneExpiration = false;
 export const retryLaneExpirationMs = 5000;

diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js
@@ -21,6 +21,7 @@ export const disableSchedulerTimeoutInWorkLoop = __VARIANT__;
 export const enableAddPropertiesFastPath = __VARIANT__;
 export const enableDeferRootSchedulingToMicrotask = __VARIANT__;
 export const enableDO_NOT_USE_disableStrictPassiveEffect = __VARIANT__;
+export const enableHiddenSubtreeInsertionEffectCleanup = __VARIANT__;
 export const enableNoCloningMemoCache = __VARIANT__;
 export const enableObjectFiber = __VARIANT__;
 export const enableRenderableContext = __VARIANT__;

diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js
@@ -30,6 +30,7 @@ export const {
   enableRetryLaneExpiration,
   enableTransitionTracing,
   enableTrustedTypesIntegration,
+  enableHiddenSubtreeInsertionEffectCleanup,
   favorSafetyOverHydrationPerf,
   renameElementSymbol,
   retryLaneExpirationMs,