Don't bother comparing constructor when deps are not provided (#14594)
* Don't bother comparing constructor when deps are not provided When no dependencies are passed to an effect hook, what we used to do is compare the effect constructor. If there was no change, then we would skip firing the effect. In practice, this is a useless optimization because the constructor will always be different when you pass an inline closure. And if you don't pass an inline closure, then you can't access any props or state. There are some edge cases where an effect that doesn't close over props or state could be useful, like reference counting the number of mounted components. But those are rare and can be addressed by passing an empty array of dependencies. By removing this "optimization," we can avoid retaining the constructor in the majority of cases where it's a closure that changes on every render. I made corresponding changes to the other hooks that accept dependencies, too (useMemo, useCallback, and useImperativeHandle). * Improve hook dependencies warning It now includes the name of the hook in the message. * Nits
Andrew Clark committed
Jan 16, 2019 at 17:53 UTC
1454a8be03794f5e0b23a7e7696cbbbdcf8b0f5d
5 files changed
+221
-84
packages/react-dom/src/server/ReactPartialRendererHooks.js
+60
-1
@@ -10,12 +10,12 @@
10
import typeof {Dispatcher as DispatcherType} from 'react-reconciler/src/ReactFiberDispatcher';
11
import type {ThreadID} from './ReactThreadIDAllocator';
12
import type {ReactContext} from 'shared/ReactTypes';
13
-import areHookInputsEqual from 'shared/areHookInputsEqual';
13
14
import {validateContextBounds} from './ReactPartialRendererContext';
15
16
import invariant from 'shared/invariant';
17
import warning from 'shared/warning';
18
+import is from 'shared/objectIs';
19
20
type BasicStateAction<S> = (S => S) | S;
21
type Dispatch<A> = A => void;
@@ -49,6 +49,9 @@ let renderPhaseUpdates: Map<UpdateQueue<any>, Update<any>> | null = null;
49
let numberOfReRenders: number = 0;
50
const RE_RENDER_LIMIT = 25;
51
52
+// In DEV, this is the name of the currently executing primitive hook
53
+let currentHookNameInDev: ?string;
54
+
55
function resolveCurrentlyRenderingComponent(): Object {
56
invariant(
57
currentlyRenderingComponent !== null,
@@ -57,6 +60,48 @@ function resolveCurrentlyRenderingComponent(): Object {
60
return currentlyRenderingComponent;
61
}
62
63
+function areHookInputsEqual(
64
+ nextDeps: Array<mixed>,
65
+ prevDeps: Array<mixed> | null,
66
+) {
67
+ if (prevDeps === null) {
68
+ if (__DEV__) {
69
+ warning(
70
+ false,
71
+ '%s received a final argument during this render, but not during ' +
72
+ 'the previous render. Even though the final argument is optional, ' +
73
+ 'its type cannot change between renders.',
74
+ currentHookNameInDev,
75
+ );
76
+ }
77
+ return false;
78
+ }
79
+
80
+ if (__DEV__) {
81
+ // Don't bother comparing lengths in prod because these arrays should be
82
+ // passed inline.
83
+ if (nextDeps.length !== prevDeps.length) {
84
+ warning(
85
+ false,
86
+ 'The final argument passed to %s changed size between renders. The ' +
87
+ 'order and size of this array must remain constant.\n\n' +
88
+ 'Previous: %s\n' +
89
+ 'Incoming: %s',
90
+ currentHookNameInDev,
91
+ `[${nextDeps.join(', ')}]`,
92
+ `[${prevDeps.join(', ')}]`,
93
+ );
94
+ }
95
+ }
96
+ for (let i = 0; i < prevDeps.length && i < nextDeps.length; i++) {
97
+ if (is(nextDeps[i], prevDeps[i])) {
98
+ continue;
99
+ }
100
+ return false;
101
+ }
102
+ return true;
103
+}
104
+
105
function createHook(): Hook {
106
return {
107
memoizedState: null,
@@ -153,6 +198,9 @@ function useContext<T>(
198
context: ReactContext<T>,
199
observedBits: void | number | boolean,
200
): T {
201
+ if (__DEV__) {
202
+ currentHookNameInDev = 'useContext';
203
+ }
204
resolveCurrentlyRenderingComponent();
205
let threadID = currentThreadID;
206
validateContextBounds(context, threadID);
@@ -166,6 +214,9 @@ function basicStateReducer<S>(state: S, action: BasicStateAction<S>): S {
214
export function useState<S>(
215
initialState: (() => S) | S,
216
): [S, Dispatch<BasicStateAction<S>>] {
217
+ if (__DEV__) {
218
+ currentHookNameInDev = 'useState';
219
+ }
220
return useReducer(
221
basicStateReducer,
222
// useReducer has a special case to support lazy useState initializers
@@ -178,6 +229,11 @@ export function useReducer<S, A>(
229
initialState: S,
230
initialAction: A | void | null,
231
): [S, Dispatch<A>] {
232
+ if (__DEV__) {
233
+ if (reducer !== basicStateReducer) {
234
+ currentHookNameInDev = 'useReducer';
235
+ }
236
+ }
237
currentlyRenderingComponent = resolveCurrentlyRenderingComponent();
238
workInProgressHook = createWorkInProgressHook();
239
if (isReRender) {
@@ -276,6 +332,9 @@ export function useLayoutEffect(
332
create: () => mixed,
333
inputs: Array<mixed> | void | null,
334
) {
335
+ if (__DEV__) {
336
+ currentHookNameInDev = 'useLayoutEffect';
337
+ }
338
warning(
339
false,
340
'useLayoutEffect does nothing on the server, because its effect cannot ' +
packages/react-reconciler/src/ReactFiberHooks.js
+122
-35
@@ -36,7 +36,7 @@ import {
36
import invariant from 'shared/invariant';
37
import warning from 'shared/warning';
38
import getComponentName from 'shared/getComponentName';
39
-import areHookInputsEqual from 'shared/areHookInputsEqual';
39
+import is from 'shared/objectIs';
40
import {markWorkInProgressReceivedUpdate} from './ReactFiberBeginWork';
41
42
type Update<S, A> = {
@@ -68,7 +68,7 @@ type Effect = {
68
tag: HookEffectTag,
69
create: () => mixed,
70
destroy: (() => mixed) | null,
71
- inputs: Array<mixed>,
71
+ deps: Array<mixed> | null,
72
next: Effect,
73
};
74
@@ -117,6 +117,9 @@ let renderPhaseUpdates: Map<
117
let numberOfReRenders: number = -1;
118
const RE_RENDER_LIMIT = 25;
119
120
+// In DEV, this is the name of the currently executing primitive hook
121
+let currentHookNameInDev: ?string;
122
+
123
function resolveCurrentlyRenderingFiber(): Fiber {
124
invariant(
125
currentlyRenderingFiber !== null,
@@ -125,6 +128,48 @@ function resolveCurrentlyRenderingFiber(): Fiber {
128
return currentlyRenderingFiber;
129
}
130
131
+function areHookInputsEqual(
132
+ nextDeps: Array<mixed>,
133
+ prevDeps: Array<mixed> | null,
134
+) {
135
+ if (prevDeps === null) {
136
+ if (__DEV__) {
137
+ warning(
138
+ false,
139
+ '%s received a final argument during this render, but not during ' +
140
+ 'the previous render. Even though the final argument is optional, ' +
141
+ 'its type cannot change between renders.',
142
+ currentHookNameInDev,
143
+ );
144
+ }
145
+ return false;
146
+ }
147
+
148
+ if (__DEV__) {
149
+ // Don't bother comparing lengths in prod because these arrays should be
150
+ // passed inline.
151
+ if (nextDeps.length !== prevDeps.length) {
152
+ warning(
153
+ false,
154
+ 'The final argument passed to %s changed size between renders. The ' +
155
+ 'order and size of this array must remain constant.\n\n' +
156
+ 'Previous: %s\n' +
157
+ 'Incoming: %s',
158
+ currentHookNameInDev,
159
+ `[${nextDeps.join(', ')}]`,
160
+ `[${prevDeps.join(', ')}]`,
161
+ );
162
+ }
163
+ }
164
+ for (let i = 0; i < prevDeps.length && i < nextDeps.length; i++) {
165
+ if (is(nextDeps[i], prevDeps[i])) {
166
+ continue;
167
+ }
168
+ return false;
169
+ }
170
+ return true;
171
+}
172
+
173
export function renderWithHooks(
174
current: Fiber | null,
175
workInProgress: Fiber,
@@ -202,6 +247,10 @@ export function renderWithHooks(
247
remainingExpirationTime = NoWork;
248
componentUpdateQueue = null;
249
250
+ if (__DEV__) {
251
+ currentHookNameInDev = undefined;
252
+ }
253
+
254
// These were reset above
255
// didScheduleRenderPhaseUpdate = false;
256
// renderPhaseUpdates = null;
@@ -247,6 +296,10 @@ export function resetHooks(): void {
296
remainingExpirationTime = NoWork;
297
componentUpdateQueue = null;
298
299
+ if (__DEV__) {
300
+ currentHookNameInDev = undefined;
301
+ }
302
+
303
didScheduleRenderPhaseUpdate = false;
304
renderPhaseUpdates = null;
305
numberOfReRenders = -1;
@@ -335,6 +388,9 @@ export function useContext<T>(
388
context: ReactContext<T>,
389
observedBits: void | number | boolean,
390
): T {
391
+ if (__DEV__) {
392
+ currentHookNameInDev = 'useContext';
393
+ }
394
// Ensure we're in a function component (class components support only the
395
// .unstable_read() form)
396
resolveCurrentlyRenderingFiber();
@@ -344,6 +400,9 @@ export function useContext<T>(
400
export function useState<S>(
401
initialState: (() => S) | S,
402
): [S, Dispatch<BasicStateAction<S>>] {
403
+ if (__DEV__) {
404
+ currentHookNameInDev = 'useState';
405
+ }
406
return useReducer(
407
basicStateReducer,
408
// useReducer has a special case to support lazy useState initializers
@@ -356,6 +415,11 @@ export function useReducer<S, A>(
415
initialState: S,
416
initialAction: A | void | null,
417
): [S, Dispatch<A>] {
418
+ if (__DEV__) {
419
+ if (reducer !== basicStateReducer) {
420
+ currentHookNameInDev = 'useReducer';
421
+ }
422
+ }
423
currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
424
workInProgressHook = createWorkInProgressHook();
425
let queue: UpdateQueue<S, A> | null = (workInProgressHook.queue: any);
@@ -500,12 +564,12 @@ export function useReducer<S, A>(
564
return [workInProgressHook.memoizedState, dispatch];
565
}
566
503
-function pushEffect(tag, create, destroy, inputs) {
567
+function pushEffect(tag, create, destroy, deps) {
568
const effect: Effect = {
569
tag,
570
create,
571
destroy,
508
- inputs,
572
+ deps,
573
// Circular
574
next: (null: any),
575
};
@@ -545,35 +609,44 @@ export function useRef<T>(initialValue: T): {current: T} {
609
610
export function useLayoutEffect(
611
create: () => mixed,
548
- inputs: Array<mixed> | void | null,
612
+ deps: Array<mixed> | void | null,
613
): void {
550
- useEffectImpl(UpdateEffect, UnmountMutation | MountLayout, create, inputs);
614
+ if (__DEV__) {
615
+ currentHookNameInDev = 'useLayoutEffect';
616
+ }
617
+ useEffectImpl(UpdateEffect, UnmountMutation | MountLayout, create, deps);
618
}
619
620
export function useEffect(
621
create: () => mixed,
555
- inputs: Array<mixed> | void | null,
622
+ deps: Array<mixed> | void | null,
623
): void {
624
+ if (__DEV__) {
625
+ currentHookNameInDev = 'useEffect';
626
+ }
627
useEffectImpl(
628
UpdateEffect | PassiveEffect,
629
UnmountPassive | MountPassive,
630
create,
561
- inputs,
631
+ deps,
632
);
633
}
634
565
-function useEffectImpl(fiberEffectTag, hookEffectTag, create, inputs): void {
635
+function useEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
636
currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
637
workInProgressHook = createWorkInProgressHook();
638
569
- let nextInputs = inputs !== undefined && inputs !== null ? inputs : [create];
639
+ const nextDeps = deps === undefined ? null : deps;
640
let destroy = null;
641
if (currentHook !== null) {
642
const prevEffect = currentHook.memoizedState;
643
destroy = prevEffect.destroy;
574
- if (areHookInputsEqual(nextInputs, prevEffect.inputs)) {
575
- pushEffect(NoHookEffect, create, destroy, nextInputs);
576
- return;
644
+ if (nextDeps !== null) {
645
+ const prevDeps = prevEffect.deps;
646
+ if (areHookInputsEqual(nextDeps, prevDeps)) {
647
+ pushEffect(NoHookEffect, create, destroy, nextDeps);
648
+ return;
649
+ }
650
}
651
}
652
@@ -582,20 +655,21 @@ function useEffectImpl(fiberEffectTag, hookEffectTag, create, inputs): void {
655
hookEffectTag,
656
create,
657
destroy,
585
- nextInputs,
658
+ nextDeps,
659
);
660
}
661
662
export function useImperativeHandle<T>(
663
ref: {current: T | null} | ((inst: T | null) => mixed) | null | void,
664
create: () => T,
592
- inputs: Array<mixed> | void | null,
665
+ deps: Array<mixed> | void | null,
666
): void {
594
- // TODO: If inputs are provided, should we skip comparing the ref itself?
595
- const nextInputs =
596
- inputs !== null && inputs !== undefined
597
- ? inputs.concat([ref])
598
- : [ref, create];
667
+ if (__DEV__) {
668
+ currentHookNameInDev = 'useImperativeHandle';
669
+ }
670
+ // TODO: If deps are provided, should we skip comparing the ref itself?
671
+ const nextDeps =
672
+ deps !== null && deps !== undefined ? deps.concat([ref]) : [ref];
673
674
// TODO: I've implemented this on top of useEffect because it's almost the
675
// same thing, and it would require an equal amount of code. It doesn't seem
@@ -614,13 +688,17 @@ export function useImperativeHandle<T>(
688
refObject.current = null;
689
};
690
}
617
- }, nextInputs);
691
+ }, nextDeps);
692
}
693
694
export function useDebugValue(
695
value: any,
696
formatterFn: ?(value: any) => any,
697
): void {
698
+ if (__DEV__) {
699
+ currentHookNameInDev = 'useDebugValue';
700
+ }
701
+
702
// This will trigger a warning if the hook is used in a non-Function component.
703
resolveCurrentlyRenderingFiber();
704
@@ -631,45 +709,54 @@ export function useDebugValue(
709
710
export function useCallback<T>(
711
callback: T,
634
- inputs: Array<mixed> | void | null,
712
+ deps: Array<mixed> | void | null,
713
): T {
714
+ if (__DEV__) {
715
+ currentHookNameInDev = 'useCallback';
716
+ }
717
currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
718
workInProgressHook = createWorkInProgressHook();
719
639
- const nextInputs =
640
- inputs !== undefined && inputs !== null ? inputs : [callback];
720
+ const nextDeps = deps === undefined ? null : deps;
721
722
const prevState = workInProgressHook.memoizedState;
723
if (prevState !== null) {
644
- const prevInputs = prevState[1];
645
- if (areHookInputsEqual(nextInputs, prevInputs)) {
646
- return prevState[0];
724
+ if (nextDeps !== null) {
725
+ const prevDeps: Array<mixed> | null = prevState[1];
726
+ if (areHookInputsEqual(nextDeps, prevDeps)) {
727
+ return prevState[0];
728
+ }
729
}
730
}
649
- workInProgressHook.memoizedState = [callback, nextInputs];
731
+ workInProgressHook.memoizedState = [callback, nextDeps];
732
return callback;
733
}
734
735
export function useMemo<T>(
736
nextCreate: () => T,
655
- inputs: Array<mixed> | void | null,
737
+ deps: Array<mixed> | void | null,
738
): T {
739
+ if (__DEV__) {
740
+ currentHookNameInDev = 'useMemo';
741
+ }
742
currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
743
workInProgressHook = createWorkInProgressHook();
744
660
- const nextInputs =
661
- inputs !== undefined && inputs !== null ? inputs : [nextCreate];
745
+ const nextDeps = deps === undefined ? null : deps;
746
747
const prevState = workInProgressHook.memoizedState;
748
if (prevState !== null) {
665
- const prevInputs = prevState[1];
666
- if (areHookInputsEqual(nextInputs, prevInputs)) {
667
- return prevState[0];
749
+ // Assume these are defined. If they're not, areHookInputsEqual will warn.
750
+ if (nextDeps !== null) {
751
+ const prevDeps: Array<mixed> | null = prevState[1];
752
+ if (areHookInputsEqual(nextDeps, prevDeps)) {
753
+ return prevState[0];
754
+ }
755
}
756
}
757
758
const nextValue = nextCreate();
672
- workInProgressHook.memoizedState = [nextValue, nextInputs];
759
+ workInProgressHook.memoizedState = [nextValue, nextDeps];
760
return nextValue;
761
}
762
packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js
+29
-5
@@ -447,12 +447,36 @@ describe('ReactHooks', () => {
447
expect(() => {
448
root.update(<App dependencies={['A', 'B']} />);
449
}).toWarnDev([
450
- 'Warning: Detected a variable number of hook dependencies. The length ' +
451
- 'of the dependencies array should be constant between renders.\n\n' +
452
- 'Previous: A, B\n' +
453
- 'Incoming: A',
450
+ 'Warning: The final argument passed to useLayoutEffect changed size ' +
451
+ 'between renders. The order and size of this array must remain ' +
452
+ 'constant.\n\n' +
453
+ 'Previous: [A, B]\n' +
454
+ 'Incoming: [A]\n',
455
+ ]);
456
+ });
457
+
458
+ it('warns if switching from dependencies to no dependencies', () => {
459
+ const {useMemo} = React;
460
+ function App({text, hasDeps}) {
461
+ const resolvedText = useMemo(() => {
462
+ ReactTestRenderer.unstable_yield('Compute');
463
+ return text.toUpperCase();
464
+ }, hasDeps ? null : [text]);
465
+ return resolvedText;
466
+ }
467
+
468
+ const root = ReactTestRenderer.create(null);
469
+ root.update(<App text="Hello" hasDeps={true} />);
470
+ expect(ReactTestRenderer).toHaveYielded(['Compute']);
471
+ expect(root).toMatchRenderedOutput('HELLO');
472
+
473
+ expect(() => {
474
+ root.update(<App text="Hello" hasDeps={false} />);
475
+ }).toWarnDev([
476
+ 'Warning: useMemo received a final argument during this render, but ' +
477
+ 'not during the previous render. Even though the final argument is ' +
478
+ 'optional, its type cannot change between renders.',
479
]);
455
- expect(ReactTestRenderer).toHaveYielded(['Did commit: A, B']);
480
});
481
482
it('warns for bad useEffect return values', () => {
packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
+10
-9
@@ -1015,11 +1015,11 @@ describe('ReactHooksWithNoopRenderer', () => {
1015
expect(ReactNoop.getChildren()).toEqual([]);
1016
});
1017
1018
- it('skips effect if constructor has not changed', () => {
1018
+ it('always fires effects if no dependencies are provided', () => {
1019
function effect() {
1020
- ReactNoop.yield(`Did mount`);
1020
+ ReactNoop.yield(`Did create`);
1021
return () => {
1022
- ReactNoop.yield(`Did unmount`);
1022
+ ReactNoop.yield(`Did destroy`);
1023
};
1024
}
1025
function Counter(props) {
@@ -1030,15 +1030,16 @@ describe('ReactHooksWithNoopRenderer', () => {
1030
expect(ReactNoop.flush()).toEqual(['Count: 0']);
1031
expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]);
1032
flushPassiveEffects();
1033
- expect(ReactNoop.clearYields()).toEqual(['Did mount']);
1033
+ expect(ReactNoop.clearYields()).toEqual(['Did create']);
1034
1035
ReactNoop.render(<Counter count={1} />);
1036
- // No effect, because constructor was hoisted outside render
1036
expect(ReactNoop.flush()).toEqual(['Count: 1']);
1037
expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]);
1038
+ flushPassiveEffects();
1039
+ expect(ReactNoop.clearYields()).toEqual(['Did destroy', 'Did create']);
1040
1041
ReactNoop.render(null);
1041
- expect(ReactNoop.flush()).toEqual(['Did unmount']);
1042
+ expect(ReactNoop.flush()).toEqual(['Did destroy']);
1043
expect(ReactNoop.getChildren()).toEqual([]);
1044
});
1045
@@ -1456,7 +1457,7 @@ describe('ReactHooksWithNoopRenderer', () => {
1457
expect(ReactNoop.getChildren()).toEqual([span('GOODBYE')]);
1458
});
1459
1459
- it('compares function if no inputs are provided', () => {
1460
+ it('always re-computes if no inputs are provided', () => {
1461
function LazyCompute(props) {
1462
const computed = useMemo(props.compute);
1463
return <Text text={computed} />;
@@ -1476,10 +1477,10 @@ describe('ReactHooksWithNoopRenderer', () => {
1477
expect(ReactNoop.flush()).toEqual(['compute A', 'A']);
1478
1479
ReactNoop.render(<LazyCompute compute={computeA} />);
1479
- expect(ReactNoop.flush()).toEqual(['A']);
1480
+ expect(ReactNoop.flush()).toEqual(['compute A', 'A']);
1481
1482
ReactNoop.render(<LazyCompute compute={computeA} />);
1482
- expect(ReactNoop.flush()).toEqual(['A']);
1483
+ expect(ReactNoop.flush()).toEqual(['compute A', 'A']);
1484
1485
ReactNoop.render(<LazyCompute compute={computeB} />);
1486
expect(ReactNoop.flush()).toEqual(['compute B', 'B']);
packages/shared/areHookInputsEqual.js
deleted
-34
@@ -1,34 +0,0 @@
1
-/**
2
- * Copyright (c) Facebook, Inc. and its affiliates.
3
- *
4
- * This source code is licensed under the MIT license found in the
5
- * LICENSE file in the root directory of this source tree.
6
- *
7
- * @flow
8
- */
9
-
10
-import warning from 'shared/warning';
11
-import is from './objectIs';
12
-
13
-export default function areHookInputsEqual(arr1: any[], arr2: any[]) {
14
- // Don't bother comparing lengths in prod because these arrays should be
15
- // passed inline.
16
- if (__DEV__) {
17
- warning(
18
- arr1.length === arr2.length,
19
- 'Detected a variable number of hook dependencies. The length of the ' +
20
- 'dependencies array should be constant between renders.\n\n' +
21
- 'Previous: %s\n' +
22
- 'Incoming: %s',
23
- arr1.join(', '),
24
- arr2.join(', '),
25
- );
26
- }
27
- for (let i = 0; i < arr1.length; i++) {
28
- if (is(arr1[i], arr2[i])) {
29
- continue;
30
- }
31
- return false;
32
- }
33
- return true;
34
-}