Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: warn when returning different hooks on subsequent renders #14585

Merged
merged 41 commits into from
Jan 22, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
4d8a8cf
warn when returning different hooks on next render
Jan 14, 2019
0daee73
lint
Jan 14, 2019
623fb58
review changes
Jan 14, 2019
1204a0c
cleaner detection location
Jan 14, 2019
d597fcd
redundant comments
Jan 14, 2019
1c7314f
different EffectHook / LayoutEffectHook
Jan 14, 2019
5221e91
prettier
Jan 14, 2019
bcbce28
top level currentHookType
Jan 14, 2019
a604ea1
nulling currentHookType
Jan 14, 2019
8b22c87
small enhancements
Jan 17, 2019
95aa003
hook order checks for useContext/useImperative
Jan 17, 2019
cbf4068
Merge branch 'master' into wrong-hook-order
Jan 18, 2019
4676b6b
prettier
Jan 18, 2019
5a94b54
stray whitespace
Jan 18, 2019
aca87cd
move some bits around
Jan 18, 2019
428a3eb
better errors
Jan 18, 2019
6cd966c
pass tests
Jan 18, 2019
13debbb
lint, flow
Jan 18, 2019
65ebacb
Merge remote-tracking branch 'upstream/master' into wrong-hook-order
Jan 18, 2019
9bd7051
show a before - after diff
Jan 18, 2019
0eea976
Merge remote-tracking branch 'upstream/master' into wrong-hook-order
Jan 18, 2019
f09966d
an error stack in the warning
Jan 18, 2019
e366069
lose currentHookMatches, fix a test
Jan 19, 2019
d8c4236
tidy
Jan 19, 2019
4801b12
clear the mismatch only in dev
Jan 19, 2019
c77e448
pass flow
Jan 19, 2019
b6fb3ca
side by side diff
Jan 21, 2019
055ebf6
tweak warning
Jan 21, 2019
c7a872b
Merge remote-tracking branch 'upstream/master' into wrong-hook-order
Jan 21, 2019
e06b72b
pass flow
Jan 21, 2019
190e9b1
dedupe warnings per fiber, nits
Jan 21, 2019
f37f4fd
better format
Jan 21, 2019
26fd179
Merge remote-tracking branch 'upstream/master' into wrong-hook-order
Jan 21, 2019
3e8a217
nit
Jan 21, 2019
c2d686a
Merge remote-tracking branch 'upstream/master' into wrong-hook-order
Jan 21, 2019
f493675
fix bad merge, pass flow
Jan 22, 2019
a985e72
lint
Jan 22, 2019
1621a2d
missing hooktype enum
Jan 22, 2019
5ecda4c
merge currentHookType/currentHookNameInDev, fix nits
Jan 22, 2019
d4860ca
lint
Jan 22, 2019
3ad6523
final nits
Jan 22, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
205 changes: 187 additions & 18 deletions packages/react-reconciler/src/ReactFiberHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,27 @@ type UpdateQueue<S, A> = {
eagerState: S | null,
};

type HookType =
| 'useState'
| 'useReducer'
| 'useContext'
| 'useRef'
| 'useEffect'
| 'useLayoutEffect'
| 'useCallback'
| 'useMemo'
| 'useImperativeHandle'
| 'useDebugValue';

// the first instance of a hook mismatch in a component,
// represented by a portion of it's stacktrace
let currentHookMismatch = null;

let didWarnAboutMismatchedHooksForComponent;
if (__DEV__) {
didWarnAboutMismatchedHooksForComponent = new Set();
}

