From 2c95cc98db6c2df040b37ac68e06c4b854c3065a Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 5 Mar 2019 15:01:15 +0000 Subject: [PATCH 1/3] Warn on mount when deps are not an array --- .../react-reconciler/src/ReactFiberHooks.js | 11 +++++++ .../src/__tests__/ReactHooks-test.internal.js | 30 +++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 9244864b11c2f..842113b47924d 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -1265,6 +1265,17 @@ if (__DEV__) { ): void { currentHookNameInDev = 'useEffect'; mountHookTypesDev(); + if (deps !== undefined && deps !== null && !Array.isArray(deps)) { + // Verify deps, but only on mount to avoid extra checks. + // It's unlikely their type would change as usually you define them inline. + warning( + false, + '%s received a final argument that is not an array (instead, received `%s`). When ' + + 'specified, the final argument must be an array.', + currentHookNameInDev, + typeof deps, + ); + } return mountEffect(create, deps); }, useImperativeHandle( diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index b960406504925..b962080592b6b 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -632,6 +632,36 @@ describe('ReactHooks', () => { ]); }); + it('warns if deps is not an array', () => { + const {useEffect} = React; + + function App(props) { + useEffect(() => {}, props.deps); + return null; + } + expect(() => { + ReactTestRenderer.create(); + }).toWarnDev([ + 'Warning: useEffect received a final argument that is not an array (instead, received `string`). ' + + 'When specified, the final argument must be an array.', + ]); + expect(() => { + ReactTestRenderer.create(); + }).toWarnDev([ + 'Warning: useEffect received a final argument that is not an array (instead, received `number`). ' + + 'When specified, the final argument must be an array.', + ]); + expect(() => { + ReactTestRenderer.create(); + }).toWarnDev([ + 'Warning: useEffect received a final argument that is not an array (instead, received `object`). ' + + 'When specified, the final argument must be an array.', + ]); + ReactTestRenderer.create(); + ReactTestRenderer.create(); + ReactTestRenderer.create(); + }); + it('assumes useEffect clean-up function is either a function or undefined', () => { const {useLayoutEffect} = React; From 48da01e9da7aeed98d2b23d582041b50a987feac Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 5 Mar 2019 17:15:52 +0000 Subject: [PATCH 2/3] Check other Hooks --- .../react-reconciler/src/ReactFiberHooks.js | 32 ++++++---- .../src/__tests__/ReactHooks-test.internal.js | 58 ++++++++++++++++++- 2 files changed, 78 insertions(+), 12 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 842113b47924d..03e4533602501 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -209,6 +209,22 @@ function updateHookTypesDev() { } } +function checkDepsAreArrayDev(deps: mixed) { + if (__DEV__) { + if (deps !== undefined && deps !== null && !Array.isArray(deps)) { + // Verify deps, but only on mount to avoid extra checks. + // It's unlikely their type would change as usually you define them inline. + warning( + false, + '%s received a final argument that is not an array (instead, received `%s`). When ' + + 'specified, the final argument must be an array.', + currentHookNameInDev, + typeof deps, + ); + } + } +} + function warnOnHookMismatchInDev(currentHookName: HookType) { if (__DEV__) { const componentName = getComponentName( @@ -1249,6 +1265,7 @@ if (__DEV__) { useCallback(callback: T, deps: Array | void | null): T { currentHookNameInDev = 'useCallback'; mountHookTypesDev(); + checkDepsAreArrayDev(deps); return mountCallback(callback, deps); }, useContext( @@ -1265,17 +1282,7 @@ if (__DEV__) { ): void { currentHookNameInDev = 'useEffect'; mountHookTypesDev(); - if (deps !== undefined && deps !== null && !Array.isArray(deps)) { - // Verify deps, but only on mount to avoid extra checks. - // It's unlikely their type would change as usually you define them inline. - warning( - false, - '%s received a final argument that is not an array (instead, received `%s`). When ' + - 'specified, the final argument must be an array.', - currentHookNameInDev, - typeof deps, - ); - } + checkDepsAreArrayDev(deps); return mountEffect(create, deps); }, useImperativeHandle( @@ -1285,6 +1292,7 @@ if (__DEV__) { ): void { currentHookNameInDev = 'useImperativeHandle'; mountHookTypesDev(); + checkDepsAreArrayDev(deps); return mountImperativeHandle(ref, create, deps); }, useLayoutEffect( @@ -1293,11 +1301,13 @@ if (__DEV__) { ): void { currentHookNameInDev = 'useLayoutEffect'; mountHookTypesDev(); + checkDepsAreArrayDev(deps); return mountLayoutEffect(create, deps); }, useMemo(create: () => T, deps: Array | void | null): T { currentHookNameInDev = 'useMemo'; mountHookTypesDev(); + checkDepsAreArrayDev(deps); const prevDispatcher = ReactCurrentDispatcher.current; ReactCurrentDispatcher.current = InvalidNestedHooksDispatcherOnMountInDEV; try { diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index b962080592b6b..81fcc6a00a8f4 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -633,10 +633,13 @@ describe('ReactHooks', () => { }); it('warns if deps is not an array', () => { - const {useEffect} = React; + const {useEffect, useLayoutEffect, useMemo, useCallback} = React; function App(props) { useEffect(() => {}, props.deps); + useLayoutEffect(() => {}, props.deps); + useMemo(() => {}, props.deps); + useCallback(() => {}, props.deps); return null; } expect(() => { @@ -644,24 +647,77 @@ describe('ReactHooks', () => { }).toWarnDev([ 'Warning: useEffect received a final argument that is not an array (instead, received `string`). ' + 'When specified, the final argument must be an array.', + 'Warning: useLayoutEffect received a final argument that is not an array (instead, received `string`). ' + + 'When specified, the final argument must be an array.', + 'Warning: useMemo received a final argument that is not an array (instead, received `string`). ' + + 'When specified, the final argument must be an array.', + 'Warning: useCallback received a final argument that is not an array (instead, received `string`). ' + + 'When specified, the final argument must be an array.', ]); expect(() => { ReactTestRenderer.create(); }).toWarnDev([ 'Warning: useEffect received a final argument that is not an array (instead, received `number`). ' + 'When specified, the final argument must be an array.', + 'Warning: useLayoutEffect received a final argument that is not an array (instead, received `number`). ' + + 'When specified, the final argument must be an array.', + 'Warning: useMemo received a final argument that is not an array (instead, received `number`). ' + + 'When specified, the final argument must be an array.', + 'Warning: useCallback received a final argument that is not an array (instead, received `number`). ' + + 'When specified, the final argument must be an array.', ]); expect(() => { ReactTestRenderer.create(); }).toWarnDev([ 'Warning: useEffect received a final argument that is not an array (instead, received `object`). ' + 'When specified, the final argument must be an array.', + 'Warning: useLayoutEffect received a final argument that is not an array (instead, received `object`). ' + + 'When specified, the final argument must be an array.', + 'Warning: useMemo received a final argument that is not an array (instead, received `object`). ' + + 'When specified, the final argument must be an array.', + 'Warning: useCallback received a final argument that is not an array (instead, received `object`). ' + + 'When specified, the final argument must be an array.', ]); ReactTestRenderer.create(); ReactTestRenderer.create(); ReactTestRenderer.create(); }); + it('warns if deps is not an array for useImperativeHandle', () => { + const {useImperativeHandle} = React; + + const App = React.forwardRef((props, ref) => { + useImperativeHandle(ref, () => {}, props.deps); + return null; + }); + + expect(() => { + ReactTestRenderer.create(); + }).toWarnDev([ + 'Warning: useImperativeHandle received a final argument that is not an array (instead, received `string`). ' + + 'When specified, the final argument must be an array.', + ]); + expect(() => { + expect(() => { + ReactTestRenderer.create(); + }).toWarnDev([ + 'Warning: useImperativeHandle received a final argument that is not an array (instead, received `number`). ' + + 'When specified, the final argument must be an array.', + ]); + }).toThrow('deps.concat is not a function'); + expect(() => { + expect(() => { + ReactTestRenderer.create(); + }).toWarnDev([ + 'Warning: useImperativeHandle received a final argument that is not an array (instead, received `object`). ' + + 'When specified, the final argument must be an array.', + ]); + }).toThrow('deps.concat is not a function'); + ReactTestRenderer.create(); + ReactTestRenderer.create(); + ReactTestRenderer.create(); + }); + it('assumes useEffect clean-up function is either a function or undefined', () => { const {useLayoutEffect} = React; From 72b1308ce19631884b841a62f6b4c2890b42eef2 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 5 Mar 2019 17:28:21 +0000 Subject: [PATCH 3/3] I can't figure out how to fix error/warning nesting lint But it doesn't really matter much because we test other cases in the other test. --- .../src/__tests__/ReactHooks-test.internal.js | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 81fcc6a00a8f4..087c8572cd66e 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -642,6 +642,7 @@ describe('ReactHooks', () => { useCallback(() => {}, props.deps); return null; } + expect(() => { ReactTestRenderer.create(); }).toWarnDev([ @@ -697,22 +698,6 @@ describe('ReactHooks', () => { 'Warning: useImperativeHandle received a final argument that is not an array (instead, received `string`). ' + 'When specified, the final argument must be an array.', ]); - expect(() => { - expect(() => { - ReactTestRenderer.create(); - }).toWarnDev([ - 'Warning: useImperativeHandle received a final argument that is not an array (instead, received `number`). ' + - 'When specified, the final argument must be an array.', - ]); - }).toThrow('deps.concat is not a function'); - expect(() => { - expect(() => { - ReactTestRenderer.create(); - }).toWarnDev([ - 'Warning: useImperativeHandle received a final argument that is not an array (instead, received `object`). ' + - 'When specified, the final argument must be an array.', - ]); - }).toThrow('deps.concat is not a function'); ReactTestRenderer.create(); ReactTestRenderer.create(); ReactTestRenderer.create();