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; -}