From 91bf47ceecc6d4f08890cd980be1d0da230905df Mon Sep 17 00:00:00 2001 From: Jan Kassens Date: Thu, 12 Sep 2024 17:53:32 -0700 Subject: [PATCH] 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 https://github.com/facebook/react/issues/26670 --- .../src/ReactFiberCommitWork.js | 6 +++- .../ReactHooksWithNoopRenderer-test.js | 32 +++++++++++++++++++ packages/shared/ReactFeatureFlags.js | 5 +++ .../ReactFeatureFlags.native-fb-dynamic.js | 1 + .../forks/ReactFeatureFlags.native-fb.js | 1 + .../forks/ReactFeatureFlags.native-oss.js | 1 + .../forks/ReactFeatureFlags.test-renderer.js | 1 + ...actFeatureFlags.test-renderer.native-fb.js | 1 + .../ReactFeatureFlags.test-renderer.www.js | 1 + .../forks/ReactFeatureFlags.www-dynamic.js | 1 + .../shared/forks/ReactFeatureFlags.www.js | 1 + 11 files changed, 50 insertions(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 6d600971d4461..0d2b85aca04ef 100644 --- 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 index 0fc64b39da899..4106655340b1f 100644 --- 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( + + + , + ); + 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 index a9ff23eb8d28f..80536580036e4 100644 --- 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 index 2d49c56377a2f..2cae164dfdea5 100644 --- 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 index 61138ddbee6c1..a6001a9559609 100644 --- 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 index 4e50442c2579f..723cc06c33a27 100644 --- 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 index 53a662cffeb1a..3581d31831784 100644 --- 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 index ebc3ddab2a551..edf67adf18bc3 100644 --- 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 index a378ab3edf17c..e64904005e143 100644 --- 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 index d6e11f92649ba..dbd00f671d84e 100644 --- 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 index e207dbb71b669..e6025242d4a78 100644 --- 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,