Skip to content

Commit

Permalink
Improve hook dependencies warning
Browse files Browse the repository at this point in the history 
It now includes the name of the hook in the message.
  • Loading branch information
@acdlite
acdlite committed Jan 15, 2019
1 parent c0d53ae commit ed34baf
Show file tree
Hide file tree
Showing 4 changed files with 169 additions and 64 deletions.
61 changes: 60 additions & 1 deletion packages/react-dom/src/server/ReactPartialRendererHooks.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@

import type {ThreadID} from './ReactThreadIDAllocator';
import type {ReactContext} from 'shared/ReactTypes';
import areHookInputsEqual from 'shared/areHookInputsEqual';

import {validateContextBounds} from './ReactPartialRendererContext';

import invariant from 'shared/invariant';
import warning from 'shared/warning';
import is from 'shared/objectIs';

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

// In DEV, this is the name of the currently executing primitive hook
let currentHookNameInDev: ?string;

function resolveCurrentlyRenderingComponent(): Object {
invariant(
currentlyRenderingComponent !== null,
Expand All @@ -56,6 +59,48 @@ function resolveCurrentlyRenderingComponent(): Object {
return currentlyRenderingComponent;
}

function areHookInputsEqual(
nextDeps: Array<mixed>,
prevDeps: Array<mixed> | null,
) {
if (prevDeps === null) {
if (__DEV__) {
warning(
false,
'%s received a final argument during this render, but not during ' +
'the previous render. Even though the final argument is optional, ' +
'its type cannot change between renders.',
currentHookNameInDev,
);
}
return false;
}

if (__DEV__) {
// Don't bother comparing lengths in prod because these arrays should be
// passed inline.
if (nextDeps.length !== prevDeps.length) {
warning(
false,
'The final argument passed to %s changed size between renders. The ' +
'order and size of this array must remain constant.\n\n' +
'Previous: %s\n' +
'Incoming: %s',
currentHookNameInDev,
`[${nextDeps.join(', ')}]`,
`[${prevDeps.join(', ')}]`,
);
}
}
for (let i = 0; i < nextDeps.length; i++) {
if (is(nextDeps[i], prevDeps[i])) {
continue;
}
return false;
}
return true;
}

function createHook(): Hook {
return {
memoizedState: null,
Expand Down Expand Up @@ -152,6 +197,9 @@ function useContext<T>(
context: ReactContext<T>,
observedBits: void | number | boolean,
): T {
if (__DEV__) {
currentHookNameInDev = 'useContext';
}
resolveCurrentlyRenderingComponent();
let threadID = currentThreadID;
validateContextBounds(context, threadID);
Expand All @@ -165,6 +213,9 @@ function basicStateReducer<S>(state: S, action: BasicStateAction<S>): S {
export function useState<S>(
initialState: (() => S) | S,
): [S, Dispatch<BasicStateAction<S>>] {
if (__DEV__) {
currentHookNameInDev = 'useState';
}
return useReducer(
basicStateReducer,
// useReducer has a special case to support lazy useState initializers
Expand All @@ -177,6 +228,11 @@ export function useReducer<S, A>(
initialState: S,
initialAction: A | void | null,
): [S, Dispatch<A>] {
if (__DEV__) {
if (reducer !== basicStateReducer) {
currentHookNameInDev = 'useReducer';
}
}
currentlyRenderingComponent = resolveCurrentlyRenderingComponent();
workInProgressHook = createWorkInProgressHook();
if (isReRender) {
Expand Down Expand Up @@ -275,6 +331,9 @@ export function useLayoutEffect(
create: () => mixed,
inputs: Array<mixed> | void | null,
) {
if (__DEV__) {
currentHookNameInDev = 'useLayoutEffect';
}
warning(
false,
'useLayoutEffect does nothing on the server, because its effect cannot ' +
Expand Down
113 changes: 100 additions & 13 deletions packages/react-reconciler/src/ReactFiberHooks.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ import {
} from './ReactFiberScheduler';

import invariant from 'shared/invariant';
import areHookInputsEqual from 'shared/areHookInputsEqual';
import warning from 'shared/warning';
import is from 'shared/objectIs';

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

// In DEV, this is the name of the currently executing primitive hook
let currentHookNameInDev: ?string;

function resolveCurrentlyRenderingFiber(): Fiber {
invariant(
currentlyRenderingFiber !== null,
Expand All @@ -117,6 +121,48 @@ function resolveCurrentlyRenderingFiber(): Fiber {
return currentlyRenderingFiber;
}

function areHookInputsEqual(
nextDeps: Array<mixed>,
prevDeps: Array<mixed> | null,
) {
if (prevDeps === null) {
if (__DEV__) {
warning(
false,
'%s received a final argument during this render, but not during ' +
'the previous render. Even though the final argument is optional, ' +
'its type cannot change between renders.',
currentHookNameInDev,
);
}
return false;
}

if (__DEV__) {
// Don't bother comparing lengths in prod because these arrays should be
// passed inline.
if (nextDeps.length !== prevDeps.length) {
warning(
false,
'The final argument passed to %s changed size between renders. The ' +
'order and size of this array must remain constant.\n\n' +
'Previous: %s\n' +
'Incoming: %s',
currentHookNameInDev,
`[${nextDeps.join(', ')}]`,
`[${prevDeps.join(', ')}]`,
);
}
}
for (let i = 0; i < nextDeps.length; i++) {
if (is(nextDeps[i], prevDeps[i])) {
continue;
}
return false;
}
return true;
}

export function prepareToUseHooks(
current: Fiber | null,
workInProgress: Fiber,
Expand Down Expand Up @@ -193,6 +239,10 @@ export function finishHooks(
remainingExpirationTime = NoWork;
componentUpdateQueue = null;

if (__DEV__) {
currentHookNameInDev = undefined;
}

// Always set during createWorkInProgress
// isReRender = false;

Expand Down Expand Up @@ -229,6 +279,10 @@ export function resetHooks(): void {
remainingExpirationTime = NoWork;
componentUpdateQueue = null;

if (__DEV__) {
currentHookNameInDev = undefined;
}

// Always set during createWorkInProgress
// isReRender = false;

Expand Down Expand Up @@ -324,6 +378,9 @@ export function useContext<T>(
context: ReactContext<T>,
observedBits: void | number | boolean,
): T {
if (__DEV__) {
currentHookNameInDev = 'useContext';
}
// Ensure we're in a function component (class components support only the
// .unstable_read() form)
resolveCurrentlyRenderingFiber();
Expand All @@ -333,6 +390,9 @@ export function useContext<T>(
export function useState<S>(
initialState: (() => S) | S,
): [S, Dispatch<BasicStateAction<S>>] {
if (__DEV__) {
currentHookNameInDev = 'useState';
}
return useReducer(
basicStateReducer,
// useReducer has a special case to support lazy useState initializers
Expand All @@ -345,6 +405,11 @@ export function useReducer<S, A>(
initialState: S,
initialAction: A | void | null,
): [S, Dispatch<A>] {
if (__DEV__) {
if (reducer !== basicStateReducer) {
currentHookNameInDev = 'useReducer';
}
}
currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
workInProgressHook = createWorkInProgressHook();
let queue: UpdateQueue<A> | null = (workInProgressHook.queue: any);
Expand Down Expand Up @@ -518,13 +583,19 @@ export function useLayoutEffect(
create: () => mixed,
deps: Array<mixed> | void | null,
): void {
if (__DEV__) {
currentHookNameInDev = 'useLayoutEffect';
}
useEffectImpl(UpdateEffect, UnmountMutation | MountLayout, create, deps);
}

export function useEffect(
create: () => mixed,
deps: Array<mixed> | void | null,
): void {
if (__DEV__) {
currentHookNameInDev = 'useEffect';
}
useEffectImpl(
UpdateEffect | PassiveEffect,
UnmountPassive | MountPassive,
Expand All @@ -542,11 +613,12 @@ function useEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
if (currentHook !== null) {
const prevEffect = currentHook.memoizedState;
destroy = prevEffect.destroy;
// Assume these are defined. If they're not, areHookInputsEqual will warn.
const prevDeps: Array<mixed> = (prevEffect.deps: any);
if (nextDeps !== null && areHookInputsEqual(nextDeps, prevDeps)) {
pushEffect(NoHookEffect, create, destroy, nextDeps);
return;
if (nextDeps !== null) {
const prevDeps = prevEffect.deps;
if (areHookInputsEqual(nextDeps, prevDeps)) {
pushEffect(NoHookEffect, create, destroy, nextDeps);
return;
}
}
}

Expand All @@ -564,6 +636,9 @@ export function useImperativeHandle<T>(
create: () => T,
deps: Array<mixed> | void | null,
): void {
if (__DEV__) {
currentHookNameInDev = 'useImperativeHandle';
}
// TODO: If deps are provided, should we skip comparing the ref itself?
const nextDeps =
deps !== null && deps !== undefined ? deps.concat([ref]) : [ref];
Expand Down Expand Up @@ -592,6 +667,9 @@ export function useDebugValue(
value: any,
formatterFn: ?(value: any) => any,
): void {
if (__DEV__) {
currentHookNameInDev = 'useDebugValue';
}
// This hook is normally a no-op.
// The react-debug-hooks package injects its own implementation
// so that e.g. DevTools can display custom hook values.
Expand All @@ -601,17 +679,21 @@ export function useCallback<T>(
callback: T,
deps: Array<mixed> | void | null,
): T {
if (__DEV__) {
currentHookNameInDev = 'useCallback';
}
currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
workInProgressHook = createWorkInProgressHook();

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

const prevState = workInProgressHook.memoizedState;
if (prevState !== null) {
// Assume these are defined. If they're not, areHookInputsEqual will warn.
const prevDeps: Array<mixed> = prevState[1];
if (nextDeps !== null && areHookInputsEqual(nextDeps, prevDeps)) {
return prevState[0];
if (nextDeps !== null) {
const prevDeps: Array<mixed> | null = prevState[1];
if (areHookInputsEqual(nextDeps, prevDeps)) {
return prevState[0];
}
}
}
workInProgressHook.memoizedState = [callback, nextDeps];
Expand All @@ -622,6 +704,9 @@ export function useMemo<T>(
nextCreate: () => T,
deps: Array<mixed> | void | null,
): T {
if (__DEV__) {
currentHookNameInDev = 'useMemo';
}
currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
workInProgressHook = createWorkInProgressHook();

Expand All @@ -630,9 +715,11 @@ export function useMemo<T>(
const prevState = workInProgressHook.memoizedState;
if (prevState !== null) {
// Assume these are defined. If they're not, areHookInputsEqual will warn.
const prevDeps = prevState[1];
if (nextDeps !== null && areHookInputsEqual(nextDeps, prevDeps)) {
return prevState[0];
if (nextDeps !== null) {
const prevDeps: Array<mixed> | null = prevState[1];
if (areHookInputsEqual(nextDeps, prevDeps)) {
return prevState[0];
}
}
}

Expand Down
23 changes: 9 additions & 14 deletions packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,17 +46,16 @@ describe('ReactHooks', () => {
expect(() => {
root.update(<App dependencies={['A', 'B']} />);
}).toWarnDev([
'Warning: Detected a variable number of hook dependencies. The length ' +
'of the dependencies array should be constant between renders.\n\n' +
'Warning: The final argument passed to useLayoutEffect changed size ' +
'between renders. The order and size of this array must remain ' +
'constant.\n\n' +
'Previous: [A, B]\n' +
'Incoming: [A]',
'Incoming: [A]\n',
]);
expect(ReactTestRenderer).toHaveYielded(['Did commit: A, B']);
});

it('warns if switching from dependencies to no dependencies', () => {
spyOnDev(console, 'error');

const {useMemo} = React;
function App({text, hasDeps}) {
const resolvedText = useMemo(() => {
Expand All @@ -72,16 +71,12 @@ describe('ReactHooks', () => {
expect(root).toMatchRenderedOutput('HELLO');

expect(() => {
// This prints a warning message then throws a null access error
root.update(<App text="Hello" hasDeps={false} />);
}).toThrow('null');

if (__DEV__) {
expect(console.error).toHaveBeenCalledTimes(3);
expect(console.error.calls.argsFor(0)[0]).toContain(
'Warning: Detected a variable number of hook dependencies.',
);
}
}).toWarnDev([
'Warning: useMemo received a final argument during this render, but ' +
'not during the previous render. Even though the final argument is ' +
'optional, its type cannot change between renders.',
]);
});

it('warns for bad useEffect return values', () => {
Expand Down
Loading

0 comments on commit ed34baf

Please sign in to comment.