diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index e5073c31def17..4234732bc7a72 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -592,9 +592,10 @@ function commitHookEffectListUnmount( do { if ((effect.tag & flags) === flags) { // Unmount - const destroy = effect.destroy; - effect.destroy = undefined; + const inst = effect.inst; + const destroy = inst.destroy; if (destroy !== undefined) { + inst.destroy = undefined; if (enableSchedulingProfiler) { if ((flags & HookPassive) !== NoHookEffect) { markComponentPassiveEffectUnmountStarted(finishedWork); @@ -653,7 +654,9 @@ function commitHookEffectListMount(flags: HookFlags, finishedWork: Fiber) { setIsRunningInsertionEffect(true); } } - effect.destroy = create(); + const inst = effect.inst; + const destroy = create(); + inst.destroy = destroy; if (__DEV__) { if ((flags & HookInsertion) !== NoHookEffect) { setIsRunningInsertionEffect(false); @@ -669,7 +672,6 @@ function commitHookEffectListMount(flags: HookFlags, finishedWork: Fiber) { } if (__DEV__) { - const destroy = effect.destroy; if (destroy !== undefined && typeof destroy !== 'function') { let hookName; if ((effect.tag & HookLayout) !== NoFlags) { @@ -2188,9 +2190,12 @@ function commitDeletionEffectsOnFiber( let effect = firstEffect; do { - const {destroy, tag} = effect; + 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, @@ -2203,6 +2208,7 @@ function commitDeletionEffectsOnFiber( if (shouldProfile(deletedFiber)) { startLayoutEffectTimer(); + inst.destroy = undefined; safelyCallDestroy( deletedFiber, nearestMountedAncestor, @@ -2210,6 +2216,7 @@ function commitDeletionEffectsOnFiber( ); recordLayoutEffectDuration(deletedFiber); } else { + inst.destroy = undefined; safelyCallDestroy( deletedFiber, nearestMountedAncestor, diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 4c05ab5a5ac7b..369a662cb61b1 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -180,11 +180,29 @@ export type Hook = { next: Hook | null, }; +// The effect "instance" is a shared object that remains the same for the entire +// lifetime of an effect. In Rust terms, a RefCell. We use it to store the +// "destroy" function that is returned from an effect, because that is stateful. +// The field is `undefined` if the effect is unmounted, or if the effect ran +// but is not stateful. We don't explicitly track whether the effect is mounted +// or unmounted because that can be inferred by the hiddenness of the fiber in +// the tree, i.e. whether there is a hidden Offscreen fiber above it. +// +// It's unfortunate that this is stored on a separate object, because it adds +// more memory per effect instance, but it's conceptually sound. I think there's +// likely a better data structure we could use for effects; perhaps just one +// array of effect instances per fiber. But I think this is OK for now despite +// the additional memory and we can follow up with performance +// optimizations later. +type EffectInstance = { + destroy: void | (() => void), +}; + export type Effect = { tag: HookFlags, create: () => (() => void) | void, - destroy: (() => void) | void, - deps: Array | void | null, + inst: EffectInstance, + deps: Array | null, next: Effect, }; @@ -1662,7 +1680,7 @@ function mountSyncExternalStore( pushEffect( HookHasEffect | HookPassive, updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot), - undefined, + createEffectInstance(), null, ); @@ -1719,7 +1737,7 @@ function updateSyncExternalStore( pushEffect( HookHasEffect | HookPassive, updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot), - undefined, + createEffectInstance(), null, ); @@ -1860,13 +1878,13 @@ function rerenderState( function pushEffect( tag: HookFlags, create: () => (() => void) | void, - destroy: (() => void) | void, - deps: Array | void | null, + inst: EffectInstance, + deps: Array | null, ): Effect { const effect: Effect = { tag, create, - destroy, + inst, deps, // Circular next: (null: any), @@ -1891,6 +1909,10 @@ function pushEffect( return effect; } +function createEffectInstance(): EffectInstance { + return {destroy: undefined}; +} + let stackContainsErrorMessage: boolean | null = null; function getCallerStackFrame(): string { @@ -1994,7 +2016,7 @@ function mountEffectImpl( hook.memoizedState = pushEffect( HookHasEffect | hookFlags, create, - undefined, + createEffectInstance(), nextDeps, ); } @@ -2007,16 +2029,16 @@ function updateEffectImpl( ): void { const hook = updateWorkInProgressHook(); const nextDeps = deps === undefined ? null : deps; - let destroy = undefined; + const effect: Effect = hook.memoizedState; + const inst = effect.inst; // currentHook is null when rerendering after a render phase state update. if (currentHook !== null) { - const prevEffect = currentHook.memoizedState; - destroy = prevEffect.destroy; if (nextDeps !== null) { + const prevEffect: Effect = currentHook.memoizedState; const prevDeps = prevEffect.deps; if (areHookInputsEqual(nextDeps, prevDeps)) { - hook.memoizedState = pushEffect(hookFlags, create, destroy, nextDeps); + hook.memoizedState = pushEffect(hookFlags, create, inst, nextDeps); return; } } @@ -2027,7 +2049,7 @@ function updateEffectImpl( hook.memoizedState = pushEffect( HookHasEffect | hookFlags, create, - destroy, + inst, nextDeps, ); } diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js index 8988cbd692de3..12414e9e20509 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js @@ -496,4 +496,82 @@ describe('ReactSuspenseEffectsSemanticsDOM', () => { ReactDOM.render(null, container); assertLog(['Unmount']); }); + + it('does not call cleanup effects twice after a bailout', async () => { + const never = new Promise(resolve => {}); + function Never() { + throw never; + } + + let setSuspended; + let setLetter; + + function App() { + const [suspended, _setSuspended] = React.useState(false); + setSuspended = _setSuspended; + const [letter, _setLetter] = React.useState('A'); + setLetter = _setLetter; + + return ( + + + {suspended && } + + ); + } + + let nextId = 0; + const freed = new Set(); + let setStep; + + function Child({letter}) { + const [, _setStep] = React.useState(0); + setStep = _setStep; + + React.useLayoutEffect(() => { + const localId = nextId++; + Scheduler.log('Did mount: ' + letter + localId); + return () => { + if (freed.has(localId)) { + throw Error('Double free: ' + letter + localId); + } + freed.add(localId); + Scheduler.log('Will unmount: ' + letter + localId); + }; + }, [letter]); + } + + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render(); + }); + assertLog(['Did mount: A0']); + + await act(() => { + setStep(1); + setSuspended(false); + }); + assertLog([]); + + await act(() => { + setStep(1); + }); + assertLog([]); + + await act(() => { + setSuspended(true); + }); + assertLog(['Will unmount: A0']); + + await act(() => { + setSuspended(false); + setLetter('B'); + }); + assertLog(['Did mount: B1']); + + await act(() => { + root.unmount(); + }); + assertLog(['Will unmount: B1']); + }); });