[flags] add enableEffectEventMutationPhase (#35548)

Small optimization for useEffectEvent. Not sure we even need a flag for
it, but it will be a nice killswitch.

As an added benefit, it fixes a bug when `enableViewTransition` is on,
where we were not updating the useEffectEvent callback when a tree went
from hidden to visible.

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);
 

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.

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 <CounterB count={count} onEventParent={onEvent} />;
+    }
+
+    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(<CounterA count={1} />);
+    });
+
+    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(<CounterA count={2} />);
+    });
+
+    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 (
+        <React.Activity mode={hideChild ? 'hidden' : 'visible'}>
+          <CounterB count={count} state={state} onEventParent={onEvent} />
+        </React.Activity>
+      );
+    }
+
+    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(<CounterA count={1} hideChild={true} />);
+      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(<CounterA count={2} hideChild={true} />);
+
+      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(<CounterA count={3} hideChild={true} />);
+
+      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(<CounterA count={3} hideChild={false} />);
+
+      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(<CounterA count={3} hideChild={true} />);
+
+      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,66 @@ describe('useEffectEvent', () => {
     logContextValue();
     assertLog(['ContextReader (Effect event): second']);
   });
+
+  // @gate enableActivity
+  it('effect events are fresh inside Activity', async () => {
+    function Child({value}) {
+      const getValue = useEffectEvent(() => {
+        return value;
+      });
+      useInsertionEffect(() => {
+        Scheduler.log('insertion create: ' + getValue());
+        return () => {
+          Scheduler.log('insertion destroy: ' + getValue());
+        };
+      });
+      useLayoutEffect(() => {
+        Scheduler.log('layout create: ' + getValue());
+        return () => {
+          Scheduler.log('layout destroy: ' + getValue());
+        };
+      });
+
+      Scheduler.log('render: ' + value);
+      return null;
+    }
+
+    function App({value, mode}) {
+      return (
+        <React.Activity mode={mode}>
+          <Child value={value} />
+        </React.Activity>
+      );
+    }
+
+    const root = ReactNoop.createRoot();
+
+    // Mount hidden
+    await act(async () => root.render(<App value={1} mode="hidden" />));
+    assertLog(['render: 1', 'insertion create: 1']);
+
+    // Update, still hidden
+    await act(async () => root.render(<App value={2} mode="hidden" />));
+
+    // Bug in enableViewTransition. Insertion and layout see stale closure.
+    assertLog([
+      'render: 2',
+      ...(gate('enableViewTransition') &&
+      !gate('enableEffectEventMutationPhase')
+        ? ['insertion destroy: 1', 'insertion create: 1']
+        : ['insertion destroy: 2', 'insertion create: 2']),
+    ]);
+
+    // Switch to visible
+    await act(async () => root.render(<App value={2} mode="visible" />));
+
+    // Bug in enableViewTransition. Even when switching to visible, sees stale closure.
+    assertLog([
+      'render: 2',
+      ...(gate('enableViewTransition') &&
+      !gate('enableEffectEventMutationPhase')
+        ? ['insertion destroy: 1', 'insertion create: 1', 'layout create: 1']
+        : ['insertion destroy: 2', 'insertion create: 2', 'layout create: 2']),
+    ]);
+  });
 });

packages/shared/ReactFeatureFlags.js

@@ -123,6 +123,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;

packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js

@@ -25,4 +25,5 @@ export const passChildrenWhenCloningPersistedNodes = __VARIANT__;
 export const enableFragmentRefs = __VARIANT__;
 export const enableFragmentRefsScrollIntoView = __VARIANT__;
 export const enableFragmentRefsInstanceHandles = __VARIANT__;
+export const enableEffectEventMutationPhase = __VARIANT__;
 export const enableFragmentRefsTextNodes = __VARIANT__;

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,

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;

packages/shared/forks/ReactFeatureFlags.test-renderer.js

@@ -56,6 +56,7 @@ export const disableClientCache: boolean = true;
 export const enableInfiniteRenderLoopDetection: boolean = false;
 
 export const enableEagerAlternateStateNodeCleanup: boolean = true;
+export const enableEffectEventMutationPhase: boolean = false;
 
 export const enableYieldingBeforePassive: boolean = true;
 

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;

packages/shared/forks/ReactFeatureFlags.test-renderer.www.js

@@ -62,6 +62,7 @@ export const disableLegacyMode: boolean = true;
 
 export const enableObjectFiber: boolean = false;
 export const enableEagerAlternateStateNodeCleanup: boolean = true;
+export const enableEffectEventMutationPhase: boolean = false;
 
 export const enableYieldingBeforePassive: boolean = false;
 

packages/shared/forks/ReactFeatureFlags.www-dynamic.js

@@ -39,6 +39,8 @@ export const enableInternalInstanceMap: boolean = __VARIANT__;
 export const enableTrustedTypesIntegration: boolean = __VARIANT__;
 export const enableParallelTransitions: 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__.

packages/shared/forks/ReactFeatureFlags.www.js

@@ -18,6 +18,7 @@ export const {
   alwaysThrottleRetries,
   disableLegacyContextForFunctionComponents,
   disableSchedulerTimeoutInWorkLoop,
+  enableEffectEventMutationPhase,
   enableHiddenSubtreeInsertionEffectCleanup,
   enableInfiniteRenderLoopDetection,
   enableNoCloningMemoCache,