From 372177dae50fcf74aaac714a7e8e1e3bc3a0b54c Mon Sep 17 00:00:00 2001 From: lauren Date: Wed, 19 Oct 2022 11:59:27 -0700 Subject: [PATCH] [useEvent] Non-stable function identity (#25473) * [useEvent] Non-stable function identity Since useEvent shouldn't go in the dependency list of whatever is consuming it (which is enforced by the fact that useEvent functions are always locally created and never passed by reference), its identity doesn't matter. Effectively, this PR is a runtime assertion that you can't rely on the return value of useEvent to be stable. * Test: Events should see latest bindings The key feature of useEvent that makes it different from useCallback is that events always see the latest committed values. There's no such thing as a "stale" event handler. * Don't queue a commit effect on mount * Inline event function wrapping - Inlines wrapping of the callback - Use a mutable ref-style object instead of a callable object - Fix types Co-authored-by: Andrew Clark --- .../src/ReactFiberCommitWork.new.js | 16 +--- .../src/ReactFiberCommitWork.old.js | 16 +--- .../src/ReactFiberHooks.new.js | 65 +++++++------ .../src/ReactFiberHooks.old.js | 65 +++++++------ .../src/ReactInternalTypes.js | 15 +-- .../src/__tests__/useEvent-test.js | 93 ++++++++++++++++++- packages/react-server/src/ReactFizzHooks.js | 8 +- 7 files changed, 179 insertions(+), 99 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index ea8e0cf26354b..e16aa04507961 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -15,11 +15,7 @@ import type { ChildSet, UpdatePayload, } from './ReactFiberHostConfig'; -import type { - Fiber, - FiberRoot, - EventFunctionWrapper, -} from './ReactInternalTypes'; +import type {Fiber, FiberRoot} from './ReactInternalTypes'; import type {Lanes} from './ReactFiberLane.new'; import type {SuspenseState} from './ReactFiberSuspenseComponent.new'; import type {UpdateQueue} from './ReactFiberClassUpdateQueue.new'; @@ -689,13 +685,9 @@ function commitUseEventMount(finishedWork: Fiber) { const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any); const eventPayloads = updateQueue !== null ? updateQueue.events : null; if (eventPayloads !== null) { - // FunctionComponentUpdateQueue.events is a flat array of - // [EventFunctionWrapper, EventFunction, ...], so increment by 2 each iteration to find the next - // pair. - for (let ii = 0; ii < eventPayloads.length; ii += 2) { - const eventFn: EventFunctionWrapper = eventPayloads[ii]; - const nextImpl = eventPayloads[ii + 1]; - eventFn._impl = nextImpl; + for (let ii = 0; ii < eventPayloads.length; ii++) { + const {ref, nextImpl} = eventPayloads[ii]; + ref.impl = nextImpl; } } } diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index b33a051ee09d0..4aa073d0db31a 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -15,11 +15,7 @@ import type { ChildSet, UpdatePayload, } from './ReactFiberHostConfig'; -import type { - Fiber, - FiberRoot, - EventFunctionWrapper, -} from './ReactInternalTypes'; +import type {Fiber, FiberRoot} from './ReactInternalTypes'; import type {Lanes} from './ReactFiberLane.old'; import type {SuspenseState} from './ReactFiberSuspenseComponent.old'; import type {UpdateQueue} from './ReactFiberClassUpdateQueue.old'; @@ -689,13 +685,9 @@ function commitUseEventMount(finishedWork: Fiber) { const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any); const eventPayloads = updateQueue !== null ? updateQueue.events : null; if (eventPayloads !== null) { - // FunctionComponentUpdateQueue.events is a flat array of - // [EventFunctionWrapper, EventFunction, ...], so increment by 2 each iteration to find the next - // pair. - for (let ii = 0; ii < eventPayloads.length; ii += 2) { - const eventFn: EventFunctionWrapper = eventPayloads[ii]; - const nextImpl = eventPayloads[ii + 1]; - eventFn._impl = nextImpl; + for (let ii = 0; ii < eventPayloads.length; ii++) { + const {ref, nextImpl} = eventPayloads[ii]; + ref.impl = nextImpl; } } } diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index 8183ddd1557c0..2e399e7897794 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -22,7 +22,6 @@ import type { Dispatcher, HookType, MemoCache, - EventFunctionWrapper, } from './ReactInternalTypes'; import type {Lanes, Lane} from './ReactFiberLane.new'; import type {HookFlags} from './ReactHookEffectTags'; @@ -189,9 +188,17 @@ type StoreConsistencyCheck = { getSnapshot: () => T, }; +type EventFunctionPayload) => Return> = { + ref: { + eventFn: F, + impl: F, + }, + nextImpl: F, +}; + export type FunctionComponentUpdateQueue = { lastEffect: Effect | null, - events: Array<() => mixed> | null, + events: Array> | null, stores: Array> | null, // NOTE: optional, only set when enableUseMemoCacheHook is enabled memoCache?: MemoCache | null, @@ -1909,52 +1916,56 @@ function updateEffect( } function useEventImpl) => Return>( - event: EventFunctionWrapper, - nextImpl: F, + payload: EventFunctionPayload, ) { currentlyRenderingFiber.flags |= UpdateEffect; let componentUpdateQueue: null | FunctionComponentUpdateQueue = (currentlyRenderingFiber.updateQueue: any); if (componentUpdateQueue === null) { componentUpdateQueue = createFunctionComponentUpdateQueue(); currentlyRenderingFiber.updateQueue = (componentUpdateQueue: any); - componentUpdateQueue.events = [event, nextImpl]; + componentUpdateQueue.events = [payload]; } else { const events = componentUpdateQueue.events; if (events === null) { - componentUpdateQueue.events = [event, nextImpl]; + componentUpdateQueue.events = [payload]; } else { - events.push(event, nextImpl); + events.push(payload); } } } function mountEvent) => Return>( callback: F, -): EventFunctionWrapper { +): F { const hook = mountWorkInProgressHook(); - const eventFn: EventFunctionWrapper = function eventFn() { + const ref = {impl: callback}; + hook.memoizedState = ref; + // $FlowIgnore[incompatible-return] + return function eventFn() { if (isInvalidExecutionContextForEventFunction()) { throw new Error( "A function wrapped in useEvent can't be called during rendering.", ); } - // $FlowFixMe[prop-missing] found when upgrading Flow - return eventFn._impl.apply(undefined, arguments); + return ref.impl.apply(undefined, arguments); }; - eventFn._impl = callback; - - useEventImpl(eventFn, callback); - hook.memoizedState = eventFn; - return eventFn; } function updateEvent) => Return>( callback: F, -): EventFunctionWrapper { +): F { const hook = updateWorkInProgressHook(); - const eventFn = hook.memoizedState; - useEventImpl(eventFn, callback); - return eventFn; + const ref = hook.memoizedState; + useEventImpl({ref, nextImpl: callback}); + // $FlowIgnore[incompatible-return] + return function eventFn() { + if (isInvalidExecutionContextForEventFunction()) { + throw new Error( + "A function wrapped in useEvent can't be called during rendering.", + ); + } + return ref.impl.apply(undefined, arguments); + }; } function mountInsertionEffect( @@ -2916,7 +2927,7 @@ if (__DEV__) { Args, Return, F: (...Array) => Return, - >(callback: F): EventFunctionWrapper { + >(callback: F): F { currentHookNameInDev = 'useEvent'; mountHookTypesDev(); return mountEvent(callback); @@ -3073,7 +3084,7 @@ if (__DEV__) { Args, Return, F: (...Array) => Return, - >(callback: F): EventFunctionWrapper { + >(callback: F): F { currentHookNameInDev = 'useEvent'; updateHookTypesDev(); return mountEvent(callback); @@ -3230,7 +3241,7 @@ if (__DEV__) { Args, Return, F: (...Array) => Return, - >(callback: F): EventFunctionWrapper { + >(callback: F): F { currentHookNameInDev = 'useEvent'; updateHookTypesDev(); return updateEvent(callback); @@ -3388,7 +3399,7 @@ if (__DEV__) { Args, Return, F: (...Array) => Return, - >(callback: F): EventFunctionWrapper { + >(callback: F): F { currentHookNameInDev = 'useEvent'; updateHookTypesDev(); return updateEvent(callback); @@ -3572,7 +3583,7 @@ if (__DEV__) { Args, Return, F: (...Array) => Return, - >(callback: F): EventFunctionWrapper { + >(callback: F): F { currentHookNameInDev = 'useEvent'; warnInvalidHookAccess(); mountHookTypesDev(); @@ -3757,7 +3768,7 @@ if (__DEV__) { Args, Return, F: (...Array) => Return, - >(callback: F): EventFunctionWrapper { + >(callback: F): F { currentHookNameInDev = 'useEvent'; warnInvalidHookAccess(); updateHookTypesDev(); @@ -3943,7 +3954,7 @@ if (__DEV__) { Args, Return, F: (...Array) => Return, - >(callback: F): EventFunctionWrapper { + >(callback: F): F { currentHookNameInDev = 'useEvent'; warnInvalidHookAccess(); updateHookTypesDev(); diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js index 97aa7dcd4a41a..7c4a705517346 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -22,7 +22,6 @@ import type { Dispatcher, HookType, MemoCache, - EventFunctionWrapper, } from './ReactInternalTypes'; import type {Lanes, Lane} from './ReactFiberLane.old'; import type {HookFlags} from './ReactHookEffectTags'; @@ -189,9 +188,17 @@ type StoreConsistencyCheck = { getSnapshot: () => T, }; +type EventFunctionPayload) => Return> = { + ref: { + eventFn: F, + impl: F, + }, + nextImpl: F, +}; + export type FunctionComponentUpdateQueue = { lastEffect: Effect | null, - events: Array<() => mixed> | null, + events: Array> | null, stores: Array> | null, // NOTE: optional, only set when enableUseMemoCacheHook is enabled memoCache?: MemoCache | null, @@ -1909,52 +1916,56 @@ function updateEffect( } function useEventImpl) => Return>( - event: EventFunctionWrapper, - nextImpl: F, + payload: EventFunctionPayload, ) { currentlyRenderingFiber.flags |= UpdateEffect; let componentUpdateQueue: null | FunctionComponentUpdateQueue = (currentlyRenderingFiber.updateQueue: any); if (componentUpdateQueue === null) { componentUpdateQueue = createFunctionComponentUpdateQueue(); currentlyRenderingFiber.updateQueue = (componentUpdateQueue: any); - componentUpdateQueue.events = [event, nextImpl]; + componentUpdateQueue.events = [payload]; } else { const events = componentUpdateQueue.events; if (events === null) { - componentUpdateQueue.events = [event, nextImpl]; + componentUpdateQueue.events = [payload]; } else { - events.push(event, nextImpl); + events.push(payload); } } } function mountEvent) => Return>( callback: F, -): EventFunctionWrapper { +): F { const hook = mountWorkInProgressHook(); - const eventFn: EventFunctionWrapper = function eventFn() { + const ref = {impl: callback}; + hook.memoizedState = ref; + // $FlowIgnore[incompatible-return] + return function eventFn() { if (isInvalidExecutionContextForEventFunction()) { throw new Error( "A function wrapped in useEvent can't be called during rendering.", ); } - // $FlowFixMe[prop-missing] found when upgrading Flow - return eventFn._impl.apply(undefined, arguments); + return ref.impl.apply(undefined, arguments); }; - eventFn._impl = callback; - - useEventImpl(eventFn, callback); - hook.memoizedState = eventFn; - return eventFn; } function updateEvent) => Return>( callback: F, -): EventFunctionWrapper { +): F { const hook = updateWorkInProgressHook(); - const eventFn = hook.memoizedState; - useEventImpl(eventFn, callback); - return eventFn; + const ref = hook.memoizedState; + useEventImpl({ref, nextImpl: callback}); + // $FlowIgnore[incompatible-return] + return function eventFn() { + if (isInvalidExecutionContextForEventFunction()) { + throw new Error( + "A function wrapped in useEvent can't be called during rendering.", + ); + } + return ref.impl.apply(undefined, arguments); + }; } function mountInsertionEffect( @@ -2916,7 +2927,7 @@ if (__DEV__) { Args, Return, F: (...Array) => Return, - >(callback: F): EventFunctionWrapper { + >(callback: F): F { currentHookNameInDev = 'useEvent'; mountHookTypesDev(); return mountEvent(callback); @@ -3073,7 +3084,7 @@ if (__DEV__) { Args, Return, F: (...Array) => Return, - >(callback: F): EventFunctionWrapper { + >(callback: F): F { currentHookNameInDev = 'useEvent'; updateHookTypesDev(); return mountEvent(callback); @@ -3230,7 +3241,7 @@ if (__DEV__) { Args, Return, F: (...Array) => Return, - >(callback: F): EventFunctionWrapper { + >(callback: F): F { currentHookNameInDev = 'useEvent'; updateHookTypesDev(); return updateEvent(callback); @@ -3388,7 +3399,7 @@ if (__DEV__) { Args, Return, F: (...Array) => Return, - >(callback: F): EventFunctionWrapper { + >(callback: F): F { currentHookNameInDev = 'useEvent'; updateHookTypesDev(); return updateEvent(callback); @@ -3572,7 +3583,7 @@ if (__DEV__) { Args, Return, F: (...Array) => Return, - >(callback: F): EventFunctionWrapper { + >(callback: F): F { currentHookNameInDev = 'useEvent'; warnInvalidHookAccess(); mountHookTypesDev(); @@ -3757,7 +3768,7 @@ if (__DEV__) { Args, Return, F: (...Array) => Return, - >(callback: F): EventFunctionWrapper { + >(callback: F): F { currentHookNameInDev = 'useEvent'; warnInvalidHookAccess(); updateHookTypesDev(); @@ -3943,7 +3954,7 @@ if (__DEV__) { Args, Return, F: (...Array) => Return, - >(callback: F): EventFunctionWrapper { + >(callback: F): F { currentHookNameInDev = 'useEvent'; warnInvalidHookAccess(); updateHookTypesDev(); diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js index 10c4c9853a300..a5f28fd5d2450 100644 --- a/packages/react-reconciler/src/ReactInternalTypes.js +++ b/packages/react-reconciler/src/ReactInternalTypes.js @@ -290,17 +290,6 @@ type SuspenseCallbackOnlyFiberRootProperties = { hydrationCallbacks: null | SuspenseHydrationCallbacks, }; -// A wrapper callable object around a useEvent callback that throws if the callback is called during -// rendering. The _impl property points to the actual implementation. -export type EventFunctionWrapper< - Args, - Return, - F: (...Array) => Return, -> = { - (): F, - _impl: F, -}; - export type TransitionTracingCallbacks = { onTransitionStart?: (transitionName: string, startTime: number) => void, onTransitionProgress?: ( @@ -390,9 +379,7 @@ export type Dispatcher = { create: () => (() => void) | void, deps: Array | void | null, ): void, - useEvent?: ) => Return>( - callback: F, - ) => EventFunctionWrapper, + useEvent?: ) => Return>(callback: F) => F, useInsertionEffect( create: () => (() => void) | void, deps: Array | void | null, diff --git a/packages/react-reconciler/src/__tests__/useEvent-test.js b/packages/react-reconciler/src/__tests__/useEvent-test.js index e3b0d872f857e..fecf99679d263 100644 --- a/packages/react-reconciler/src/__tests__/useEvent-test.js +++ b/packages/react-reconciler/src/__tests__/useEvent-test.js @@ -557,6 +557,95 @@ describe('useEvent', () => { expect(Scheduler).toHaveYielded(['Effect value: 2', 'Event value: 2']); }); + // @gate enableUseEventHook + it("doesn't provide a stable identity", () => { + function Counter({shouldRender, value}) { + const onClick = useEvent(() => { + Scheduler.unstable_yieldValue( + 'onClick, shouldRender=' + shouldRender + ', value=' + value, + ); + }); + + // onClick doesn't have a stable function identity so this effect will fire on every render. + // In a real app useEvent functions should *not* be passed as a dependency, this is for + // testing purposes only. + useEffect(() => { + onClick(); + }, [onClick]); + + useEffect(() => { + onClick(); + }, [shouldRender]); + + return <>; + } + + ReactNoop.render(); + expect(Scheduler).toFlushAndYield([ + 'onClick, shouldRender=true, value=0', + 'onClick, shouldRender=true, value=0', + ]); + + ReactNoop.render(); + expect(Scheduler).toFlushAndYield(['onClick, shouldRender=true, value=1']); + + ReactNoop.render(); + expect(Scheduler).toFlushAndYield([ + 'onClick, shouldRender=false, value=2', + 'onClick, shouldRender=false, value=2', + ]); + }); + + // @gate enableUseEventHook + it('event handlers always see the latest committed value', async () => { + let committedEventHandler = null; + + function App({value}) { + const event = useEvent(() => { + return 'Value seen by useEvent: ' + value; + }); + + // Set up an effect that registers the event handler with an external + // event system (e.g. addEventListener). + useEffect( + () => { + // Log when the effect fires. In the test below, we'll assert that this + // only happens during initial render, not during updates. + Scheduler.unstable_yieldValue('Commit new event handler'); + committedEventHandler = event; + return () => { + committedEventHandler = null; + }; + }, + // Note that we've intentionally omitted the event from the dependency + // array. But it will still be able to see the latest `value`. This is the + // key feature of useEvent that makes it different from a regular closure. + [], + ); + return 'Latest rendered value ' + value; + } + + // Initial render + const root = ReactNoop.createRoot(); + await act(async () => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['Commit new event handler']); + expect(root).toMatchRenderedOutput('Latest rendered value 1'); + expect(committedEventHandler()).toBe('Value seen by useEvent: 1'); + + // Update + await act(async () => { + root.render(); + }); + // No new event handler should be committed, because it was omitted from + // the dependency array. + expect(Scheduler).toHaveYielded([]); + // But the event handler should still be able to see the latest value. + expect(root).toMatchRenderedOutput('Latest rendered value 2'); + expect(committedEventHandler()).toBe('Value seen by useEvent: 2'); + }); + // @gate enableUseEventHook it('integration: implements docs chat room example', () => { function createConnection() { @@ -597,7 +686,7 @@ describe('useEvent', () => { }); connection.connect(); return () => connection.disconnect(); - }, [roomId, onConnected]); + }, [roomId]); return ; } @@ -676,7 +765,7 @@ describe('useEvent', () => { const onVisit = useEvent(visitedUrl => { Scheduler.unstable_yieldValue( - 'url: ' + url + ', numberOfItems: ' + numberOfItems, + 'url: ' + visitedUrl + ', numberOfItems: ' + numberOfItems, ); }); diff --git a/packages/react-server/src/ReactFizzHooks.js b/packages/react-server/src/ReactFizzHooks.js index c8ceeddc400e4..a9295b3143008 100644 --- a/packages/react-server/src/ReactFizzHooks.js +++ b/packages/react-server/src/ReactFizzHooks.js @@ -7,10 +7,7 @@ * @flow */ -import type { - Dispatcher, - EventFunctionWrapper, -} from 'react-reconciler/src/ReactInternalTypes'; +import type {Dispatcher} from 'react-reconciler/src/ReactInternalTypes'; import type { MutableSource, @@ -522,7 +519,8 @@ function throwOnUseEventCall() { export function useEvent) => Return>( callback: F, -): EventFunctionWrapper { +): F { + // $FlowIgnore[incompatible-return] return throwOnUseEventCall; }