Skip to content

Commit

Permalink
Don't bother comparing constructor when deps are not provided (#14594)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
acdlite authored Jan 17, 2019
1 parent 71b64d5 commit 1454a8b
Show file tree
Hide file tree
Showing 5 changed files with 221 additions and 84 deletions.
61 changes: 60 additions & 1 deletion packages/react-dom/src/server/ReactPartialRendererHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,12 @@
import typeof {Dispatcher as DispatcherType} from 'react-reconciler/src/ReactFiberDispatcher';
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 @@ -49,6 +49,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 @@ -57,6 +60,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 < prevDeps.length && 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 @@ -153,6 +198,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 @@ -166,6 +214,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 @@ -178,6 +229,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 @@ -276,6 +332,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
Loading

0 comments on commit 1454a8b

Please sign in to comment.