diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 43db744007d..08882a04766 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -47,6 +47,7 @@ import type {ViewTransitionState} from './ReactFiberViewTransitionComponent'; import { alwaysThrottleRetries, enableCreateEventHandleAPI, + enableEffectEventMutationPhase, enableHiddenSubtreeInsertionEffectCleanup, enableProfilerTimer, enableProfilerCommitHooks, @@ -499,7 +500,7 @@ function commitBeforeMutationEffectsOnFiber( case FunctionComponent: case ForwardRef: case SimpleMemoComponent: { - if ((flags & Update) !== NoFlags) { + if (!enableEffectEventMutationPhase && (flags & Update) !== NoFlags) { const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any); const eventPayloads = updateQueue !== null ? updateQueue.events : null; @@ -2042,6 +2043,24 @@ function commitMutationEffectsOnFiber( case ForwardRef: case MemoComponent: case SimpleMemoComponent: { + // Mutate event effect callbacks on the way down, before mutation effects. + // This ensures that parent event effects are mutated before child effects. + // This isn't a supported use case, so we can re-consider it, + // but this was the behavior we originally shipped. + if (enableEffectEventMutationPhase) { + if (flags & Update) { + const updateQueue: FunctionComponentUpdateQueue | null = + (finishedWork.updateQueue: any); + const eventPayloads = + updateQueue !== null ? updateQueue.events : null; + if (eventPayloads !== null) { + for (let ii = 0; ii < eventPayloads.length; ii++) { + const {ref, nextImpl} = eventPayloads[ii]; + ref.impl = nextImpl; + } + } + } + } recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork, lanes); diff --git a/packages/react-reconciler/src/ReactFiberFlags.js b/packages/react-reconciler/src/ReactFiberFlags.js index 2d8d3f7fd16..9f85897fb05 100644 --- a/packages/react-reconciler/src/ReactFiberFlags.js +++ b/packages/react-reconciler/src/ReactFiberFlags.js @@ -7,7 +7,10 @@ * @flow */ -import {enableCreateEventHandleAPI} from 'shared/ReactFeatureFlags'; +import { + enableCreateEventHandleAPI, + enableEffectEventMutationPhase, +} from 'shared/ReactFeatureFlags'; export type Flags = number; @@ -99,10 +102,11 @@ export const BeforeMutationMask: number = // TODO: Only need to visit Deletions during BeforeMutation phase if an // element is focused. Update | ChildDeletion | Visibility - : // TODO: The useEffectEvent hook uses the snapshot phase for clean up but it - // really should use the mutation phase for this or at least schedule an - // explicit Snapshot phase flag for this. - Update); + : // useEffectEvent uses the snapshot phase, + // but we're moving it to the mutation phase. + enableEffectEventMutationPhase + ? 0 + : Update); // For View Transition support we use the snapshot phase to scan the tree for potentially // affected ViewTransition components. diff --git a/packages/react-reconciler/src/__tests__/useEffectEvent-test.js b/packages/react-reconciler/src/__tests__/useEffectEvent-test.js index 2b5af2206b9..78379360ed3 100644 --- a/packages/react-reconciler/src/__tests__/useEffectEvent-test.js +++ b/packages/react-reconciler/src/__tests__/useEffectEvent-test.js @@ -27,6 +27,7 @@ describe('useEffectEvent', () => { let waitForAll; let assertLog; let waitForThrow; + let waitFor; beforeEach(() => { React = require('react'); @@ -46,6 +47,7 @@ describe('useEffectEvent', () => { waitForAll = InternalTestUtils.waitForAll; assertLog = InternalTestUtils.assertLog; waitForThrow = InternalTestUtils.waitForThrow; + waitFor = InternalTestUtils.waitFor; }); function Text(props) { @@ -595,6 +597,358 @@ describe('useEffectEvent', () => { assertLog(['Effect value: 2', 'Event value: 2']); }); + it('fires all (interleaved) effects with useEffectEvent in correct order', async () => { + function CounterA({count}) { + const onEvent = useEffectEvent(() => { + return `A ${count}`; + }); + + useInsertionEffect(() => { + // Call the event function to verify it sees the latest value + Scheduler.log(`Parent Insertion Create: ${onEvent()}`); + return () => { + Scheduler.log(`Parent Insertion Create: ${onEvent()}`); + }; + }); + + useLayoutEffect(() => { + Scheduler.log(`Parent Layout Create: ${onEvent()}`); + return () => { + Scheduler.log(`Parent Layout Cleanup: ${onEvent()}`); + }; + }); + + useEffect(() => { + Scheduler.log(`Parent Passive Create: ${onEvent()}`); + return () => { + Scheduler.log(`Parent Passive Destroy ${onEvent()}`); + }; + }); + + // this breaks the rules, but ensures the ordering is correct. + return ; + } + + function CounterB({count, onEventParent}) { + const onEvent = useEffectEvent(() => { + return `${onEventParent()} B ${count}`; + }); + + useInsertionEffect(() => { + Scheduler.log(`Child Insertion Create ${onEvent()}`); + return () => { + Scheduler.log(`Child Insertion Destroy ${onEvent()}`); + }; + }); + + useLayoutEffect(() => { + Scheduler.log(`Child Layout Create ${onEvent()}`); + return () => { + Scheduler.log(`Child Layout Destroy ${onEvent()}`); + }; + }); + + useEffect(() => { + Scheduler.log(`Child Passive Create ${onEvent()}`); + return () => { + Scheduler.log(`Child Passive Destroy ${onEvent()}`); + }; + }); + + return null; + } + + await act(async () => { + ReactNoop.render(); + }); + + assertLog([ + 'Child Insertion Create A 1 B 1', + 'Parent Insertion Create: A 1', + 'Child Layout Create A 1 B 1', + 'Parent Layout Create: A 1', + 'Child Passive Create A 1 B 1', + 'Parent Passive Create: A 1', + ]); + + await act(async () => { + ReactNoop.render(); + }); + + assertLog([ + 'Child Insertion Destroy A 2 B 2', + 'Child Insertion Create A 2 B 2', + 'Child Layout Destroy A 2 B 2', + 'Parent Insertion Create: A 2', + 'Parent Insertion Create: A 2', + 'Parent Layout Cleanup: A 2', + 'Child Layout Create A 2 B 2', + 'Parent Layout Create: A 2', + 'Child Passive Destroy A 2 B 2', + 'Parent Passive Destroy A 2', + 'Child Passive Create A 2 B 2', + 'Parent Passive Create: A 2', + ]); + + // Unmount everything + await act(async () => { + ReactNoop.render(null); + }); + + assertLog([ + 'Parent Insertion Create: A 2', + 'Parent Layout Cleanup: A 2', + 'Child Insertion Destroy A 2 B 2', + 'Child Layout Destroy A 2 B 2', + 'Parent Passive Destroy A 2', + 'Child Passive Destroy A 2 B 2', + ]); + }); + + it('correctly mutates effect event with Activity', async () => { + let setState; + let setChildState; + function CounterA({count, hideChild}) { + const [state, _setState] = useState(1); + setState = _setState; + const onEvent = useEffectEvent(() => { + return `A ${count} ${state}`; + }); + + useInsertionEffect(() => { + // Call the event function to verify it sees the latest value + Scheduler.log(`Parent Insertion Create: ${onEvent()}`); + return () => { + Scheduler.log(`Parent Insertion Create: ${onEvent()}`); + }; + }); + + useLayoutEffect(() => { + Scheduler.log(`Parent Layout Create: ${onEvent()}`); + return () => { + Scheduler.log(`Parent Layout Cleanup: ${onEvent()}`); + }; + }); + + // this breaks the rules, but ensures the ordering is correct. + return ( + + + + ); + } + + function CounterB({count, state, onEventParent}) { + const [childState, _setChildState] = useState(1); + setChildState = _setChildState; + const onEvent = useEffectEvent(() => { + return `${onEventParent()} B ${count} ${state} ${childState}`; + }); + + useInsertionEffect(() => { + Scheduler.log(`Child Insertion Create ${onEvent()}`); + return () => { + Scheduler.log(`Child Insertion Destroy ${onEvent()}`); + }; + }); + + useLayoutEffect(() => { + Scheduler.log(`Child Layout Create ${onEvent()}`); + return () => { + Scheduler.log(`Child Layout Destroy ${onEvent()}`); + }; + }); + + useEffect(() => { + Scheduler.log(`Child Passive Create ${onEvent()}`); + return () => { + Scheduler.log(`Child Passive Destroy ${onEvent()}`); + }; + }); + + return null; + } + + await act(async () => { + ReactNoop.render(); + await waitFor([ + 'Parent Insertion Create: A 1 1', + 'Parent Layout Create: A 1 1', + 'Child Insertion Create A 1 1 B 1 1 1', + ]); + }); + + assertLog([]); + + await act(async () => { + ReactNoop.render(); + + await waitFor([ + 'Parent Insertion Create: A 2 1', + 'Parent Insertion Create: A 2 1', + 'Parent Layout Cleanup: A 2 1', + 'Parent Layout Create: A 2 1', + ...(gate('enableViewTransition') && + !gate('enableEffectEventMutationPhase') + ? [ + 'Child Insertion Destroy A 2 1 B 1 1 1', + 'Child Insertion Create A 2 1 B 1 1 1', + ] + : [ + 'Child Insertion Destroy A 2 1 B 2 1 1', + 'Child Insertion Create A 2 1 B 2 1 1', + ]), + ]); + }); + + assertLog([]); + + await act(async () => { + setState(2); + + await waitFor([ + 'Parent Insertion Create: A 2 2', + 'Parent Insertion Create: A 2 2', + 'Parent Layout Cleanup: A 2 2', + 'Parent Layout Create: A 2 2', + ...(gate('enableViewTransition') && + !gate('enableEffectEventMutationPhase') + ? [ + 'Child Insertion Destroy A 2 2 B 1 1 1', + 'Child Insertion Create A 2 2 B 1 1 1', + ] + : [ + 'Child Insertion Destroy A 2 2 B 2 2 1', + 'Child Insertion Create A 2 2 B 2 2 1', + ]), + ]); + }); + + assertLog([]); + + await act(async () => { + setChildState(2); + + await waitFor( + gate('enableViewTransition') && !gate('enableEffectEventMutationPhase') + ? [ + 'Child Insertion Destroy A 2 2 B 1 1 1', + 'Child Insertion Create A 2 2 B 1 1 1', + ] + : [ + 'Child Insertion Destroy A 2 2 B 2 2 2', + 'Child Insertion Create A 2 2 B 2 2 2', + ], + ); + }); + + assertLog([]); + + await act(async () => { + ReactNoop.render(); + + await waitFor([ + 'Parent Insertion Create: A 3 2', + 'Parent Insertion Create: A 3 2', + 'Parent Layout Cleanup: A 3 2', + 'Parent Layout Create: A 3 2', + ]); + }); + + assertLog( + gate('enableViewTransition') && !gate('enableEffectEventMutationPhase') + ? [ + 'Child Insertion Destroy A 3 2 B 1 1 1', + 'Child Insertion Create A 3 2 B 1 1 1', + ] + : [ + 'Child Insertion Destroy A 3 2 B 3 2 2', + 'Child Insertion Create A 3 2 B 3 2 2', + ], + ); + + await act(async () => { + ReactNoop.render(); + + await waitFor([ + ...(gate('enableViewTransition') && + !gate('enableEffectEventMutationPhase') + ? [ + 'Child Insertion Destroy A 3 2 B 1 1 1', + 'Child Insertion Create A 3 2 B 1 1 1', + ] + : [ + 'Child Insertion Destroy A 3 2 B 3 2 2', + 'Child Insertion Create A 3 2 B 3 2 2', + ]), + 'Parent Insertion Create: A 3 2', + 'Parent Insertion Create: A 3 2', + 'Parent Layout Cleanup: A 3 2', + ...(gate('enableViewTransition') && + !gate('enableEffectEventMutationPhase') + ? ['Child Layout Create A 3 2 B 1 1 1'] + : ['Child Layout Create A 3 2 B 3 2 2']), + + 'Parent Layout Create: A 3 2', + ]); + }); + + assertLog( + gate('enableViewTransition') && !gate('enableEffectEventMutationPhase') + ? ['Child Passive Create A 3 2 B 1 1 1'] + : ['Child Passive Create A 3 2 B 3 2 2'], + ); + + await act(async () => { + ReactNoop.render(); + + await waitFor([ + ...(gate('enableViewTransition') && + !gate('enableEffectEventMutationPhase') + ? ['Child Layout Destroy A 3 2 B 1 1 1'] + : ['Child Layout Destroy A 3 2 B 3 2 2']), + 'Parent Insertion Create: A 3 2', + 'Parent Insertion Create: A 3 2', + 'Parent Layout Cleanup: A 3 2', + 'Parent Layout Create: A 3 2', + ...(gate('enableViewTransition') && + !gate('enableEffectEventMutationPhase') + ? ['Child Passive Destroy A 3 2 B 1 1 1'] + : ['Child Passive Destroy A 3 2 B 3 2 2']), + ]); + }); + + assertLog( + gate('enableViewTransition') && !gate('enableEffectEventMutationPhase') + ? [ + 'Child Insertion Destroy A 3 2 B 1 1 1', + 'Child Insertion Create A 3 2 B 1 1 1', + ] + : [ + 'Child Insertion Destroy A 3 2 B 3 2 2', + 'Child Insertion Create A 3 2 B 3 2 2', + ], + ); + + // Unmount everything + await act(async () => { + ReactNoop.render(null); + }); + + assertLog([ + 'Parent Insertion Create: A 3 2', + 'Parent Layout Cleanup: A 3 2', + ...(gate('enableHiddenSubtreeInsertionEffectCleanup') + ? [ + gate('enableViewTransition') && + !gate('enableEffectEventMutationPhase') + ? 'Child Insertion Destroy A 3 2 B 1 1 1' + : 'Child Insertion Destroy A 3 2 B 3 2 2', + ] + : []), + ]); + }); + it("doesn't provide a stable identity", async () => { function Counter({shouldRender, value}) { const onClick = useEffectEvent(() => { @@ -916,4 +1270,56 @@ describe('useEffectEvent', () => { logContextValue(); assertLog(['ContextReader (Effect event): second']); }); + + // @gate enableActivity + it('effect events are fresh in deeply nested hidden Activities', async () => { + function Child({value}) { + const logValue = useEffectEvent(() => { + Scheduler.log('effect event: ' + value); + }); + useInsertionEffect(() => { + logValue(); + return () => { + logValue(); + }; + }); + Scheduler.log('render: ' + value); + return null; + } + + function App({value}) { + return ( + + + + + + + + ); + } + + const root = ReactNoop.createRoot(); + + // Initial mount with value=1 + await act(() => root.render()); + assertLog([ + 'render: 1', + 'effect event: 1', // Insertion effect mount should see fresh value + ]); + + // Update to value=2 + await act(() => root.render()); + + // Bug in enableViewTransition. + assertLog([ + 'render: 2', + gate('enableViewTransition') && !gate('enableEffectEventMutationPhase') + ? 'effect event: 1' + : 'effect event: 2', + gate('enableViewTransition') && !gate('enableEffectEventMutationPhase') + ? 'effect event: 1' + : 'effect event: 2', + ]); + }); }); diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 587e4e6a1da..3e68d8c8991 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -125,6 +125,10 @@ export const enableFizzExternalRuntime = __EXPERIMENTAL__; export const alwaysThrottleRetries: boolean = true; +// Gate whether useEffectEvent uses the mutation phase (true) or before-mutation +// phase (false) for updating event function references. +export const enableEffectEventMutationPhase: boolean = false; + export const passChildrenWhenCloningPersistedNodes: boolean = false; export const enableEagerAlternateStateNodeCleanup: boolean = true; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js b/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js index f139a3fcf28..389d46be497 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js @@ -27,3 +27,4 @@ export const enableFragmentRefs = __VARIANT__; export const enableFragmentRefsScrollIntoView = __VARIANT__; export const enableFragmentRefsInstanceHandles = __VARIANT__; export const enableComponentPerformanceTrack = __VARIANT__; +export const enableEffectEventMutationPhase = __VARIANT__; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index e2674b70fee..cb0b0b66f60 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -20,6 +20,7 @@ const dynamicFlags: DynamicExportsType = (dynamicFlagsUntyped: any); // the exports object every time a flag is read. export const { alwaysThrottleRetries, + enableEffectEventMutationPhase, enableHiddenSubtreeInsertionEffectCleanup, enableObjectFiber, enableEagerAlternateStateNodeCleanup, diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index 672c3755108..f1e697b7b21 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -46,6 +46,7 @@ export const enableSchedulingProfiler: boolean = !enableComponentPerformanceTrack && __PROFILE__; export const enableScopeAPI: boolean = false; export const enableEagerAlternateStateNodeCleanup: boolean = true; +export const enableEffectEventMutationPhase: boolean = false; export const enableSuspenseAvoidThisFallback: boolean = false; export const enableSuspenseCallback: boolean = false; export const enableTaint: boolean = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 67c08d94367..8e65aea17b2 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -58,6 +58,7 @@ export const enableInfiniteRenderLoopDetection: boolean = false; export const renameElementSymbol: boolean = true; export const enableEagerAlternateStateNodeCleanup: boolean = true; +export const enableEffectEventMutationPhase: boolean = false; export const enableYieldingBeforePassive: boolean = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js index a0ee01baa7d..120a20160f8 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js @@ -43,6 +43,7 @@ export const enableComponentPerformanceTrack = false; export const enablePerformanceIssueReporting = false; export const enableScopeAPI = false; export const enableEagerAlternateStateNodeCleanup = true; +export const enableEffectEventMutationPhase = false; export const enableSuspenseAvoidThisFallback = false; export const enableSuspenseCallback = false; export const enableTaint = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index 058fd752d6e..dee7642449a 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -64,6 +64,7 @@ export const renameElementSymbol: boolean = false; export const enableObjectFiber: boolean = false; export const enableEagerAlternateStateNodeCleanup: boolean = true; +export const enableEffectEventMutationPhase: boolean = false; export const enableHydrationLaneScheduling: boolean = true; diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js index 1d174609cb9..206e9ec3a36 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -40,6 +40,8 @@ export const enableAsyncDebugInfo: boolean = __VARIANT__; export const enableInternalInstanceMap: boolean = __VARIANT__; +export const enableEffectEventMutationPhase: boolean = __VARIANT__; + // TODO: These flags are hard-coded to the default values used in open source. // Update the tests so that they pass in either mode, then set these // to __VARIANT__. diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 0752f266afc..1db222df0bc 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -18,6 +18,7 @@ export const { alwaysThrottleRetries, disableLegacyContextForFunctionComponents, disableSchedulerTimeoutInWorkLoop, + enableEffectEventMutationPhase, enableHiddenSubtreeInsertionEffectCleanup, enableInfiniteRenderLoopDetection, enableNoCloningMemoCache,