From 3cc792bfb53c63de34bbb1e8ca131c11faca6cba 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 ea8e0cf263..e16aa04507 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 b33a051ee0..4aa073d0db 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 8183ddd155..2e399e7897 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 97aa7dcd4a..7c4a705517 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 10c4c9853a..a5f28fd5d2 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 e3b0d872f8..fecf99679d 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 c8ceeddc40..a9295b3143 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; }