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 38 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
226 changes: 202 additions & 24 deletions packages/react-reconciler/src/ReactFiberHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,39 @@ type UpdateQueue<S, A> = {
eagerState: S | null,
};

type HookType = 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9;

const StateHook = 0;
const ReducerHook = 1;
const ContextHook = 2;
const RefHook = 3;
const EffectHook = 4;
const LayoutEffectHook = 5;
const CallbackHook = 6;
const MemoHook = 7;
const ImperativeHandleHook = 8;
const DebugValueHook = 9;

const HookDevNames = [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd write these as switch so that the 1:1 correspondence is clear, and also so that we can (type) check if is exhaustive, HookType -> string

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ugh I missed this, will get to in a bit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this, and maybe I don't think this is critical? we're not planning on changing this list anytime soon, and it'll add complexity.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not saying this is critical but I'm also not sure how this will add complexity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't want to add extra function calls and the comparisons when it was readily indexed and available here.

'useState',
'useReducer',
'useContext',
'useRef',
'useEffect',
'useLayoutEffect',
'useCallback',
'useMemo',
'useImperativeHandle',
'useDebugValue',
];
threepointone marked this conversation as resolved.
Show resolved Hide resolved

let currentHookType: HookType | null = null;
// the first instance of a hook mismatch in a component,
// represented by a portion of it's stacktrace
let currentHookMismatch = null;
const PossiblyWeakSet = typeof WeakSet === 'function' ? WeakSet : Set;
threepointone marked this conversation as resolved.
Show resolved Hide resolved
const hookMismatchedFibers = new PossiblyWeakSet();

