From 1e887e5c7cb1dad608ba5eaec031b0793833a8b9 Mon Sep 17 00:00:00 2001 From: Aaron Pettengill Date: Sat, 11 Apr 2020 16:07:29 -0400 Subject: [PATCH 1/3] Add test cases for support exhaustive deps ending in Effect --- .../ESLintRuleExhaustiveDeps-test.js | 130 +++++++++++++++++- 1 file changed, 123 insertions(+), 7 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index feaf672b9bdb3..e84d3477c616f 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -360,32 +360,50 @@ const tests = { { code: normalizeIndent` function MyComponent(props) { - useCustomEffect(() => { + useCustomHook(() => { console.log(props.foo); }); } `, - options: [{additionalHooks: 'useCustomEffect'}], + options: [{additionalHooks: 'useCustomHook'}], }, { code: normalizeIndent` function MyComponent(props) { - useCustomEffect(() => { + useCustomHook(() => { console.log(props.foo); }, [props.foo]); } `, - options: [{additionalHooks: 'useCustomEffect'}], + options: [{additionalHooks: 'useCustomHook'}], }, { code: normalizeIndent` function MyComponent(props) { - useCustomEffect(() => { + useCustomHook(() => { console.log(props.foo); }, []); } `, - options: [{additionalHooks: 'useAnotherEffect'}], + options: [{additionalHooks: 'useAnotherHook'}], + }, + { + code: normalizeIndent` + function MyComponent(props) { + useCustomEffect(() => { + console.log(props.foo); + }); + } + `, + }, + { + code: normalizeIndent` + function MyComponent(props) { + useCustomEffect(() => { + console.log(props.foo); + }, [props.foo]); + } + `, }, { // Valid because we don't care about hooks outside of components. @@ -3002,6 +3020,105 @@ const tests = { }, ], }, + { + code: normalizeIndent` + function MyComponent(props) { + useCustomHook(() => { + console.log(props.foo); + }, []); + useEffect(() => { + console.log(props.foo); + }, []); + React.useEffect(() => { + console.log(props.foo); + }, []); + React.useCustomHook(() => { + console.log(props.foo); + }, []); + } + `, + options: [{additionalHooks: 'useCustomHook'}], + errors: [ + { + message: + "React Hook useCustomHook has a missing dependency: 'props.foo'. " + + 'Either include it or remove the dependency array.', + suggestions: [ + { + desc: 'Update the dependencies array to be: [props.foo]', + output: normalizeIndent` + function MyComponent(props) { + useCustomHook(() => { + console.log(props.foo); + }, [props.foo]); + useEffect(() => { + console.log(props.foo); + }, []); + React.useEffect(() => { + console.log(props.foo); + }, []); + React.useCustomHook(() => { + console.log(props.foo); + }, []); + } + `, + }, + ], + }, + { + message: + "React Hook useEffect has a missing dependency: 'props.foo'. " + + 'Either include it or remove the dependency array.', + suggestions: [ + { + desc: 'Update the dependencies array to be: [props.foo]', + output: normalizeIndent` + function MyComponent(props) { + useCustomHook(() => { + console.log(props.foo); + }, []); + useEffect(() => { + console.log(props.foo); + }, [props.foo]); + React.useEffect(() => { + console.log(props.foo); + }, []); + React.useCustomHook(() => { + console.log(props.foo); + }, []); + } + `, + }, + ], + }, + { + message: + "React Hook React.useEffect has a missing dependency: 'props.foo'. " + + 'Either include it or remove the dependency array.', + suggestions: [ + { + desc: 'Update the dependencies array to be: [props.foo]', + output: normalizeIndent` + function MyComponent(props) { + useCustomHook(() => { + console.log(props.foo); + }, []); + useEffect(() => { + console.log(props.foo); + }, []); + React.useEffect(() => { + console.log(props.foo); + }, [props.foo]); + React.useCustomHook(() => { + console.log(props.foo); + }, []); + } + `, + }, + ], + }, + ], + }, { code: normalizeIndent` function MyComponent(props) { @@ -3019,7 +3136,6 @@ const tests = { }, []); } `, - options: [{additionalHooks: 'useCustomEffect'}], errors: [ { message: From 70f95c9a1e6edccc1eaea5ee018f91296679966d Mon Sep 17 00:00:00 2001 From: Aaron Pettengill Date: Sat, 11 Apr 2020 16:15:20 -0400 Subject: [PATCH 2/3] Apply the exhaustive deps lint rule to any hook ending with Effect --- packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index d2cb068562046..c96efa08002cc 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -1504,7 +1504,9 @@ function getReactiveHookCallbackIndex(calleeNode, options) { // useImperativeHandle(ref, fn) return 1; default: - if (node === calleeNode && options && options.additionalHooks) { + if (node === calleeNode && node.name.match(/use.+Effect/)) { + return 0; + } else if (node === calleeNode && options && options.additionalHooks) { // Allow the user to provide a regular expression which enables the lint to // target custom reactive hooks. let name; From 47994e7bbb0c80639b7b42802f3be273754fa470 Mon Sep 17 00:00:00 2001 From: Aaron Pettengill Date: Sat, 11 Apr 2020 16:38:25 -0400 Subject: [PATCH 3/3] Add another test for supporting linting useXEffect hooks --- .../ESLintRuleExhaustiveDeps-test.js | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index e84d3477c616f..9d20d062461fe 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -4170,6 +4170,36 @@ const tests = { ], options: [{additionalHooks: 'useLayoutEffect_SAFE_FOR_SSR'}], }, + { + code: ` + function MyComponent() { + const myRef = useRef(); + useIsomorphicLayoutEffect(() => { + const handleMove = () => {}; + myRef.current.addEventListener('mousemove', handleMove); + return () => myRef.current.removeEventListener('mousemove', handleMove); + }); + return
; + } + `, + output: ` + function MyComponent() { + const myRef = useRef(); + useIsomorphicLayoutEffect(() => { + const handleMove = () => {}; + myRef.current.addEventListener('mousemove', handleMove); + return () => myRef.current.removeEventListener('mousemove', handleMove); + }); + return
; + } + `, + errors: [ + `The ref value 'myRef.current' will likely have changed by the time ` + + `this effect cleanup function runs. If this ref points to a node ` + + `rendered by React, copy 'myRef.current' to a variable inside the effect, ` + + `and use that variable in the cleanup function.`, + ], + }, { // Autofix ignores constant primitives (leaving the ones that are there). code: normalizeIndent`