[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 <git@andrewclark.io>
lauren committed
Oct 19, 2022 at 11:59 UTC
3cc792bfb53c63de34bbb1e8ca131c11faca6cba
7 files changed
+179
-99
packages/react-reconciler/src/ReactFiberCommitWork.new.js
+4
-12
@@ -15,11 +15,7 @@ import type {
15
ChildSet,
16
UpdatePayload,
17
} from './ReactFiberHostConfig';
18
-import type {
19
- Fiber,
20
- FiberRoot,
21
- EventFunctionWrapper,
22
-} from './ReactInternalTypes';
18
+import type {Fiber, FiberRoot} from './ReactInternalTypes';
19
import type {Lanes} from './ReactFiberLane.new';
20
import type {SuspenseState} from './ReactFiberSuspenseComponent.new';
21
import type {UpdateQueue} from './ReactFiberClassUpdateQueue.new';
@@ -689,13 +685,9 @@ function commitUseEventMount(finishedWork: Fiber) {
685
const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any);
686
const eventPayloads = updateQueue !== null ? updateQueue.events : null;
687
if (eventPayloads !== null) {
692
- // FunctionComponentUpdateQueue.events is a flat array of
693
- // [EventFunctionWrapper, EventFunction, ...], so increment by 2 each iteration to find the next
694
- // pair.
695
- for (let ii = 0; ii < eventPayloads.length; ii += 2) {
696
- const eventFn: EventFunctionWrapper<any, any, any> = eventPayloads[ii];
697
- const nextImpl = eventPayloads[ii + 1];
698
- eventFn._impl = nextImpl;
688
+ for (let ii = 0; ii < eventPayloads.length; ii++) {
689
+ const {ref, nextImpl} = eventPayloads[ii];
690
+ ref.impl = nextImpl;
691
}
692
}
693
}
packages/react-reconciler/src/ReactFiberCommitWork.old.js
+4
-12
@@ -15,11 +15,7 @@ import type {
15
ChildSet,
16
UpdatePayload,
17
} from './ReactFiberHostConfig';
18
-import type {
19
- Fiber,
20
- FiberRoot,
21
- EventFunctionWrapper,
22
-} from './ReactInternalTypes';
18
+import type {Fiber, FiberRoot} from './ReactInternalTypes';
19
import type {Lanes} from './ReactFiberLane.old';
20
import type {SuspenseState} from './ReactFiberSuspenseComponent.old';
21
import type {UpdateQueue} from './ReactFiberClassUpdateQueue.old';
@@ -689,13 +685,9 @@ function commitUseEventMount(finishedWork: Fiber) {
685
const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any);
686
const eventPayloads = updateQueue !== null ? updateQueue.events : null;
687
if (eventPayloads !== null) {
692
- // FunctionComponentUpdateQueue.events is a flat array of
693
- // [EventFunctionWrapper, EventFunction, ...], so increment by 2 each iteration to find the next
694
- // pair.
695
- for (let ii = 0; ii < eventPayloads.length; ii += 2) {
696
- const eventFn: EventFunctionWrapper<any, any, any> = eventPayloads[ii];
697
- const nextImpl = eventPayloads[ii + 1];
698
- eventFn._impl = nextImpl;
688
+ for (let ii = 0; ii < eventPayloads.length; ii++) {
689
+ const {ref, nextImpl} = eventPayloads[ii];
690
+ ref.impl = nextImpl;
691
}
692
}
693
}
packages/react-reconciler/src/ReactFiberHooks.new.js
+38
-27
@@ -22,7 +22,6 @@ import type {
22
Dispatcher,
23
HookType,
24
MemoCache,
25
- EventFunctionWrapper,
25
} from './ReactInternalTypes';
26
import type {Lanes, Lane} from './ReactFiberLane.new';
27
import type {HookFlags} from './ReactHookEffectTags';
@@ -189,9 +188,17 @@ type StoreConsistencyCheck<T> = {
188
getSnapshot: () => T,
189
};
190
191
+type EventFunctionPayload<Args, Return, F: (...Array<Args>) => Return> = {
192
+ ref: {
193
+ eventFn: F,
194
+ impl: F,
195
+ },
196
+ nextImpl: F,
197
+};
198
+
199
export type FunctionComponentUpdateQueue = {
200
lastEffect: Effect | null,
194
- events: Array<() => mixed> | null,
201
+ events: Array<EventFunctionPayload<any, any, any>> | null,
202
stores: Array<StoreConsistencyCheck<any>> | null,
203
// NOTE: optional, only set when enableUseMemoCacheHook is enabled
204
memoCache?: MemoCache | null,
@@ -1909,52 +1916,56 @@ function updateEffect(
1916
}
1917
1918
function useEventImpl<Args, Return, F: (...Array<Args>) => Return>(
1912
- event: EventFunctionWrapper<Args, Return, F>,
1913
- nextImpl: F,
1919
+ payload: EventFunctionPayload<Args, Return, F>,
1920
) {
1921
currentlyRenderingFiber.flags |= UpdateEffect;
1922
let componentUpdateQueue: null | FunctionComponentUpdateQueue = (currentlyRenderingFiber.updateQueue: any);
1923
if (componentUpdateQueue === null) {
1924
componentUpdateQueue = createFunctionComponentUpdateQueue();
1925
currentlyRenderingFiber.updateQueue = (componentUpdateQueue: any);
1920
- componentUpdateQueue.events = [event, nextImpl];
1926
+ componentUpdateQueue.events = [payload];
1927
} else {
1928
const events = componentUpdateQueue.events;
1929
if (events === null) {
1924
- componentUpdateQueue.events = [event, nextImpl];
1930
+ componentUpdateQueue.events = [payload];
1931
} else {
1926
- events.push(event, nextImpl);
1932
+ events.push(payload);
1933
}
1934
}
1935
}
1936
1937
function mountEvent<Args, Return, F: (...Array<Args>) => Return>(
1938
callback: F,
1933
-): EventFunctionWrapper<Args, Return, F> {
1939
+): F {
1940
const hook = mountWorkInProgressHook();
1935
- const eventFn: EventFunctionWrapper<Args, Return, F> = function eventFn() {
1941
+ const ref = {impl: callback};
1942
+ hook.memoizedState = ref;
1943
+ // $FlowIgnore[incompatible-return]
1944
+ return function eventFn() {
1945
if (isInvalidExecutionContextForEventFunction()) {
1946
throw new Error(
1947
"A function wrapped in useEvent can't be called during rendering.",
1948
);
1949
}
1941
- // $FlowFixMe[prop-missing] found when upgrading Flow
1942
- return eventFn._impl.apply(undefined, arguments);
1950
+ return ref.impl.apply(undefined, arguments);
1951
};
1944
- eventFn._impl = callback;
1945
-
1946
- useEventImpl(eventFn, callback);
1947
- hook.memoizedState = eventFn;
1948
- return eventFn;
1952
}
1953
1954
function updateEvent<Args, Return, F: (...Array<Args>) => Return>(
1955
callback: F,
1953
-): EventFunctionWrapper<Args, Return, F> {
1956
+): F {
1957
const hook = updateWorkInProgressHook();
1955
- const eventFn = hook.memoizedState;
1956
- useEventImpl(eventFn, callback);
1957
- return eventFn;
1958
+ const ref = hook.memoizedState;
1959
+ useEventImpl({ref, nextImpl: callback});
1960
+ // $FlowIgnore[incompatible-return]
1961
+ return function eventFn() {
1962
+ if (isInvalidExecutionContextForEventFunction()) {
1963
+ throw new Error(
1964
+ "A function wrapped in useEvent can't be called during rendering.",
1965
+ );
1966
+ }
1967
+ return ref.impl.apply(undefined, arguments);
1968
+ };
1969
}
1970
1971
function mountInsertionEffect(
@@ -2916,7 +2927,7 @@ if (__DEV__) {
2927
Args,
2928
Return,
2929
F: (...Array<Args>) => Return,
2919
- >(callback: F): EventFunctionWrapper<Args, Return, F> {
2930
+ >(callback: F): F {
2931
currentHookNameInDev = 'useEvent';
2932
mountHookTypesDev();
2933
return mountEvent(callback);
@@ -3073,7 +3084,7 @@ if (__DEV__) {
3084
Args,
3085
Return,
3086
F: (...Array<Args>) => Return,
3076
- >(callback: F): EventFunctionWrapper<Args, Return, F> {
3087
+ >(callback: F): F {
3088
currentHookNameInDev = 'useEvent';
3089
updateHookTypesDev();
3090
return mountEvent(callback);
@@ -3230,7 +3241,7 @@ if (__DEV__) {
3241
Args,
3242
Return,
3243
F: (...Array<Args>) => Return,
3233
- >(callback: F): EventFunctionWrapper<Args, Return, F> {
3244
+ >(callback: F): F {
3245
currentHookNameInDev = 'useEvent';
3246
updateHookTypesDev();
3247
return updateEvent(callback);
@@ -3388,7 +3399,7 @@ if (__DEV__) {
3399
Args,
3400
Return,
3401
F: (...Array<Args>) => Return,
3391
- >(callback: F): EventFunctionWrapper<Args, Return, F> {
3402
+ >(callback: F): F {
3403
currentHookNameInDev = 'useEvent';
3404
updateHookTypesDev();
3405
return updateEvent(callback);
@@ -3572,7 +3583,7 @@ if (__DEV__) {
3583
Args,
3584
Return,
3585
F: (...Array<Args>) => Return,
3575
- >(callback: F): EventFunctionWrapper<Args, Return, F> {
3586
+ >(callback: F): F {
3587
currentHookNameInDev = 'useEvent';
3588
warnInvalidHookAccess();
3589
mountHookTypesDev();
@@ -3757,7 +3768,7 @@ if (__DEV__) {
3768
Args,
3769
Return,
3770
F: (...Array<Args>) => Return,
3760
- >(callback: F): EventFunctionWrapper<Args, Return, F> {
3771
+ >(callback: F): F {
3772
currentHookNameInDev = 'useEvent';
3773
warnInvalidHookAccess();
3774
updateHookTypesDev();
@@ -3943,7 +3954,7 @@ if (__DEV__) {
3954
Args,
3955
Return,
3956
F: (...Array<Args>) => Return,
3946
- >(callback: F): EventFunctionWrapper<Args, Return, F> {
3957
+ >(callback: F): F {
3958
currentHookNameInDev = 'useEvent';
3959
warnInvalidHookAccess();
3960
updateHookTypesDev();
packages/react-reconciler/src/ReactFiberHooks.old.js
+38
-27
@@ -22,7 +22,6 @@ import type {
22
Dispatcher,
23
HookType,
24
MemoCache,
25
- EventFunctionWrapper,
25
} from './ReactInternalTypes';
26
import type {Lanes, Lane} from './ReactFiberLane.old';
27
import type {HookFlags} from './ReactHookEffectTags';
@@ -189,9 +188,17 @@ type StoreConsistencyCheck<T> = {
188
getSnapshot: () => T,
189
};
190
191
+type EventFunctionPayload<Args, Return, F: (...Array<Args>) => Return> = {
192
+ ref: {
193
+ eventFn: F,
194
+ impl: F,
195
+ },
196
+ nextImpl: F,
197
+};
198
+
199
export type FunctionComponentUpdateQueue = {
200
lastEffect: Effect | null,
194
- events: Array<() => mixed> | null,
201
+ events: Array<EventFunctionPayload<any, any, any>> | null,
202
stores: Array<StoreConsistencyCheck<any>> | null,
203
// NOTE: optional, only set when enableUseMemoCacheHook is enabled
204
memoCache?: MemoCache | null,
@@ -1909,52 +1916,56 @@ function updateEffect(
1916
}
1917
1918
function useEventImpl<Args, Return, F: (...Array<Args>) => Return>(
1912
- event: EventFunctionWrapper<Args, Return, F>,
1913
- nextImpl: F,
1919
+ payload: EventFunctionPayload<Args, Return, F>,
1920
) {
1921
currentlyRenderingFiber.flags |= UpdateEffect;
1922
let componentUpdateQueue: null | FunctionComponentUpdateQueue = (currentlyRenderingFiber.updateQueue: any);
1923
if (componentUpdateQueue === null) {
1924
componentUpdateQueue = createFunctionComponentUpdateQueue();
1925
currentlyRenderingFiber.updateQueue = (componentUpdateQueue: any);
1920
- componentUpdateQueue.events = [event, nextImpl];
1926
+ componentUpdateQueue.events = [payload];
1927
} else {
1928
const events = componentUpdateQueue.events;
1929
if (events === null) {
1924
- componentUpdateQueue.events = [event, nextImpl];
1930
+ componentUpdateQueue.events = [payload];
1931
} else {
1926
- events.push(event, nextImpl);
1932
+ events.push(payload);
1933
}
1934
}
1935
}
1936
1937
function mountEvent<Args, Return, F: (...Array<Args>) => Return>(
1938
callback: F,
1933
-): EventFunctionWrapper<Args, Return, F> {
1939
+): F {
1940
const hook = mountWorkInProgressHook();
1935
- const eventFn: EventFunctionWrapper<Args, Return, F> = function eventFn() {
1941
+ const ref = {impl: callback};
1942
+ hook.memoizedState = ref;
1943
+ // $FlowIgnore[incompatible-return]
1944
+ return function eventFn() {
1945
if (isInvalidExecutionContextForEventFunction()) {
1946
throw new Error(
1947
"A function wrapped in useEvent can't be called during rendering.",
1948
);
1949
}
1941
- // $FlowFixMe[prop-missing] found when upgrading Flow
1942
- return eventFn._impl.apply(undefined, arguments);
1950
+ return ref.impl.apply(undefined, arguments);
1951
};
1944
- eventFn._impl = callback;
1945
-
1946
- useEventImpl(eventFn, callback);
1947
- hook.memoizedState = eventFn;
1948
- return eventFn;
1952
}
1953
1954
function updateEvent<Args, Return, F: (...Array<Args>) => Return>(
1955
callback: F,
1953
-): EventFunctionWrapper<Args, Return, F> {
1956
+): F {
1957
const hook = updateWorkInProgressHook();
1955
- const eventFn = hook.memoizedState;
1956
- useEventImpl(eventFn, callback);
1957
- return eventFn;
1958
+ const ref = hook.memoizedState;
1959
+ useEventImpl({ref, nextImpl: callback});
1960
+ // $FlowIgnore[incompatible-return]
1961
+ return function eventFn() {
1962
+ if (isInvalidExecutionContextForEventFunction()) {
1963
+ throw new Error(
1964
+ "A function wrapped in useEvent can't be called during rendering.",
1965
+ );
1966
+ }
1967
+ return ref.impl.apply(undefined, arguments);
1968
+ };
1969
}
1970
1971
function mountInsertionEffect(
@@ -2916,7 +2927,7 @@ if (__DEV__) {
2927
Args,
2928
Return,
2929
F: (...Array<Args>) => Return,
2919
- >(callback: F): EventFunctionWrapper<Args, Return, F> {
2930
+ >(callback: F): F {
2931
currentHookNameInDev = 'useEvent';
2932
mountHookTypesDev();
2933
return mountEvent(callback);
@@ -3073,7 +3084,7 @@ if (__DEV__) {
3084
Args,
3085
Return,
3086
F: (...Array<Args>) => Return,
3076
- >(callback: F): EventFunctionWrapper<Args, Return, F> {
3087
+ >(callback: F): F {
3088
currentHookNameInDev = 'useEvent';
3089
updateHookTypesDev();
3090
return mountEvent(callback);
@@ -3230,7 +3241,7 @@ if (__DEV__) {
3241
Args,
3242
Return,
3243
F: (...Array<Args>) => Return,
3233
- >(callback: F): EventFunctionWrapper<Args, Return, F> {
3244
+ >(callback: F): F {
3245
currentHookNameInDev = 'useEvent';
3246
updateHookTypesDev();
3247
return updateEvent(callback);
@@ -3388,7 +3399,7 @@ if (__DEV__) {
3399
Args,
3400
Return,
3401
F: (...Array<Args>) => Return,
3391
- >(callback: F): EventFunctionWrapper<Args, Return, F> {
3402
+ >(callback: F): F {
3403
currentHookNameInDev = 'useEvent';
3404
updateHookTypesDev();
3405
return updateEvent(callback);
@@ -3572,7 +3583,7 @@ if (__DEV__) {
3583
Args,
3584
Return,
3585
F: (...Array<Args>) => Return,
3575
- >(callback: F): EventFunctionWrapper<Args, Return, F> {
3586
+ >(callback: F): F {
3587
currentHookNameInDev = 'useEvent';
3588
warnInvalidHookAccess();
3589
mountHookTypesDev();
@@ -3757,7 +3768,7 @@ if (__DEV__) {
3768
Args,
3769
Return,
3770
F: (...Array<Args>) => Return,
3760
- >(callback: F): EventFunctionWrapper<Args, Return, F> {
3771
+ >(callback: F): F {
3772
currentHookNameInDev = 'useEvent';
3773
warnInvalidHookAccess();
3774
updateHookTypesDev();
@@ -3943,7 +3954,7 @@ if (__DEV__) {
3954
Args,
3955
Return,
3956
F: (...Array<Args>) => Return,
3946
- >(callback: F): EventFunctionWrapper<Args, Return, F> {
3957
+ >(callback: F): F {
3958
currentHookNameInDev = 'useEvent';
3959
warnInvalidHookAccess();
3960
updateHookTypesDev();
packages/react-reconciler/src/ReactInternalTypes.js
+1
-14
@@ -290,17 +290,6 @@ type SuspenseCallbackOnlyFiberRootProperties = {
290
hydrationCallbacks: null | SuspenseHydrationCallbacks,
291
};
292
293
-// A wrapper callable object around a useEvent callback that throws if the callback is called during
294
-// rendering. The _impl property points to the actual implementation.
295
-export type EventFunctionWrapper<
296
- Args,
297
- Return,
298
- F: (...Array<Args>) => Return,
299
-> = {
300
- (): F,
301
- _impl: F,
302
-};
303
-
293
export type TransitionTracingCallbacks = {
294
onTransitionStart?: (transitionName: string, startTime: number) => void,
295
onTransitionProgress?: (
@@ -390,9 +379,7 @@ export type Dispatcher = {
379
create: () => (() => void) | void,
380
deps: Array<mixed> | void | null,
381
): void,
393
- useEvent?: <Args, Return, F: (...Array<Args>) => Return>(
394
- callback: F,
395
- ) => EventFunctionWrapper<Args, Return, F>,
382
+ useEvent?: <Args, Return, F: (...Array<Args>) => Return>(callback: F) => F,
383
useInsertionEffect(
384
create: () => (() => void) | void,
385
deps: Array<mixed> | void | null,
packages/react-reconciler/src/__tests__/useEvent-test.js
+91
-2
@@ -557,6 +557,95 @@ describe('useEvent', () => {
557
expect(Scheduler).toHaveYielded(['Effect value: 2', 'Event value: 2']);
558
});
559
560
+ // @gate enableUseEventHook
561
+ it("doesn't provide a stable identity", () => {
562
+ function Counter({shouldRender, value}) {
563
+ const onClick = useEvent(() => {
564
+ Scheduler.unstable_yieldValue(
565
+ 'onClick, shouldRender=' + shouldRender + ', value=' + value,
566
+ );
567
+ });
568
+
569
+ // onClick doesn't have a stable function identity so this effect will fire on every render.
570
+ // In a real app useEvent functions should *not* be passed as a dependency, this is for
571
+ // testing purposes only.
572
+ useEffect(() => {
573
+ onClick();
574
+ }, [onClick]);
575
+
576
+ useEffect(() => {
577
+ onClick();
578
+ }, [shouldRender]);
579
+
580
+ return <></>;
581
+ }
582
+
583
+ ReactNoop.render(<Counter shouldRender={true} value={0} />);
584
+ expect(Scheduler).toFlushAndYield([
585
+ 'onClick, shouldRender=true, value=0',
586
+ 'onClick, shouldRender=true, value=0',
587
+ ]);
588
+
589
+ ReactNoop.render(<Counter shouldRender={true} value={1} />);
590
+ expect(Scheduler).toFlushAndYield(['onClick, shouldRender=true, value=1']);
591
+
592
+ ReactNoop.render(<Counter shouldRender={false} value={2} />);
593
+ expect(Scheduler).toFlushAndYield([
594
+ 'onClick, shouldRender=false, value=2',
595
+ 'onClick, shouldRender=false, value=2',
596
+ ]);
597
+ });
598
+
599
+ // @gate enableUseEventHook
600
+ it('event handlers always see the latest committed value', async () => {
601
+ let committedEventHandler = null;
602
+
603
+ function App({value}) {
604
+ const event = useEvent(() => {
605
+ return 'Value seen by useEvent: ' + value;
606
+ });
607
+
608
+ // Set up an effect that registers the event handler with an external
609
+ // event system (e.g. addEventListener).
610
+ useEffect(
611
+ () => {
612
+ // Log when the effect fires. In the test below, we'll assert that this
613
+ // only happens during initial render, not during updates.
614
+ Scheduler.unstable_yieldValue('Commit new event handler');
615
+ committedEventHandler = event;
616
+ return () => {
617
+ committedEventHandler = null;
618
+ };
619
+ },
620
+ // Note that we've intentionally omitted the event from the dependency
621
+ // array. But it will still be able to see the latest `value`. This is the
622
+ // key feature of useEvent that makes it different from a regular closure.
623
+ [],
624
+ );
625
+ return 'Latest rendered value ' + value;
626
+ }
627
+
628
+ // Initial render
629
+ const root = ReactNoop.createRoot();
630
+ await act(async () => {
631
+ root.render(<App value={1} />);
632
+ });
633
+ expect(Scheduler).toHaveYielded(['Commit new event handler']);
634
+ expect(root).toMatchRenderedOutput('Latest rendered value 1');
635
+ expect(committedEventHandler()).toBe('Value seen by useEvent: 1');
636
+
637
+ // Update
638
+ await act(async () => {
639
+ root.render(<App value={2} />);
640
+ });
641
+ // No new event handler should be committed, because it was omitted from
642
+ // the dependency array.
643
+ expect(Scheduler).toHaveYielded([]);
644
+ // But the event handler should still be able to see the latest value.
645
+ expect(root).toMatchRenderedOutput('Latest rendered value 2');
646
+ expect(committedEventHandler()).toBe('Value seen by useEvent: 2');
647
+ });
648
+
649
// @gate enableUseEventHook
650
it('integration: implements docs chat room example', () => {
651
function createConnection() {
@@ -597,7 +686,7 @@ describe('useEvent', () => {
686
});
687
connection.connect();
688
return () => connection.disconnect();
600
- }, [roomId, onConnected]);
689
+ }, [roomId]);
690
691
return <Text text={`Welcome to the ${roomId} room!`} />;
692
}
@@ -676,7 +765,7 @@ describe('useEvent', () => {
765
766
const onVisit = useEvent(visitedUrl => {
767
Scheduler.unstable_yieldValue(
679
- 'url: ' + url + ', numberOfItems: ' + numberOfItems,
768
+ 'url: ' + visitedUrl + ', numberOfItems: ' + numberOfItems,
769
);
770
});
771
packages/react-server/src/ReactFizzHooks.js
+3
-5
@@ -7,10 +7,7 @@
7
* @flow
8
*/
9
10
-import type {
11
- Dispatcher,
12
- EventFunctionWrapper,
13
-} from 'react-reconciler/src/ReactInternalTypes';
10
+import type {Dispatcher} from 'react-reconciler/src/ReactInternalTypes';
11
12
import type {
13
MutableSource,
@@ -522,7 +519,8 @@ function throwOnUseEventCall() {
519
520
export function useEvent<Args, Return, F: (...Array<Args>) => Return>(
521
callback: F,
525
-): EventFunctionWrapper<Args, Return, F> {
522
+): F {
523
+ // $FlowIgnore[incompatible-return]
524
return throwOnUseEventCall;
525
}
526