Skip to content

Commit ed34baf

Browse files
acdliteacdlite
committed
Improve hook dependencies warning
It now includes the name of the hook in the message.
1 parent c0d53ae commit ed34baf

File tree

4 files changed

+169
-64
lines changed

4 files changed

+169
-64
lines changed

‎packages/react-dom/src/server/ReactPartialRendererHooks.js

Lines changed: 60 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,12 @@
99

1010
import type {ThreadID} from './ReactThreadIDAllocator';
1111
import type {ReactContext} from 'shared/ReactTypes';
12-
import areHookInputsEqual from 'shared/areHookInputsEqual';
1312

1413
import {validateContextBounds} from './ReactPartialRendererContext';
1514

1615
import invariant from 'shared/invariant';
1716
import warning from 'shared/warning';
17+
import is from 'shared/objectIs';
1818

1919
type BasicStateAction<S> = (S => S) | S;
2020
type Dispatch<A> = A => void;
@@ -48,6 +48,9 @@ let renderPhaseUpdates: Map<UpdateQueue<any>, Update<any>> | null = null;
4848
let numberOfReRenders: number = 0;
4949
const RE_RENDER_LIMIT = 25;
5050

51+
// In DEV, this is the name of the currently executing primitive hook
52+
let currentHookNameInDev: ?string;
53+
5154
function resolveCurrentlyRenderingComponent(): Object {
5255
invariant(
5356
currentlyRenderingComponent !== null,
@@ -56,6 +59,48 @@ function resolveCurrentlyRenderingComponent(): Object {
5659
return currentlyRenderingComponent;
5760
}
5861

62+
function areHookInputsEqual(
63+
nextDeps: Array<mixed>,
64+
prevDeps: Array<mixed> | null,
65+
) {
66+
if (prevDeps === null) {
67+
if (__DEV__) {
68+
warning(
69+
false,
70+
'%s received a final argument during this render, but not during ' +
71+
'the previous render. Even though the final argument is optional, ' +
72+
'its type cannot change between renders.',
73+
currentHookNameInDev,
74+
);
75+
}
76+
return false;
77+
}
78+
79+
if (__DEV__) {
80+
// Don't bother comparing lengths in prod because these arrays should be
81+
// passed inline.
82+
if (nextDeps.length !== prevDeps.length) {
83+
warning(
84+
false,
85+
'The final argument passed to %s changed size between renders. The ' +
86+
'order and size of this array must remain constant.\n\n' +
87+
'Previous: %s\n' +
88+
'Incoming: %s',
89+
currentHookNameInDev,
90+
`[${nextDeps.join(', ')}]`,
91+
`[${prevDeps.join(', ')}]`,
92+
);
93+
}
94+
}
95+
for (let i = 0; i < nextDeps.length; i++) {
96+
if (is(nextDeps[i], prevDeps[i])) {
97+
continue;
98+
}
99+
return false;
100+
}
101+
return true;
102+
}
103+
59104
function createHook(): Hook {
60105
return {
61106
memoizedState: null,
@@ -152,6 +197,9 @@ function useContext<T>(
152197
context: ReactContext<T>,
153198
observedBits: void | number | boolean,
154199
): T {
200+
if (__DEV__) {
201+
currentHookNameInDev = 'useContext';
202+
}
155203
resolveCurrentlyRenderingComponent();
156204
let threadID = currentThreadID;
157205
validateContextBounds(context, threadID);
@@ -165,6 +213,9 @@ function basicStateReducer<S>(state: S, action: BasicStateAction<S>): S {
165213
export function useState<S>(
166214
initialState: (() => S) | S,
167215
): [S, Dispatch<BasicStateAction<S>>] {
216+
if (__DEV__) {
217+
currentHookNameInDev = 'useState';
218+
}
168219
return useReducer(
169220
basicStateReducer,
170221
// useReducer has a special case to support lazy useState initializers
@@ -177,6 +228,11 @@ export function useReducer<S, A>(
177228
initialState: S,
178229
initialAction: A | void | null,
179230
): [S, Dispatch<A>] {
231+
if (__DEV__) {
232+
if (reducer !== basicStateReducer) {
233+
currentHookNameInDev = 'useReducer';
234+
}
235+
}
180236
currentlyRenderingComponent = resolveCurrentlyRenderingComponent();
181237
workInProgressHook = createWorkInProgressHook();
182238
if (isReRender) {
@@ -275,6 +331,9 @@ export function useLayoutEffect(
275331
create: () => mixed,
276332
inputs: Array<mixed> | void | null,
277333
) {
334+
if (__DEV__) {
335+
currentHookNameInDev = 'useLayoutEffect';
336+
}
278337
warning(
279338
false,
280339
'useLayoutEffect does nothing on the server, because its effect cannot ' +

‎packages/react-reconciler/src/ReactFiberHooks.js

Lines changed: 100 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,8 @@ import {
3434
} from './ReactFiberScheduler';
3535

3636
import invariant from 'shared/invariant';
37-
import areHookInputsEqual from 'shared/areHookInputsEqual';
37+
import warning from 'shared/warning';
38+
import is from 'shared/objectIs';
3839

3940
type Update<A> = {
4041
expirationTime: ExpirationTime,
@@ -109,6 +110,9 @@ let renderPhaseUpdates: Map<UpdateQueue<any>, Update<any>> | null = null;
109110
let numberOfReRenders: number = 0;
110111
const RE_RENDER_LIMIT = 25;
111112

113+
// In DEV, this is the name of the currently executing primitive hook
114+
let currentHookNameInDev: ?string;
115+
112116
function resolveCurrentlyRenderingFiber(): Fiber {
113117
invariant(
114118
currentlyRenderingFiber !== null,
@@ -117,6 +121,48 @@ function resolveCurrentlyRenderingFiber(): Fiber {
117121
return currentlyRenderingFiber;
118122
}
119123

124+
function areHookInputsEqual(
125+
nextDeps: Array<mixed>,
126+
prevDeps: Array<mixed> | null,
127+
) {
128+
if (prevDeps === null) {
129+
if (__DEV__) {
130+
warning(
131+
false,
132+
'%s received a final argument during this render, but not during ' +
133+
'the previous render. Even though the final argument is optional, ' +
134+
'its type cannot change between renders.',
135+
currentHookNameInDev,
136+
);
137+
}
138+
return false;
139+
}
140+
141+
if (__DEV__) {
142+
// Don't bother comparing lengths in prod because these arrays should be
143+
// passed inline.
144+
if (nextDeps.length !== prevDeps.length) {
145+
warning(
146+
false,
147+
'The final argument passed to %s changed size between renders. The ' +
148+
'order and size of this array must remain constant.\n\n' +
149+
'Previous: %s\n' +
150+
'Incoming: %s',
151+
currentHookNameInDev,
152+
`[${nextDeps.join(', ')}]`,
153+
`[${prevDeps.join(', ')}]`,
154+
);
155+
}
156+
}
157+
for (let i = 0; i < nextDeps.length; i++) {
158+
if (is(nextDeps[i], prevDeps[i])) {
159+
continue;
160+
}
161+
return false;
162+
}
163+
return true;
164+
}
165+
120166
export function prepareToUseHooks(
121167
current: Fiber | null,
122168
workInProgress: Fiber,
@@ -193,6 +239,10 @@ export function finishHooks(
193239
remainingExpirationTime = NoWork;
194240
componentUpdateQueue = null;
195241

242+
if (__DEV__) {
243+
currentHookNameInDev = undefined;
244+
}
245+
196246
// Always set during createWorkInProgress
197247
// isReRender = false;
198248

@@ -229,6 +279,10 @@ export function resetHooks(): void {
229279
remainingExpirationTime = NoWork;
230280
componentUpdateQueue = null;
231281

282+
if (__DEV__) {
283+
currentHookNameInDev = undefined;
284+
}
285+
232286
// Always set during createWorkInProgress
233287
// isReRender = false;
234288

@@ -324,6 +378,9 @@ export function useContext<T>(
324378
context: ReactContext<T>,
325379
observedBits: void | number | boolean,
326380
): T {
381+
if (__DEV__) {
382+
currentHookNameInDev = 'useContext';
383+
}
327384
// Ensure we're in a function component (class components support only the
328385
// .unstable_read() form)
329386
resolveCurrentlyRenderingFiber();
@@ -333,6 +390,9 @@ export function useContext<T>(
333390
export function useState<S>(
334391
initialState: (() => S) | S,
335392
): [S, Dispatch<BasicStateAction<S>>] {
393+
if (__DEV__) {
394+
currentHookNameInDev = 'useState';
395+
}
336396
return useReducer(
337397
basicStateReducer,
338398
// useReducer has a special case to support lazy useState initializers
@@ -345,6 +405,11 @@ export function useReducer<S, A>(
345405
initialState: S,
346406
initialAction: A | void | null,
347407
): [S, Dispatch<A>] {
408+
if (__DEV__) {
409+
if (reducer !== basicStateReducer) {
410+
currentHookNameInDev = 'useReducer';
411+
}
412+
}
348413
currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
349414
workInProgressHook = createWorkInProgressHook();
350415
let queue: UpdateQueue<A> | null = (workInProgressHook.queue: any);
@@ -518,13 +583,19 @@ export function useLayoutEffect(
518583
create: () => mixed,
519584
deps: Array<mixed> | void | null,
520585
): void {
586+
if (__DEV__) {
587+
currentHookNameInDev = 'useLayoutEffect';
588+
}
521589
useEffectImpl(UpdateEffect, UnmountMutation | MountLayout, create, deps);
522590
}
523591

524592
export function useEffect(
525593
create: () => mixed,
526594
deps: Array<mixed> | void | null,
527595
): void {
596+
if (__DEV__) {
597+
currentHookNameInDev = 'useEffect';
598+
}
528599
useEffectImpl(
529600
UpdateEffect | PassiveEffect,
530601
UnmountPassive | MountPassive,
@@ -542,11 +613,12 @@ function useEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
542613
if (currentHook !== null) {
543614
const prevEffect = currentHook.memoizedState;
544615
destroy = prevEffect.destroy;
545-
// Assume these are defined. If they're not, areHookInputsEqual will warn.
546-
const prevDeps: Array<mixed> = (prevEffect.deps: any);
547-
if (nextDeps !== null && areHookInputsEqual(nextDeps, prevDeps)) {
548-
pushEffect(NoHookEffect, create, destroy, nextDeps);
549-
return;
616+
if (nextDeps !== null) {
617+
const prevDeps = prevEffect.deps;
618+
if (areHookInputsEqual(nextDeps, prevDeps)) {
619+
pushEffect(NoHookEffect, create, destroy, nextDeps);
620+
return;
621+
}
550622
}
551623
}
552624

@@ -564,6 +636,9 @@ export function useImperativeHandle<T>(
564636
create: () => T,
565637
deps: Array<mixed> | void | null,
566638
): void {
639+
if (__DEV__) {
640+
currentHookNameInDev = 'useImperativeHandle';
641+
}
567642
// TODO: If deps are provided, should we skip comparing the ref itself?
568643
const nextDeps =
569644
deps !== null && deps !== undefined ? deps.concat([ref]) : [ref];
@@ -592,6 +667,9 @@ export function useDebugValue(
592667
value: any,
593668
formatterFn: ?(value: any) => any,
594669
): void {
670+
if (__DEV__) {
671+
currentHookNameInDev = 'useDebugValue';
672+
}
595673
// This hook is normally a no-op.
596674
// The react-debug-hooks package injects its own implementation
597675
// so that e.g. DevTools can display custom hook values.
@@ -601,17 +679,21 @@ export function useCallback<T>(
601679
callback: T,
602680
deps: Array<mixed> | void | null,
603681
): T {
682+
if (__DEV__) {
683+
currentHookNameInDev = 'useCallback';
684+
}
604685
currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
605686
workInProgressHook = createWorkInProgressHook();
606687

607688
const nextDeps = deps === undefined ? null : deps;
608689

609690
const prevState = workInProgressHook.memoizedState;
610691
if (prevState !== null) {
611-
// Assume these are defined. If they're not, areHookInputsEqual will warn.
612-
const prevDeps: Array<mixed> = prevState[1];
613-
if (nextDeps !== null && areHookInputsEqual(nextDeps, prevDeps)) {
614-
return prevState[0];
692+
if (nextDeps !== null) {
693+
const prevDeps: Array<mixed> | null = prevState[1];
694+
if (areHookInputsEqual(nextDeps, prevDeps)) {
695+
return prevState[0];
696+
}
615697
}
616698
}
617699
workInProgressHook.memoizedState = [callback, nextDeps];
@@ -622,6 +704,9 @@ export function useMemo<T>(
622704
nextCreate: () => T,
623705
deps: Array<mixed> | void | null,
624706
): T {
707+
if (__DEV__) {
708+
currentHookNameInDev = 'useMemo';
709+
}
625710
currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
626711
workInProgressHook = createWorkInProgressHook();
627712

@@ -630,9 +715,11 @@ export function useMemo<T>(
630715
const prevState = workInProgressHook.memoizedState;
631716
if (prevState !== null) {
632717
// Assume these are defined. If they're not, areHookInputsEqual will warn.
633-
const prevDeps = prevState[1];
634-
if (nextDeps !== null && areHookInputsEqual(nextDeps, prevDeps)) {
635-
return prevState[0];
718+
if (nextDeps !== null) {
719+
const prevDeps: Array<mixed> | null = prevState[1];
720+
if (areHookInputsEqual(nextDeps, prevDeps)) {
721+
return prevState[0];
722+
}
636723
}
637724
}
638725

‎packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -46,17 +46,16 @@ describe('ReactHooks', () => {
4646
expect(() => {
4747
root.update(<App dependencies={['A', 'B']} />);
4848
}).toWarnDev([
49-
'Warning: Detected a variable number of hook dependencies. The length ' +
50-
'of the dependencies array should be constant between renders.\n\n' +
49+
'Warning: The final argument passed to useLayoutEffect changed size ' +
50+
'between renders. The order and size of this array must remain ' +
51+
'constant.\n\n' +
5152
'Previous: [A, B]\n' +
52-
'Incoming: [A]',
53+
'Incoming: [A]\n',
5354
]);
5455
expect(ReactTestRenderer).toHaveYielded(['Did commit: A, B']);
5556
});
5657

5758
it('warns if switching from dependencies to no dependencies', () => {
58-
spyOnDev(console, 'error');
59-
6059
const {useMemo} = React;
6160
function App({text, hasDeps}) {
6261
const resolvedText = useMemo(() => {
@@ -72,16 +71,12 @@ describe('ReactHooks', () => {
7271
expect(root).toMatchRenderedOutput('HELLO');
7372

7473
expect(() => {
75-
// This prints a warning message then throws a null access error
7674
root.update(<App text="Hello" hasDeps={false} />);
77-
}).toThrow('null');
78-
79-
if (__DEV__) {
80-
expect(console.error).toHaveBeenCalledTimes(3);
81-
expect(console.error.calls.argsFor(0)[0]).toContain(
82-
'Warning: Detected a variable number of hook dependencies.',
83-
);
84-
}
75+
}).toWarnDev([
76+
'Warning: useMemo received a final argument during this render, but ' +
77+
'not during the previous render. Even though the final argument is ' +
78+
'optional, its type cannot change between renders.',
79+
]);
8580
});
8681

8782
it('warns for bad useEffect return values', () => {

0 commit comments

Comments
 (0)