Skip to content

Commit c0d53ae

Browse files
committed
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).
1 parent edb1f59 commit c0d53ae

File tree

4 files changed

+84
-51
lines changed

4 files changed

+84
-51
lines changed

packages/react-reconciler/src/ReactFiberHooks.js

Lines changed: 31 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ type Effect = {
6161
tag: HookEffectTag,
6262
create: () => mixed,
6363
destroy: (() => mixed) | null,
64-
inputs: Array<mixed>,
64+
deps: Array<mixed> | null,
6565
next: Effect,
6666
};
6767

@@ -471,12 +471,12 @@ export function useReducer<S, A>(
471471
return [workInProgressHook.memoizedState, dispatch];
472472
}
473473

474-
function pushEffect(tag, create, destroy, inputs) {
474+
function pushEffect(tag, create, destroy, deps) {
475475
const effect: Effect = {
476476
tag,
477477
create,
478478
destroy,
479-
inputs,
479+
deps,
480480
// Circular
481481
next: (null: any),
482482
};
@@ -516,34 +516,36 @@ export function useRef<T>(initialValue: T): {current: T} {
516516

517517
export function useLayoutEffect(
518518
create: () => mixed,
519-
inputs: Array<mixed> | void | null,
519+
deps: Array<mixed> | void | null,
520520
): void {
521-
useEffectImpl(UpdateEffect, UnmountMutation | MountLayout, create, inputs);
521+
useEffectImpl(UpdateEffect, UnmountMutation | MountLayout, create, deps);
522522
}
523523

524524
export function useEffect(
525525
create: () => mixed,
526-
inputs: Array<mixed> | void | null,
526+
deps: Array<mixed> | void | null,
527527
): void {
528528
useEffectImpl(
529529
UpdateEffect | PassiveEffect,
530530
UnmountPassive | MountPassive,
531531
create,
532-
inputs,
532+
deps,
533533
);
534534
}
535535

536-
function useEffectImpl(fiberEffectTag, hookEffectTag, create, inputs): void {
536+
function useEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
537537
currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
538538
workInProgressHook = createWorkInProgressHook();
539539

540-
let nextInputs = inputs !== undefined && inputs !== null ? inputs : [create];
540+
const nextDeps = deps === undefined ? null : deps;
541541
let destroy = null;
542542
if (currentHook !== null) {
543543
const prevEffect = currentHook.memoizedState;
544544
destroy = prevEffect.destroy;
545-
if (areHookInputsEqual(nextInputs, prevEffect.inputs)) {
546-
pushEffect(NoHookEffect, create, destroy, nextInputs);
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);
547549
return;
548550
}
549551
}
@@ -553,20 +555,18 @@ function useEffectImpl(fiberEffectTag, hookEffectTag, create, inputs): void {
553555
hookEffectTag,
554556
create,
555557
destroy,
556-
nextInputs,
558+
nextDeps,
557559
);
558560
}
559561

560562
export function useImperativeHandle<T>(
561563
ref: {current: T | null} | ((inst: T | null) => mixed) | null | void,
562564
create: () => T,
563-
inputs: Array<mixed> | void | null,
565+
deps: Array<mixed> | void | null,
564566
): void {
565-
// TODO: If inputs are provided, should we skip comparing the ref itself?
566-
const nextInputs =
567-
inputs !== null && inputs !== undefined
568-
? inputs.concat([ref])
569-
: [ref, create];
567+
// TODO: If deps are provided, should we skip comparing the ref itself?
568+
const nextDeps =
569+
deps !== null && deps !== undefined ? deps.concat([ref]) : [ref];
570570

571571
// TODO: I've implemented this on top of useEffect because it's almost the
572572
// same thing, and it would require an equal amount of code. It doesn't seem
@@ -585,7 +585,7 @@ export function useImperativeHandle<T>(
585585
refObject.current = null;
586586
};
587587
}
588-
}, nextInputs);
588+
}, nextDeps);
589589
}
590590