export type Hook = {
memoizedState: any,

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

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

type Effect = {
tag: HookEffectTag,
create: () => mixed,
Expand Down Expand Up @@ -170,6 +207,92 @@ 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 (string.length >= length) {
return string;
} else {
return string + ' ' + new Array(length - string.length).join(' ');
}
}

function flushHookMismatchWarnings() {
threepointone marked this conversation as resolved.
Show resolved Hide resolved
// 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 &&
!hookMismatchedFibers.has(((currentlyRenderingFiber: any): Fiber))
) {
let componentName = getComponentName(
((currentlyRenderingFiber: any): Fiber).type,
);
hookMismatchedFibers.add(((currentlyRenderingFiber: any): Fiber));
const hookStackDiff = [];
let current = firstCurrentHook;
let previousOrder = [];
while (current !== null) {
previousOrder.push(HookDevNames[((current: any): HookDev)._debugType]);
current = current.next;
}
let workInProgress = firstWorkInProgressHook;
let nextOrder = [];
while (workInProgress !== null) {
nextOrder.push(
HookDevNames[((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) + ' Next render\n';
const hookStackWidth = hookStackHeader.length;
hookStackHeader += ' ' + new Array(hookStackWidth - 2).join('-');
let 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) +
padEndSpaces(previousOrder[i], columnLength) +
' ' +
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 +344,7 @@ export function renderWithHooks(
getComponentName(Component),
);
}
flushHookMismatchWarnings();
}
} while (didScheduleRenderPhaseUpdate);

Expand Down Expand Up @@ -281,6 +405,7 @@ export function resetHooks(): void {
if (!enableHooks) {
return;
}
flushHookMismatchWarnings();
threepointone marked this conversation as resolved.
Show resolved Hide resolved

// 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 @@ -299,34 +424,69 @@ export function resetHooks(): void {
if (__DEV__) {
currentHookNameInDev = undefined;
}

didScheduleRenderPhaseUpdate = false;
renderPhaseUpdates = null;
numberOfReRenders = -1;
}

function createHook(): Hook {
return {
memoizedState: null,
let hook: Hook = __DEV__
? {
threepointone marked this conversation as resolved.
Show resolved Hide resolved
_debugType: ((currentHookType: 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: ((currentHookType: 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 (currentHookType !== ((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 @@ -389,7 +549,10 @@ export function useContext<T>(
observedBits: void | number | boolean,
): T {
if (__DEV__) {
currentHookNameInDev = 'useContext';
currentHookNameInDev = HookDevNames[ContextHook];
currentHookType = ContextHook;
createWorkInProgressHook();
currentHookType = 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 All @@ -401,7 +564,7 @@ export function useState<S>(
initialState: (() => S) | S,
): [S, Dispatch<BasicStateAction<S>>] {
if (__DEV__) {
currentHookNameInDev = 'useState';
currentHookNameInDev = HookDevNames[StateHook];
}
return useReducer(
basicStateReducer,
Expand All @@ -417,11 +580,13 @@ export function useReducer<S, A>(
): [S, Dispatch<A>] {
if (__DEV__) {
if (reducer !== basicStateReducer) {
currentHookNameInDev = 'useReducer';
currentHookNameInDev = HookDevNames[ReducerHook];
}
}
currentHookType = reducer === basicStateReducer ? StateHook : ReducerHook;
let fiber = (currentlyRenderingFiber = resolveCurrentlyRenderingFiber());
workInProgressHook = createWorkInProgressHook();
currentHookType = 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 +765,9 @@ function pushEffect(tag, create, destroy, deps) {

export function useRef<T>(initialValue: T): {current: T} {
currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
currentHookType = RefHook;
workInProgressHook = createWorkInProgressHook();
currentHookType = null;
let ref;

if (workInProgressHook.memoizedState === null) {
Expand All @@ -620,7 +787,7 @@ export function useLayoutEffect(
deps: Array<mixed> | void | null,
): void {
if (__DEV__) {
currentHookNameInDev = 'useLayoutEffect';
currentHookNameInDev = HookDevNames[LayoutEffectHook];
}
useEffectImpl(UpdateEffect, UnmountMutation | MountLayout, create, deps);
}
Expand All @@ -630,7 +797,7 @@ export function useEffect(
deps: Array<mixed> | void | null,
): void {
if (__DEV__) {
currentHookNameInDev = 'useEffect';
currentHookNameInDev = HookDevNames[EffectHook];
}
useEffectImpl(
UpdateEffect | PassiveEffect,
Expand All @@ -642,7 +809,13 @@ export function useEffect(

function useEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
if (currentHookType !== ImperativeHandleHook) {
// it could be an ImperativeHandleHook
currentHookType =
fiberEffectTag === UpdateEffect ? EffectHook : LayoutEffectHook;
}
workInProgressHook = createWorkInProgressHook();
currentHookType = null;

const nextDeps = deps === undefined ? null : deps;
let destroy = null;
Expand Down Expand Up @@ -673,14 +846,15 @@ export function useImperativeHandle<T>(
deps: Array<mixed> | void | null,
): void {
if (__DEV__) {
currentHookNameInDev = 'useImperativeHandle';
currentHookNameInDev = HookDevNames[ImperativeHandleHook];
warning(
typeof create === 'function',
'Expected useImperativeHandle() second argument to be a function ' +
'that creates a handle. Instead received: %s.',
create !== null ? typeof create : 'null',
);
}
currentHookType = ImperativeHandleHook;
// 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 @@ -718,7 +892,7 @@ export function useDebugValue(
formatterFn: ?(value: any) => any,
): void {
if (__DEV__) {
currentHookNameInDev = 'useDebugValue';
currentHookNameInDev = HookDevNames[DebugValueHook];
}

// This will trigger a warning if the hook is used in a non-Function component.
Expand All @@ -734,10 +908,12 @@ export function useCallback<T>(
deps: Array<mixed> | void | null,
): T {
if (__DEV__) {
currentHookNameInDev = 'useCallback';
currentHookNameInDev = HookDevNames[CallbackHook];
}
currentHookType = CallbackHook;
currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
workInProgressHook = createWorkInProgressHook();
currentHookType = null;

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

Expand All @@ -759,10 +935,12 @@ export function useMemo<T>(
deps: Array<mixed> | void | null,
): T {
if (__DEV__) {
currentHookNameInDev = 'useMemo';
currentHookNameInDev = HookDevNames[MemoHook];
}
currentHookType = MemoHook;
let fiber = (currentlyRenderingFiber = resolveCurrentlyRenderingFiber());
workInProgressHook = createWorkInProgressHook();
currentHookType = null;

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

Expand Down
Loading