From 94d92a8fd1b2fefaf3ddfce246ab422672435369 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 14 Jan 2019 18:38:05 -0800 Subject: [PATCH 1/3] 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). --- .../react-reconciler/src/ReactFiberHooks.js | 62 +++++++++---------- .../src/__tests__/ReactHooks-test.internal.js | 34 +++++++++- ...eactHooksWithNoopRenderer-test.internal.js | 19 +++--- packages/shared/areHookInputsEqual.js | 20 +++--- 4 files changed, 84 insertions(+), 51 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 4b9d8c704dbb0..aae909aadffff 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -68,7 +68,7 @@ type Effect = { tag: HookEffectTag, create: () => mixed, destroy: (() => mixed) | null, - inputs: Array, + deps: Array | null, next: Effect, }; @@ -500,12 +500,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,34 +545,36 @@ 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); + useEffectImpl(UpdateEffect, UnmountMutation | MountLayout, create, deps); } export function useEffect( create: () => mixed, - inputs: Array | void | null, + deps: Array | void | null, ): void { 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); + // Assume these are defined. If they're not, areHookInputsEqual will warn. + const prevDeps: Array = (prevEffect.deps: any); + if (nextDeps !== null && areHookInputsEqual(nextDeps, prevDeps)) { + pushEffect(NoHookEffect, create, destroy, nextDeps); return; } } @@ -582,20 +584,18 @@ 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]; + // 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,7 +614,7 @@ export function useImperativeHandle( refObject.current = null; }; } - }, nextInputs); + }, nextDeps); } export function useDebugValue( @@ -631,45 +631,45 @@ export function useDebugValue( export function useCallback( callback: T, - inputs: Array | void | null, + deps: Array | void | null, ): T { 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)) { + // Assume these are defined. If they're not, areHookInputsEqual will warn. + const prevDeps: Array = prevState[1]; + if (nextDeps !== null && 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 { 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)) { + // Assume these are defined. If they're not, areHookInputsEqual will warn. + const prevDeps = prevState[1]; + if (nextDeps !== null && 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..07b78626e15d3 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -449,12 +449,42 @@ describe('ReactHooks', () => { }).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', + 'Previous: [A, B]\n' + + 'Incoming: [A]', ]); expect(ReactTestRenderer).toHaveYielded(['Did commit: A, B']); }); + it('warns if switching from dependencies to no dependencies', () => { + spyOnDev(console, 'error'); + + 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(() => { + // This prints a warning message then throws a null access error + root.update(); + }).toThrow('null'); + + if (__DEV__) { + expect(console.error).toHaveBeenCalledTimes(3); + expect(console.error.calls.argsFor(0)[0]).toContain( + 'Warning: Detected a variable number of hook dependencies.', + ); + } + }); + it('warns for bad useEffect return values', () => { const {useLayoutEffect} = React; function App(props) { 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 index 8a78a113fbbda..9ef441d9b6d65 100644 --- a/packages/shared/areHookInputsEqual.js +++ b/packages/shared/areHookInputsEqual.js @@ -14,15 +14,17 @@ 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(', '), - ); + if (arr1 === null || arr2 === null || arr1.length !== arr2.length) { + warning( + false, + '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 === null ? '(empty)' : `[${arr1.join(', ')}]`, + arr2 === null ? '(empty)' : `[${arr2.join(', ')}]`, + ); + } } for (let i = 0; i < arr1.length; i++) { if (is(arr1[i], arr2[i])) { From 62e1048acc0507f37e8b175640556743477c2e58 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 15 Jan 2019 13:09:04 -0800 Subject: [PATCH 2/3] Improve hook dependencies warning It now includes the name of the hook in the message. --- .../src/server/ReactPartialRendererHooks.js | 61 +++++++++- .../react-reconciler/src/ReactFiberHooks.js | 113 ++++++++++++++++-- .../src/__tests__/ReactHooks-test.internal.js | 23 ++-- packages/shared/areHookInputsEqual.js | 36 ------ 4 files changed, 169 insertions(+), 64 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..1bd4a98021f21 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 < 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 aae909aadffff..32a869316fbb2 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 = { @@ -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 < 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); @@ -547,6 +611,9 @@ export function useLayoutEffect( create: () => mixed, deps: Array | void | null, ): void { + if (__DEV__) { + currentHookNameInDev = 'useLayoutEffect'; + } useEffectImpl(UpdateEffect, UnmountMutation | MountLayout, create, deps); } @@ -554,6 +621,9 @@ export function useEffect( create: () => mixed, deps: Array | void | null, ): void { + if (__DEV__) { + currentHookNameInDev = 'useEffect'; + } useEffectImpl( UpdateEffect | PassiveEffect, UnmountPassive | MountPassive, @@ -571,11 +641,12 @@ function useEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void { if (currentHook !== null) { const prevEffect = currentHook.memoizedState; destroy = prevEffect.destroy; - // Assume these are defined. If they're not, areHookInputsEqual will warn. - const prevDeps: Array = (prevEffect.deps: any); - if (nextDeps !== null && areHookInputsEqual(nextDeps, prevDeps)) { - pushEffect(NoHookEffect, create, destroy, nextDeps); - return; + if (nextDeps !== null) { + const prevDeps = prevEffect.deps; + if (areHookInputsEqual(nextDeps, prevDeps)) { + pushEffect(NoHookEffect, create, destroy, nextDeps); + return; + } } } @@ -593,6 +664,9 @@ export function useImperativeHandle( create: () => T, deps: Array | void | null, ): void { + 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]; @@ -621,6 +695,10 @@ 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(); @@ -633,6 +711,9 @@ export function useCallback( callback: T, deps: Array | void | null, ): T { + if (__DEV__) { + currentHookNameInDev = 'useCallback'; + } currentlyRenderingFiber = resolveCurrentlyRenderingFiber(); workInProgressHook = createWorkInProgressHook(); @@ -640,10 +721,11 @@ export function useCallback( const prevState = workInProgressHook.memoizedState; if (prevState !== null) { - // Assume these are defined. If they're not, areHookInputsEqual will warn. - const prevDeps: Array = prevState[1]; - if (nextDeps !== null && areHookInputsEqual(nextDeps, prevDeps)) { - return prevState[0]; + if (nextDeps !== null) { + const prevDeps: Array | null = prevState[1]; + if (areHookInputsEqual(nextDeps, prevDeps)) { + return prevState[0]; + } } } workInProgressHook.memoizedState = [callback, nextDeps]; @@ -654,6 +736,9 @@ export function useMemo( nextCreate: () => T, deps: Array | void | null, ): T { + if (__DEV__) { + currentHookNameInDev = 'useMemo'; + } currentlyRenderingFiber = resolveCurrentlyRenderingFiber(); workInProgressHook = createWorkInProgressHook(); @@ -662,9 +747,11 @@ export function useMemo( const prevState = workInProgressHook.memoizedState; if (prevState !== null) { // Assume these are defined. If they're not, areHookInputsEqual will warn. - const prevDeps = prevState[1]; - if (nextDeps !== null && areHookInputsEqual(nextDeps, prevDeps)) { - return prevState[0]; + if (nextDeps !== null) { + const prevDeps: Array | null = prevState[1]; + if (areHookInputsEqual(nextDeps, prevDeps)) { + return prevState[0]; + } } } diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 07b78626e15d3..5ee1be80d1d4f 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -447,17 +447,16 @@ 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' + + '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]', + 'Incoming: [A]\n', ]); expect(ReactTestRenderer).toHaveYielded(['Did commit: A, B']); }); it('warns if switching from dependencies to no dependencies', () => { - spyOnDev(console, 'error'); - const {useMemo} = React; function App({text, hasDeps}) { const resolvedText = useMemo(() => { @@ -473,16 +472,12 @@ describe('ReactHooks', () => { expect(root).toMatchRenderedOutput('HELLO'); expect(() => { - // This prints a warning message then throws a null access error root.update(); - }).toThrow('null'); - - if (__DEV__) { - expect(console.error).toHaveBeenCalledTimes(3); - expect(console.error.calls.argsFor(0)[0]).toContain( - 'Warning: Detected a variable number of hook dependencies.', - ); - } + }).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.', + ]); }); it('warns for bad useEffect return values', () => { diff --git a/packages/shared/areHookInputsEqual.js b/packages/shared/areHookInputsEqual.js deleted file mode 100644 index 9ef441d9b6d65..0000000000000 --- a/packages/shared/areHookInputsEqual.js +++ /dev/null @@ -1,36 +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__) { - if (arr1 === null || arr2 === null || arr1.length !== arr2.length) { - warning( - false, - '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 === null ? '(empty)' : `[${arr1.join(', ')}]`, - arr2 === null ? '(empty)' : `[${arr2.join(', ')}]`, - ); - } - } - for (let i = 0; i < arr1.length; i++) { - if (is(arr1[i], arr2[i])) { - continue; - } - return false; - } - return true; -} From c7d8ebc199c003faaae361a32b882ce1426e4056 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 16 Jan 2019 17:32:11 -0800 Subject: [PATCH 3/3] Nits --- packages/react-dom/src/server/ReactPartialRendererHooks.js | 2 +- packages/react-reconciler/src/ReactFiberHooks.js | 2 +- .../react-reconciler/src/__tests__/ReactHooks-test.internal.js | 1 - 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/react-dom/src/server/ReactPartialRendererHooks.js b/packages/react-dom/src/server/ReactPartialRendererHooks.js index 1bd4a98021f21..558c34064b2c5 100644 --- a/packages/react-dom/src/server/ReactPartialRendererHooks.js +++ b/packages/react-dom/src/server/ReactPartialRendererHooks.js @@ -93,7 +93,7 @@ function areHookInputsEqual( ); } } - for (let i = 0; i < nextDeps.length; i++) { + for (let i = 0; i < prevDeps.length && i < nextDeps.length; i++) { if (is(nextDeps[i], prevDeps[i])) { continue; } diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 32a869316fbb2..a75f5cb212921 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -161,7 +161,7 @@ function areHookInputsEqual( ); } } - for (let i = 0; i < nextDeps.length; i++) { + for (let i = 0; i < prevDeps.length && i < nextDeps.length; i++) { if (is(nextDeps[i], prevDeps[i])) { continue; } diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 5ee1be80d1d4f..0e4e2dfacdb05 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -453,7 +453,6 @@ describe('ReactHooks', () => { 'Previous: [A, B]\n' + 'Incoming: [A]\n', ]); - expect(ReactTestRenderer).toHaveYielded(['Did commit: A, B']); }); it('warns if switching from dependencies to no dependencies', () => {