591591
export function useDebugValue(
@@ -599,45 +599,45 @@ export function useDebugValue(
599599

600600
export function useCallback<T>(
601601
callback: T,
602-
inputs: Array<mixed> | void | null,
602+
deps: Array<mixed> | void | null,
603603
): T {
604604
currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
605605
workInProgressHook = createWorkInProgressHook();
606606

607-
const nextInputs =
608-
inputs !== undefined && inputs !== null ? inputs : [callback];
607+
const nextDeps = deps === undefined ? null : deps;
609608

610609
const prevState = workInProgressHook.memoizedState;
611610
if (prevState !== null) {
612-
const prevInputs = prevState[1];
613-
if (areHookInputsEqual(nextInputs, prevInputs)) {
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)) {
614614
return prevState[0];
615615
}
616616
}
617-
workInProgressHook.memoizedState = [callback, nextInputs];
617+
workInProgressHook.memoizedState = [callback, nextDeps];
618618
return callback;
619619
}
620620

621621
export function useMemo<T>(
622622
nextCreate: () => T,
623-
inputs: Array<mixed> | void | null,
623+
deps: Array<mixed> | void | null,
624624
): T {
625625
currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
626626
workInProgressHook = createWorkInProgressHook();
627627

628-
const nextInputs =
629-
inputs !== undefined && inputs !== null ? inputs : [nextCreate];
628+
const nextDeps = deps === undefined ? null : deps;
630629

631630
const prevState = workInProgressHook.memoizedState;
632631
if (prevState !== null) {
633-
const prevInputs = prevState[1];
634-
if (areHookInputsEqual(nextInputs, prevInputs)) {
632+
// Assume these are defined. If they're not, areHookInputsEqual will warn.
633+
const prevDeps = prevState[1];
634+
if (nextDeps !== null && areHookInputsEqual(nextDeps, prevDeps)) {
635635
return prevState[0];
636636
}
637637
}
638638

639639
const nextValue = nextCreate();
640-
workInProgressHook.memoizedState = [nextValue, nextInputs];
640+
workInProgressHook.memoizedState = [nextValue, nextDeps];
641641
return nextValue;
642642
}
643643

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

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,12 +48,42 @@ describe('ReactHooks', () => {
4848
}).toWarnDev([
4949
'Warning: Detected a variable number of hook dependencies. The length ' +
5050
'of the dependencies array should be constant between renders.\n\n' +
51-
'Previous: A, B\n' +
52-
'Incoming: A',
51+
'Previous: [A, B]\n' +
52+
'Incoming: [A]',
5353
]);
5454
expect(ReactTestRenderer).toHaveYielded(['Did commit: A, B']);
5555
});
5656

