From 91bf47ceecc6d4f08890cd980be1d0da230905df Mon Sep 17 00:00:00 2001 From: Jan Kassens Date: Thu, 12 Sep 2024 17:53:32 -0700 Subject: [PATCH 1/7] 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, From ce113766eea8b191d552c4eabbbf5e6c09c3a36d Mon Sep 17 00:00:00 2001 From: Rick Hanlon Date: Fri, 13 Sep 2024 13:26:33 -0400 Subject: [PATCH 2/7] Add tests, add unmount setState error --- .../src/ReactFiberCommitWork.js | 77 ++++++++++- .../src/__tests__/Activity-test.js | 84 ++++++++++++ .../ReactHooksWithNoopRenderer-test.js | 76 ++++++----- .../ReactSuspenseEffectsSemantics-test.js | 129 +++++++++++++++++- packages/shared/ReactFeatureFlags.js | 2 +- 5 files changed, 327 insertions(+), 41 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 0d2b85aca04ef..17c104ff0fff9 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -148,6 +148,7 @@ import { getExecutionContext, CommitContext, NoContext, + setIsRunningInsertionEffect, } from './ReactFiberWorkLoop'; import { NoFlags as NoHookEffect, @@ -1325,10 +1326,78 @@ function commitDeletionEffectsOnFiber( case ForwardRef: case MemoComponent: case SimpleMemoComponent: { - if ( - enableHiddenSubtreeInsertionEffectCleanup || - !offscreenSubtreeWasHidden - ) { + if (enableHiddenSubtreeInsertionEffectCleanup) { + // When deleting a fiber, we may need to destroy insertion or layout effects. + // Insertion effects are not destroyed on hidden, only when destroyed, so now + // we need to destroy them. Layout effects are destroyed when hidden, so + // we only need to destroy them if the tree is visible. + 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) { + // TODO: add insertion effect marks and profiling. + if (__DEV__) { + setIsRunningInsertionEffect(true); + } + + inst.destroy = undefined; + safelyCallDestroy( + deletedFiber, + nearestMountedAncestor, + destroy, + ); + + if (__DEV__) { + setIsRunningInsertionEffect(false); + } + } else if ( + !offscreenSubtreeWasHidden && + (tag & HookLayout) !== NoHookEffect + ) { + // Offscreen fibers already unmounted their layout effects. + // We only need to destroy layout effects for visible trees. + 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, + ); + } + + if (enableSchedulingProfiler) { + markComponentLayoutEffectUnmountStopped(); + } + } + } + effect = effect.next; + } while (effect !== firstEffect); + } + } + } else if (!offscreenSubtreeWasHidden) { const updateQueue: FunctionComponentUpdateQueue | null = (deletedFiber.updateQueue: any); if (updateQueue !== null) { diff --git a/packages/react-reconciler/src/__tests__/Activity-test.js b/packages/react-reconciler/src/__tests__/Activity-test.js index e174c7c06ca76..10532899bd3ba 100644 --- a/packages/react-reconciler/src/__tests__/Activity-test.js +++ b/packages/react-reconciler/src/__tests__/Activity-test.js @@ -25,6 +25,7 @@ describe('Activity', () => { LegacyHidden = React.unstable_LegacyHidden; Activity = React.unstable_Activity; useState = React.useState; + useInsertionEffect = React.useInsertionEffect; useLayoutEffect = React.useLayoutEffect; useEffect = React.useEffect; useMemo = React.useMemo; @@ -43,6 +44,13 @@ describe('Activity', () => { } function LoggedText({text, children}) { + useInsertionEffect(() => { + Scheduler.log(`mount insertion ${text}`); + return () => { + Scheduler.log(`unmount insertion ${text}`); + }; + }); + useEffect(() => { Scheduler.log(`mount ${text}`); return () => { @@ -1436,6 +1444,63 @@ describe('Activity', () => { ); }); + // @gate enableActivity + it('insertion effects are not disconnected when the visibility changes', async () => { + function Child({step}) { + useInsertionEffect(() => { + Scheduler.log(`Commit mount [${step}]`); + return () => { + Scheduler.log(`Commit unmount [${step}]`); + }; + }, [step]); + return ; + } + + function App({show, step}) { + return ( + + {useMemo( + () => ( + + ), + [step], + )} + + ); + } + + const root = ReactNoop.createRoot(); + await act(() => { + root.render(); + }); + assertLog([1, 'Commit mount [1]']); + expect(root).toMatchRenderedOutput(); + + // Hide the tree. This will unmount the effect. + await act(() => { + root.render(); + }); + assertLog([]); + expect(root).toMatchRenderedOutput(