From d5f42543870d16e3599f6fadf88f56c7a85b7e10 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 16 Jan 2019 17:53:48 -0800 Subject: [PATCH] Don't bother comparing constructor when deps are not provided (#14594) * 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 --- .../src/server/ReactPartialRendererHooks.js | 61 ++++++- .../react-reconciler/src/ReactFiberHooks.js | 157 ++++++++++++++---- .../src/__tests__/ReactHooks-test.internal.js | 34 +++- ...eactHooksWithNoopRenderer-test.internal.js | 19 ++- packages/shared/areHookInputsEqual.js | 34 ---- 5 files changed, 221 insertions(+), 84 deletions(-) delete mode 100644 packages/shared/areHookInputsEqual.js diff --git a/packages/react-dom/src/server/ReactPartialRendererHooks.js b/packages/react-dom/src/server/ReactPartialRendererHooks.js index a4cf6ce04f8d3..558c34064b2c5 100644 --- a/packages/react-dom/src/server/ReactPartialRendererHooks.js +++ b/packages/react-dom/src/server/ReactPartialRendererHooks.js @@ -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; type Dispatch = A => void; @@ -49,6 +49,9 @@ let renderPhaseUpdates: Map, Update> | 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, @@ -57,6 +60,48 @@ function resolveCurrentlyRenderingComponent(): Object { return currentlyRenderingComponent; } +function areHookInputsEqual( + nextDeps: Array, + prevDeps: Array | 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, @@ -153,6 +198,9 @@ function useContext( context: ReactContext, observedBits: void | number | boolean, ): T { + if (__DEV__) { + currentHookNameInDev = 'useContext'; + } resolveCurrentlyRenderingComponent(); let threadID = currentThreadID; validateContextBounds(context, threadID); @@ -166,6 +214,9 @@ function basicStateReducer(state: S, action: BasicStateAction): S { export function useState( initialState: (() => S) | S, ): [S, Dispatch>] { + if (__DEV__) { + currentHookNameInDev = 'useState'; + } return useReducer( basicStateReducer, // useReducer has a special case to support lazy useState initializers @@ -178,6 +229,11 @@ export function useReducer( initialState: S, initialAction: A | void | null, ): [S, Dispatch] { + if (__DEV__) { + if (reducer !== basicStateReducer) { + currentHookNameInDev = 'useReducer'; + } + } currentlyRenderingComponent = resolveCurrentlyRenderingComponent(); workInProgressHook = createWorkInProgressHook(); if (isReRender) { @@ -276,6 +332,9 @@ export function useLayoutEffect( create: () => mixed, inputs: Array | void | null, ) { + if (__DEV__) { + currentHookNameInDev = 'useLayoutEffect'; + } warning( false, 'useLayoutEffect does nothing on the server, because its effect cannot ' + diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 4b9d8c704dbb0..a75f5cb212921 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -36,7 +36,7 @@ import { import invariant from 'shared/invariant'; import warning from 'shared/warning'; import getComponentName from 'shared/getComponentName'; -import areHookInputsEqual from 'shared/areHookInputsEqual'; +import is from 'shared/objectIs'; import {markWorkInProgressReceivedUpdate} from './ReactFiberBeginWork'; type Update = { @@ -68,7 +68,7 @@ type Effect = { tag: HookEffectTag, create: () => mixed, destroy: (() => mixed) | null, - inputs: Array, + deps: Array | null, next: Effect, }; @@ -117,6 +117,9 @@ let renderPhaseUpdates: Map< let numberOfReRenders: number = -1; 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, @@ -125,6 +128,48 @@ function resolveCurrentlyRenderingFiber(): Fiber { return currentlyRenderingFiber; } +function areHookInputsEqual( + nextDeps: Array, + prevDeps: Array | 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; +} + export function renderWithHooks( current: Fiber | null, workInProgress: Fiber, @@ -202,6 +247,10 @@ export function renderWithHooks( remainingExpirationTime = NoWork; componentUpdateQueue = null; + if (__DEV__) { + currentHookNameInDev = undefined; + } + // These were reset above // didScheduleRenderPhaseUpdate = false; // renderPhaseUpdates = null; @@ -247,6 +296,10 @@ export function resetHooks(): void { remainingExpirationTime = NoWork; componentUpdateQueue = null; + if (__DEV__) { + currentHookNameInDev = undefined; + } + didScheduleRenderPhaseUpdate = false; renderPhaseUpdates = null; numberOfReRenders = -1; @@ -335,6 +388,9 @@ export function useContext( context: ReactContext, 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(); @@ -344,6 +400,9 @@ export function useContext( export function useState( initialState: (() => S) | S, ): [S, Dispatch>] { + if (__DEV__) { + currentHookNameInDev = 'useState'; + } return useReducer( basicStateReducer, // useReducer has a special case to support lazy useState initializers @@ -356,6 +415,11 @@ export function useReducer( initialState: S, initialAction: A | void | null, ): [S, Dispatch] { + if (__DEV__) { + if (reducer !== basicStateReducer) { + currentHookNameInDev = 'useReducer'; + } + } currentlyRenderingFiber = resolveCurrentlyRenderingFiber(); workInProgressHook = createWorkInProgressHook(); let queue: UpdateQueue | null = (workInProgressHook.queue: any); @@ -500,12 +564,12 @@ export function useReducer( return [workInProgressHook.memoizedState, dispatch]; } -function pushEffect(tag, create, destroy, inputs) { +function pushEffect(tag, create, destroy, deps) { const effect: Effect = { tag, create, destroy, - inputs, + deps, // Circular next: (null: any), }; @@ -545,35 +609,44 @@ export function useRef(initialValue: T): {current: T} { export function useLayoutEffect( create: () => mixed, - inputs: Array | void | null, + deps: Array | void | null, ): void { - useEffectImpl(UpdateEffect, UnmountMutation | MountLayout, create, inputs); + if (__DEV__) { + currentHookNameInDev = 'useLayoutEffect'; + } + useEffectImpl(UpdateEffect, UnmountMutation | MountLayout, create, deps); } export function useEffect( create: () => mixed, - inputs: Array | void | null, + deps: Array | void | null, ): void { + if (__DEV__) { + currentHookNameInDev = 'useEffect'; + } useEffectImpl( UpdateEffect | PassiveEffect, UnmountPassive | MountPassive, create, - inputs, + deps, ); } -function useEffectImpl(fiberEffectTag, hookEffectTag, create, inputs): void { +function useEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void { currentlyRenderingFiber = resolveCurrentlyRenderingFiber(); workInProgressHook = createWorkInProgressHook(); - let nextInputs = inputs !== undefined && inputs !== null ? inputs : [create]; + const nextDeps = deps === undefined ? null : deps; let destroy = null; if (currentHook !== null) { const prevEffect = currentHook.memoizedState; destroy = prevEffect.destroy; - if (areHookInputsEqual(nextInputs, prevEffect.inputs)) { - pushEffect(NoHookEffect, create, destroy, nextInputs); - return; + if (nextDeps !== null) { + const prevDeps = prevEffect.deps; + if (areHookInputsEqual(nextDeps, prevDeps)) { + pushEffect(NoHookEffect, create, destroy, nextDeps); + return; + } } } @@ -582,20 +655,21 @@ function useEffectImpl(fiberEffectTag, hookEffectTag, create, inputs): void { hookEffectTag, create, destroy, - nextInputs, + nextDeps, ); } export function useImperativeHandle( ref: {current: T | null} | ((inst: T | null) => mixed) | null | void, create: () => T, - inputs: Array | void | null, + deps: Array | void | null, ): void { - // TODO: If inputs are provided, should we skip comparing the ref itself? - const nextInputs = - inputs !== null && inputs !== undefined - ? inputs.concat([ref]) - : [ref, create]; + 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]; // TODO: I've implemented this on top of useEffect because it's almost the // same thing, and it would require an equal amount of code. It doesn't seem @@ -614,13 +688,17 @@ export function useImperativeHandle( refObject.current = null; }; } - }, nextInputs); + }, nextDeps); } export function useDebugValue( value: any, formatterFn: ?(value: any) => any, ): void { + if (__DEV__) { + currentHookNameInDev = 'useDebugValue'; + } + // This will trigger a warning if the hook is used in a non-Function component. resolveCurrentlyRenderingFiber(); @@ -631,45 +709,54 @@ export function useDebugValue( export function useCallback( callback: T, - inputs: Array | void | null, + deps: Array | void | null, ): T { + if (__DEV__) { + currentHookNameInDev = 'useCallback'; + } currentlyRenderingFiber = resolveCurrentlyRenderingFiber(); workInProgressHook = createWorkInProgressHook(); - const nextInputs = - inputs !== undefined && inputs !== null ? inputs : [callback]; + const nextDeps = deps === undefined ? null : deps; const prevState = workInProgressHook.memoizedState; if (prevState !== null) { - const prevInputs = prevState[1]; - if (areHookInputsEqual(nextInputs, prevInputs)) { - return prevState[0]; + if (nextDeps !== null) { + const prevDeps: Array | null = prevState[1]; + if (areHookInputsEqual(nextDeps, prevDeps)) { + return prevState[0]; + } } } - workInProgressHook.memoizedState = [callback, nextInputs]; + workInProgressHook.memoizedState = [callback, nextDeps]; return callback; } export function useMemo( nextCreate: () => T, - inputs: Array | void | null, + deps: Array | void | null, ): T { + if (__DEV__) { + currentHookNameInDev = 'useMemo'; + } currentlyRenderingFiber = resolveCurrentlyRenderingFiber(); workInProgressHook = createWorkInProgressHook(); - const nextInputs = - inputs !== undefined && inputs !== null ? inputs : [nextCreate]; + const nextDeps = deps === undefined ? null : deps; const prevState = workInProgressHook.memoizedState; if (prevState !== null) { - const prevInputs = prevState[1]; - if (areHookInputsEqual(nextInputs, prevInputs)) { - return prevState[0]; + // Assume these are defined. If they're not, areHookInputsEqual will warn. + if (nextDeps !== null) { + const prevDeps: Array | null = prevState[1]; + if (areHookInputsEqual(nextDeps, prevDeps)) { + return prevState[0]; + } } } const nextValue = nextCreate(); - workInProgressHook.memoizedState = [nextValue, nextInputs]; + workInProgressHook.memoizedState = [nextValue, nextDeps]; return nextValue; } diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 7014bfe343094..0e4e2dfacdb05 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -447,12 +447,36 @@ describe('ReactHooks', () => { expect(() => { root.update(); }).toWarnDev([ - 'Warning: Detected a variable number of hook dependencies. The length ' + - 'of the dependencies array should be constant between renders.\n\n' + - 'Previous: A, B\n' + - 'Incoming: A', + '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]\n', + ]); + }); + + it('warns if switching from dependencies to no dependencies', () => { + const {useMemo} = React; + function App({text, hasDeps}) { + const resolvedText = useMemo(() => { + ReactTestRenderer.unstable_yield('Compute'); + return text.toUpperCase(); + }, hasDeps ? null : [text]); + return resolvedText; + } + + const root = ReactTestRenderer.create(null); + root.update(); + expect(ReactTestRenderer).toHaveYielded(['Compute']); + expect(root).toMatchRenderedOutput('HELLO'); + + expect(() => { + root.update(); + }).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.', ]); - expect(ReactTestRenderer).toHaveYielded(['Did commit: A, B']); }); it('warns for bad useEffect return values', () => { diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index ac8328005a7c9..74d69fac78de8 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -1015,11 +1015,11 @@ describe('ReactHooksWithNoopRenderer', () => { expect(ReactNoop.getChildren()).toEqual([]); }); - it('skips effect if constructor has not changed', () => { + it('always fires effects if no dependencies are provided', () => { function effect() { - ReactNoop.yield(`Did mount`); + ReactNoop.yield(`Did create`); return () => { - ReactNoop.yield(`Did unmount`); + ReactNoop.yield(`Did destroy`); }; } function Counter(props) { @@ -1030,15 +1030,16 @@ describe('ReactHooksWithNoopRenderer', () => { expect(ReactNoop.flush()).toEqual(['Count: 0']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); flushPassiveEffects(); - expect(ReactNoop.clearYields()).toEqual(['Did mount']); + expect(ReactNoop.clearYields()).toEqual(['Did create']); ReactNoop.render(); - // No effect, because constructor was hoisted outside render expect(ReactNoop.flush()).toEqual(['Count: 1']); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual(['Did destroy', 'Did create']); ReactNoop.render(null); - expect(ReactNoop.flush()).toEqual(['Did unmount']); + expect(ReactNoop.flush()).toEqual(['Did destroy']); expect(ReactNoop.getChildren()).toEqual([]); }); @@ -1456,7 +1457,7 @@ describe('ReactHooksWithNoopRenderer', () => { expect(ReactNoop.getChildren()).toEqual([span('GOODBYE')]); }); - it('compares function if no inputs are provided', () => { + it('always re-computes if no inputs are provided', () => { function LazyCompute(props) { const computed = useMemo(props.compute); return ; @@ -1476,10 +1477,10 @@ describe('ReactHooksWithNoopRenderer', () => { expect(ReactNoop.flush()).toEqual(['compute A', 'A']); ReactNoop.render(); - expect(ReactNoop.flush()).toEqual(['A']); + expect(ReactNoop.flush()).toEqual(['compute A', 'A']); ReactNoop.render(); - expect(ReactNoop.flush()).toEqual(['A']); + expect(ReactNoop.flush()).toEqual(['compute A', 'A']); ReactNoop.render(); expect(ReactNoop.flush()).toEqual(['compute B', 'B']); diff --git a/packages/shared/areHookInputsEqual.js b/packages/shared/areHookInputsEqual.js deleted file mode 100644 index 8a78a113fbbda..0000000000000 --- a/packages/shared/areHookInputsEqual.js +++ /dev/null @@ -1,34 +0,0 @@ -/** - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - * - * @flow - */ - -import warning from 'shared/warning'; -import is from './objectIs'; - -export default function areHookInputsEqual(arr1: any[], arr2: any[]) { - // Don't bother comparing lengths in prod because these arrays should be - // passed inline. - if (__DEV__) { - warning( - arr1.length === arr2.length, - 'Detected a variable number of hook dependencies. The length of the ' + - 'dependencies array should be constant between renders.\n\n' + - 'Previous: %s\n' + - 'Incoming: %s', - arr1.join(', '), - arr2.join(', '), - ); - } - for (let i = 0; i < arr1.length; i++) { - if (is(arr1[i], arr2[i])) { - continue; - } - return false; - } - return true; -}