export type Hook = {
memoizedState: any,

Expand All @@ -64,6 +85,10 @@ export type Hook = {
next: Hook | null,
};

type HookDev = Hook & {
_debugType: HookType,
};

type Effect = {
tag: HookEffectTag,
create: () => mixed,
Expand Down Expand Up @@ -118,7 +143,7 @@ let numberOfReRenders: number = -1;
const RE_RENDER_LIMIT = 25;

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

function resolveCurrentlyRenderingFiber(): Fiber {
invariant(
Expand Down Expand Up @@ -170,6 +195,95 @@ function areHookInputsEqual(
return true;
}

// till we have String::padEnd, a small function to
// right-pad strings with spaces till a minimum length
function padEndSpaces(string: string, length: number) {
threepointone marked this conversation as resolved.
Show resolved Hide resolved
if (__DEV__) {
if (string.length >= length) {
return string;
} else {
return string + ' ' + new Array(length - string.length).join(' ');
}
}
}

function flushHookMismatchWarnings() {
// we'll show the diff of the low level hooks,
// and a stack trace so the dev can locate where
// the first mismatch is coming from
if (__DEV__) {
if (currentHookMismatch !== null) {
let componentName = getComponentName(
((currentlyRenderingFiber: any): Fiber).type,
);
if (!didWarnAboutMismatchedHooksForComponent.has(componentName)) {
didWarnAboutMismatchedHooksForComponent.add(componentName);
const hookStackDiff = [];
let current = firstCurrentHook;
const previousOrder = [];
while (current !== null) {
previousOrder.push(((current: any): HookDev)._debugType);
current = current.next;
}
let workInProgress = firstWorkInProgressHook;
const nextOrder = [];
while (workInProgress !== null) {
nextOrder.push(((workInProgress: any): HookDev)._debugType);
workInProgress = workInProgress.next;
}
// some bookkeeping for formatting the output table
const columnLength = Math.max.apply(
null,
previousOrder
.map(hook => hook.length)
.concat(' Previous render'.length),
);

let hookStackHeader =
((padEndSpaces(' Previous render', columnLength): any): string) +
' Next render\n';
const hookStackWidth = hookStackHeader.length;
hookStackHeader += ' ' + new Array(hookStackWidth - 2).join('-');
const hookStackFooter = ' ' + new Array(hookStackWidth - 2).join('^');

const hookStackLength = Math.max(
previousOrder.length,
nextOrder.length,
);
for (let i = 0; i < hookStackLength; i++) {
hookStackDiff.push(
((padEndSpaces(i + 1 + '. ', 3): any): string) +
((padEndSpaces(previousOrder[i], columnLength): any): string) +
' ' +
nextOrder[i],
);
if (previousOrder[i] !== nextOrder[i]) {
break;
}
}
warning(
false,
'React has detected a change in the order of Hooks called by %s. ' +
'This will lead to bugs and errors if not fixed. ' +
'For more information, read the Rules of Hooks: https://fb.me/rules-of-hooks\n\n' +
'%s\n' +
'%s\n' +
'%s\n' +
'The first Hook type mismatch occured at:\n' +
'%s\n\n' +
'This error occurred in the following component:',
componentName,
hookStackHeader,
hookStackDiff.join('\n'),
hookStackFooter,
currentHookMismatch,
);
}
currentHookMismatch = null;
}
}
}

export function renderWithHooks(
current: Fiber | null,
workInProgress: Fiber,
Expand Down Expand Up @@ -221,6 +335,7 @@ export function renderWithHooks(
getComponentName(Component),
);
}
flushHookMismatchWarnings();
}
} while (didScheduleRenderPhaseUpdate);

Expand Down Expand Up @@ -248,7 +363,7 @@ export function renderWithHooks(
componentUpdateQueue = null;

if (__DEV__) {
currentHookNameInDev = undefined;
currentHookNameInDev = null;
}

// These were reset above
Expand Down Expand Up @@ -281,6 +396,9 @@ export function resetHooks(): void {
if (!enableHooks) {
return;
}
if (__DEV__) {
flushHookMismatchWarnings();
}

// This is used to reset the state of this module when a component throws.
// It's also called inside mountIndeterminateComponent if we determine the
Expand All @@ -297,7 +415,7 @@ export function resetHooks(): void {
componentUpdateQueue = null;

if (__DEV__) {
currentHookNameInDev = undefined;
currentHookNameInDev = null;
}

didScheduleRenderPhaseUpdate = false;
Expand All @@ -306,27 +424,63 @@ export function resetHooks(): void {
}

function createHook(): Hook {
return {
memoizedState: null,
let hook: Hook = __DEV__
? {
threepointone marked this conversation as resolved.
Show resolved Hide resolved
_debugType: ((currentHookNameInDev: any): HookType),
memoizedState: null,

baseState: null,
queue: null,
baseUpdate: null,
baseState: null,
queue: null,
baseUpdate: null,

next: null,
};
next: null,
}
: {
memoizedState: null,

baseState: null,
queue: null,
baseUpdate: null,

next: null,
};

return hook;
threepointone marked this conversation as resolved.
Show resolved Hide resolved
}

function cloneHook(hook: Hook): Hook {
return {
memoizedState: hook.memoizedState,
let nextHook: Hook = __DEV__
? {
_debugType: ((currentHookNameInDev: any): HookType),
memoizedState: hook.memoizedState,

baseState: hook.baseState,
queue: hook.queue,
baseUpdate: hook.baseUpdate,
baseState: hook.baseState,
queue: hook.queue,
baseUpdate: hook.baseUpdate,

next: null,
};
next: null,
}
: {
memoizedState: hook.memoizedState,

baseState: hook.baseState,
queue: hook.queue,
baseUpdate: hook.baseUpdate,

next: null,
};

if (__DEV__) {
if (!currentHookMismatch) {
if (currentHookNameInDev !== ((hook: any): HookDev)._debugType) {
currentHookMismatch = new Error('tracer').stack
.split('\n')
.slice(4)
.join('\n');
}
}
threepointone marked this conversation as resolved.
Show resolved Hide resolved
}
threepointone marked this conversation as resolved.
Show resolved Hide resolved
return nextHook;
}

function createWorkInProgressHook(): Hook {
Expand Down Expand Up @@ -390,6 +544,8 @@ export function useContext<T>(
): T {
if (__DEV__) {
currentHookNameInDev = 'useContext';
createWorkInProgressHook();
currentHookNameInDev = null;
}
threepointone marked this conversation as resolved.
Show resolved Hide resolved
// Ensure we're in a function component (class components support only the
// .unstable_read() form)
Expand Down Expand Up @@ -422,6 +578,7 @@ export function useReducer<S, A>(
}
let fiber = (currentlyRenderingFiber = resolveCurrentlyRenderingFiber());
workInProgressHook = createWorkInProgressHook();
currentHookNameInDev = null;
let queue: UpdateQueue<S, A> | null = (workInProgressHook.queue: any);
if (queue !== null) {
// Already have a queue, so this is an update.
Expand Down Expand Up @@ -600,7 +757,11 @@ function pushEffect(tag, create, destroy, deps) {

export function useRef<T>(initialValue: T): {current: T} {
currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
if (__DEV__) {
currentHookNameInDev = 'useRef';
}
workInProgressHook = createWorkInProgressHook();
currentHookNameInDev = null;
let ref;

if (workInProgressHook.memoizedState === null) {
Expand All @@ -620,7 +781,9 @@ export function useLayoutEffect(
deps: Array<mixed> | void | null,
): void {
if (__DEV__) {
currentHookNameInDev = 'useLayoutEffect';
if (currentHookNameInDev !== 'useImperativeHandle') {
currentHookNameInDev = 'useLayoutEffect';
}
}
useEffectImpl(UpdateEffect, UnmountMutation | MountLayout, create, deps);
}
Expand Down Expand Up @@ -653,6 +816,7 @@ function useEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
const prevDeps = prevEffect.deps;
if (areHookInputsEqual(nextDeps, prevDeps)) {
pushEffect(NoHookEffect, create, destroy, nextDeps);
currentHookNameInDev = null;
return;
}
}
Expand All @@ -665,6 +829,7 @@ function useEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
destroy,
nextDeps,
);
currentHookNameInDev = null;
}

export function useImperativeHandle<T>(
Expand Down Expand Up @@ -746,11 +911,13 @@ export function useCallback<T>(
if (nextDeps !== null) {
const prevDeps: Array<mixed> | null = prevState[1];
if (areHookInputsEqual(nextDeps, prevDeps)) {
currentHookNameInDev = null;
return prevState[0];
}
}
}
workInProgressHook.memoizedState = [callback, nextDeps];
currentHookNameInDev = null;
return callback;
}

Expand All @@ -772,6 +939,7 @@ export function useMemo<T>(
if (nextDeps !== null) {
const prevDeps: Array<mixed> | null = prevState[1];
if (areHookInputsEqual(nextDeps, prevDeps)) {
currentHookNameInDev = null;
return prevState[0];
}
}
Expand All @@ -782,6 +950,7 @@ export function useMemo<T>(
const nextValue = nextCreate();
currentlyRenderingFiber = fiber;
workInProgressHook.memoizedState = [nextValue, nextDeps];
currentHookNameInDev = null;
return nextValue;
}

Expand Down
Loading