From 3ada82b7416c51290535a054f345b99378c38dcb Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 27 Feb 2019 16:59:11 +0000 Subject: [PATCH] Allow extraneous effect dependencies (#14967) This makes cases like useEffect(() => { window.scrollTo(0, 0); }, [activeTab]); not warn. However, it still warns for unused useCallback/useMemo deps. --- .../ESLintRuleExhaustiveDeps-test.js | 107 ++++++++++++++---- .../src/ExhaustiveDeps.js | 14 ++- 2 files changed, 99 insertions(+), 22 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index d0cc18823874b..75bcfd0778d95 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -95,7 +95,7 @@ const tests = { const local1 = {}; { const local2 = {}; - useEffect(() => { + useCallback(() => { console.log(local1); console.log(local2); }, [local1, local2]); @@ -109,7 +109,7 @@ const tests = { const local1 = {}; function MyNestedComponent() { const local2 = {}; - useEffect(() => { + useCallback(() => { console.log(local1); console.log(local2); }, [local2]); @@ -590,6 +590,17 @@ const tests = { } `, }, + { + // Valid even though activeTab is "unused". + // We allow that in effects, but not callbacks or memo. + code: ` + function Foo({ activeTab }) { + useEffect(() => { + window.scrollTo(0, 0); + }, [activeTab]); + } + `, + }, ], invalid: [ { @@ -809,7 +820,7 @@ const tests = { function MyComponent() { const local1 = {}; const local2 = {}; - useEffect(() => { + useMemo(() => { console.log(local1); }, [local1, local2]); } @@ -818,13 +829,13 @@ const tests = { function MyComponent() { const local1 = {}; const local2 = {}; - useEffect(() => { + useMemo(() => { console.log(local1); }, [local1]); } `, errors: [ - "React Hook useEffect has an unnecessary dependency: 'local2'. " + + "React Hook useMemo has an unnecessary dependency: 'local2'. " + 'Either exclude it or remove the dependency array.', ], }, @@ -836,7 +847,7 @@ const tests = { const local1 = {}; function MyNestedComponent() { const local2 = {}; - useEffect(() => { + useCallback(() => { console.log(local1); console.log(local2); }, [local1]); @@ -848,7 +859,7 @@ const tests = { const local1 = {}; function MyNestedComponent() { const local2 = {}; - useEffect(() => { + useCallback(() => { console.log(local1); console.log(local2); }, [local2]); @@ -857,7 +868,7 @@ const tests = { `, errors: [ // Focus on the more important part first (missing dep) - "React Hook useEffect has a missing dependency: 'local2'. " + + "React Hook useCallback has a missing dependency: 'local2'. " + 'Either include it or remove the dependency array.', ], }, @@ -912,16 +923,16 @@ const tests = { { code: ` function MyComponent() { - useEffect(() => {}, [local]); + useCallback(() => {}, [local]); } `, output: ` function MyComponent() { - useEffect(() => {}, []); + useCallback(() => {}, []); } `, errors: [ - "React Hook useEffect has an unnecessary dependency: 'local'. " + + "React Hook useCallback has an unnecessary dependency: 'local'. " + 'Either exclude it or remove the dependency array.', ], }, @@ -1229,7 +1240,6 @@ const tests = { ], }, { - // TODO: need to think more about this case. code: ` function MyComponent() { const local = {id: 42}; @@ -1238,13 +1248,14 @@ const tests = { }, [local.id]); } `, - // TODO: this may not be a good idea. + // TODO: autofix should be smart enough + // to remove local.id from the list. output: ` function MyComponent() { const local = {id: 42}; useEffect(() => { console.log(local); - }, [local]); + }, [local, local.id]); } `, errors: [ @@ -1278,7 +1289,7 @@ const tests = { code: ` function MyComponent() { const local1 = {}; - useEffect(() => { + useCallback(() => { const local1 = {}; console.log(local1); }, [local1]); @@ -1287,14 +1298,14 @@ const tests = { output: ` function MyComponent() { const local1 = {}; - useEffect(() => { + useCallback(() => { const local1 = {}; console.log(local1); }, []); } `, errors: [ - "React Hook useEffect has an unnecessary dependency: 'local1'. " + + "React Hook useCallback has an unnecessary dependency: 'local1'. " + 'Either exclude it or remove the dependency array.', ], }, @@ -1302,17 +1313,17 @@ const tests = { code: ` function MyComponent() { const local1 = {}; - useEffect(() => {}, [local1]); + useCallback(() => {}, [local1]); } `, output: ` function MyComponent() { const local1 = {}; - useEffect(() => {}, []); + useCallback(() => {}, []); } `, errors: [ - "React Hook useEffect has an unnecessary dependency: 'local1'. " + + "React Hook useCallback has an unnecessary dependency: 'local1'. " + 'Either exclude it or remove the dependency array.', ], }, @@ -1785,6 +1796,62 @@ const tests = { "because their mutation doesn't re-render the component.", ], }, + { + code: ` + function MyComponent({ activeTab }) { + const ref1 = useRef(); + const ref2 = useRef(); + useEffect(() => { + ref1.current.scrollTop = 0; + ref2.current.scrollTop = 0; + }, [ref1.current, ref2.current, activeTab]); + } + `, + output: ` + function MyComponent({ activeTab }) { + const ref1 = useRef(); + const ref2 = useRef(); + useEffect(() => { + ref1.current.scrollTop = 0; + ref2.current.scrollTop = 0; + }, [activeTab]); + } + `, + errors: [ + "React Hook useEffect has unnecessary dependencies: 'ref1.current' and 'ref2.current'. " + + 'Either exclude them or remove the dependency array. ' + + "Mutable values like 'ref1.current' aren't valid dependencies " + + "because their mutation doesn't re-render the component.", + ], + }, + { + code: ` + function MyComponent({ activeTab, initY }) { + const ref1 = useRef(); + const ref2 = useRef(); + const fn = useCallback(() => { + ref1.current.scrollTop = initY; + ref2.current.scrollTop = initY; + }, [ref1.current, ref2.current, activeTab, initY]); + } + `, + output: ` + function MyComponent({ activeTab, initY }) { + const ref1 = useRef(); + const ref2 = useRef(); + const fn = useCallback(() => { + ref1.current.scrollTop = initY; + ref2.current.scrollTop = initY; + }, [initY]); + } + `, + errors: [ + "React Hook useCallback has unnecessary dependencies: 'activeTab', 'ref1.current', and 'ref2.current'. " + + 'Either exclude them or remove the dependency array. ' + + "Mutable values like 'ref1.current' aren't valid dependencies " + + "because their mutation doesn't re-render the component.", + ], + }, { code: ` function MyComponent() { diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index b06560bfcee14..5d5cabc2e7ea9 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -330,8 +330,18 @@ export default { duplicateDependencies.add(key); } } else { - // Unnecessary dependency. Do nothing. - unnecessaryDependencies.add(key); + if (isEffect && !key.endsWith('.current')) { + // Effects may have extra "unnecessary" deps. + // Such as resetting scroll when ID changes. + // The exception is ref.current which is always wrong. + // Consider them legit. + if (suggestedDependencies.indexOf(key) === -1) { + suggestedDependencies.push(key); + } + } else { + // Unnecessary dependency. Remember to report it. + unnecessaryDependencies.add(key); + } } });