57+
it('warns if switching from dependencies to no dependencies', () => {
58+
spyOnDev(console, 'error');
59+
60+
const {useMemo} = React;
61+
function App({text, hasDeps}) {
62+
const resolvedText = useMemo(() => {
63+
ReactTestRenderer.unstable_yield('Compute');
64+
return text.toUpperCase();
65+
}, hasDeps ? null : [text]);
66+
return resolvedText;
67+
}
68+
69+
const root = ReactTestRenderer.create(null);
70+
root.update(<App text="Hello" hasDeps={true} />);
71+
expect(ReactTestRenderer).toHaveYielded(['Compute']);
72+
expect(root).toMatchRenderedOutput('HELLO');
73+
74+
expect(() => {
75+
// This prints a warning message then throws a null access error
76+
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+
}
85+
});
86+
5787
it('warns for bad useEffect return values', () => {
5888
const {useLayoutEffect} = React;
5989
function App(props) {

packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1015,11 +1015,11 @@ describe('ReactHooksWithNoopRenderer', () => {
10151015
expect(ReactNoop.getChildren()).toEqual([]);
10161016
});
10171017

1018-
it('skips effect if constructor has not changed', () => {
1018+
it('always fires effects if no dependencies are provided', () => {
10191019
function effect() {
1020-
ReactNoop.yield(`Did mount`);
1020+
ReactNoop.yield(`Did create`);
10211021
return () => {
1022-
ReactNoop.yield(`Did unmount`);
1022+
ReactNoop.yield(`Did destroy`);
10231023
};
10241024
}
10251025
function Counter(props) {
@@ -1030,15 +1030,16 @@ describe('ReactHooksWithNoopRenderer', () => {
10301030
expect(ReactNoop.flush()).toEqual(['Count: 0']);
10311031
expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]);
10321032
flushPassiveEffects();
1033-
expect(ReactNoop.clearYields()).toEqual(['Did mount']);
1033+
expect(ReactNoop.clearYields()).toEqual(['Did create']);
10341034

10351035
ReactNoop.render(<Counter count={1} />);
1036-
// No effect, because constructor was hoisted outside render
10371036
expect(ReactNoop.flush()).toEqual(['Count: 1']);
10381037
expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]);
1038+
flushPassiveEffects();
1039+
expect(ReactNoop.clearYields()).toEqual(['Did destroy', 'Did create']);
10391040

10401041
ReactNoop.render(null);
1041-
expect(ReactNoop.flush()).toEqual(['Did unmount']);
1042+
expect(ReactNoop.flush()).toEqual(['Did destroy']);
10421043
expect(ReactNoop.getChildren()).toEqual([]);
10431044
});
10441045

@@ -1456,7 +1457,7 @@ describe('ReactHooksWithNoopRenderer', () => {
14561457
expect(ReactNoop.getChildren()).toEqual([span('GOODBYE')]);
14571458
});
14581459

1459-
it('compares function if no inputs are provided', () => {
1460+
it('always re-computes if no inputs are provided', () => {
14601461
function LazyCompute(props) {
14611462
const computed = useMemo(props.compute);
14621463
return <Text text={computed} />;
@@ -1476,10 +1477,10 @@ describe('ReactHooksWithNoopRenderer', () => {
14761477
expect(ReactNoop.flush()).toEqual(['compute A', 'A']);
14771478

14781479
ReactNoop.render(<LazyCompute compute={computeA} />);
1479-
expect(ReactNoop.flush()).toEqual(['A']);
1480+
expect(ReactNoop.flush()).toEqual(['compute A', 'A']);
14801481

14811482
ReactNoop.render(<LazyCompute compute={computeA} />);
1482-
expect(ReactNoop.flush()).toEqual(['A']);
1483+
expect(ReactNoop.flush()).toEqual(['compute A', 'A']);
14831484

14841485
ReactNoop.render(<LazyCompute compute={computeB} />);
14851486
expect(ReactNoop.flush()).toEqual(['compute B', 'B']);

packages/shared/areHookInputsEqual.js

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -14,15 +14,17 @@ export default function areHookInputsEqual(arr1: any[], arr2: any[]) {
1414
// Don't bother comparing lengths in prod because these arrays should be
1515
// passed inline.
1616
if (__DEV__) {
17-
warning(
18-
arr1.length === arr2.length,
19-
'Detected a variable number of hook dependencies. The length of the ' +
20-
'dependencies array should be constant between renders.\n\n' +
21-
'Previous: %s\n' +
22-
'Incoming: %s',
23-
arr1.join(', '),
24-
arr2.join(', '),
25-
);
17+
if (arr1 === null || arr2 === null || arr1.length !== arr2.length) {
18+
warning(
19+
false,
20+
'Detected a variable number of hook dependencies. The length of the ' +
21+
'dependencies array should be constant between renders.\n\n' +
22+
'Previous: %s\n' +
23+
'Incoming: %s',
24+
arr1 === null ? '(empty)' : `[${arr1.join(', ')}]`,
25+
arr2 === null ? '(empty)' : `[${arr2.join(', ')}]`,
26+
);
27+
}
2628
}
2729
for (let i = 0; i < arr1.length; i++) {
2830
if (is(arr1[i], arr2[i])) {

0 commit comments

Comments
 (0)