diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index ebc5c972648..e2018edf2dd 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -992,6 +992,18 @@ const tests = { } `, }, + { + code: ` + function Hello() { + const [state, setState] = useState(0); + useEffect(() => { + const handleResize = () => setState(window.innerWidth); + window.addEventListener('resize', handleResize); + return () => window.removeEventListener('resize', handleResize); + }); + } + `, + }, ], invalid: [ { @@ -4462,6 +4474,102 @@ const tests = { `Either exclude it or remove the dependency array.`, ], }, + { + code: ` + function Hello() { + const [state, setState] = useState(0); + useEffect(() => { + setState({}); + }); + } + `, + output: ` + function Hello() { + const [state, setState] = useState(0); + useEffect(() => { + setState({}); + }, []); + } + `, + errors: [ + `React Hook useEffect contains a call to 'setState'. ` + + `Without a list of dependencies, this can lead to an infinite chain of updates. ` + + `To fix this, pass [] as a second argument to the useEffect Hook.`, + ], + }, + { + code: ` + function Hello() { + const [data, setData] = useState(0); + useEffect(() => { + fetchData.then(setData); + }); + } + `, + output: ` + function Hello() { + const [data, setData] = useState(0); + useEffect(() => { + fetchData.then(setData); + }, []); + } + `, + errors: [ + `React Hook useEffect contains a call to 'setData'. ` + + `Without a list of dependencies, this can lead to an infinite chain of updates. ` + + `To fix this, pass [] as a second argument to the useEffect Hook.`, + ], + }, + { + code: ` + function Hello({ country }) { + const [data, setData] = useState(0); + useEffect(() => { + fetchData(country).then(setData); + }); + } + `, + output: ` + function Hello({ country }) { + const [data, setData] = useState(0); + useEffect(() => { + fetchData(country).then(setData); + }, [country]); + } + `, + errors: [ + `React Hook useEffect contains a call to 'setData'. ` + + `Without a list of dependencies, this can lead to an infinite chain of updates. ` + + `To fix this, pass [country] as a second argument to the useEffect Hook.`, + ], + }, + { + code: ` + function Hello({ prop1, prop2 }) { + const [state, setState] = useState(0); + useEffect(() => { + if (prop1) { + setState(prop2); + } + }); + } + `, + output: ` + function Hello({ prop1, prop2 }) { + const [state, setState] = useState(0); + useEffect(() => { + if (prop1) { + setState(prop2); + } + }, [prop1, prop2]); + } + `, + errors: [ + `React Hook useEffect contains a call to 'setState'. ` + + `Without a list of dependencies, this can lead to an infinite chain of updates. ` + + `To fix this, pass [prop1, prop2] as a second argument to the useEffect Hook.`, + ], + }, { code: ` function Thing() { diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 887ca54cdb6..f859983f7a3 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -468,9 +468,101 @@ export default { }, ); + // Warn about assigning to variables in the outer scope. + // Those are usually bugs. + let staleAssignments = new Set(); + function reportStaleAssignment(writeExpr, key) { + if (staleAssignments.has(key)) { + return; + } + staleAssignments.add(key); + context.report({ + node: writeExpr, + message: + `Assignments to the '${key}' variable from inside React Hook ` + + `${context.getSource(reactiveHook)} will be lost after each ` + + `render. To preserve the value over time, store it in a useRef ` + + `Hook and keep the mutable value in the '.current' property. ` + + `Otherwise, you can move this variable directly inside ` + + `${context.getSource(reactiveHook)}.`, + }); + } + + // Remember which deps are optional and report bad usage first. + const optionalDependencies = new Set(); + dependencies.forEach(({isStatic, references}, key) => { + if (isStatic) { + optionalDependencies.add(key); + } + references.forEach(reference => { + if (reference.writeExpr) { + reportStaleAssignment(reference.writeExpr, key); + } + }); + }); + + if (staleAssignments.size > 0) { + // The intent isn't clear so we'll wait until you fix those first. + return; + } + if (!declaredDependenciesNode) { + // Check if there are any top-level setState() calls. + // Those tend to lead to infinite loops. + let setStateInsideEffectWithoutDeps = null; + dependencies.forEach(({isStatic, references}, key) => { + if (setStateInsideEffectWithoutDeps) { + return; + } + references.forEach(reference => { + if (setStateInsideEffectWithoutDeps) { + return; + } + + const id = reference.identifier; + const isSetState = setStateCallSites.has(id); + if (!isSetState) { + return; + } + + let fnScope = reference.from; + while (fnScope.type !== 'function') { + fnScope = fnScope.upper; + } + const isDirectlyInsideEffect = fnScope.block === node; + if (isDirectlyInsideEffect) { + // TODO: we could potentially ignore early returns. + setStateInsideEffectWithoutDeps = key; + } + }); + }); + if (setStateInsideEffectWithoutDeps) { + let {suggestedDependencies} = collectRecommendations({ + dependencies, + declaredDependencies: [], + optionalDependencies, + externalDependencies: new Set(), + isEffect: true, + }); + context.report({ + node: node.parent.callee, + message: + `React Hook ${reactiveHookName} contains a call to '${setStateInsideEffectWithoutDeps}'. ` + + `Without a list of dependencies, this can lead to an infinite chain of updates. ` + + `To fix this, pass [` + + suggestedDependencies.join(', ') + + `] as a second argument to the ${reactiveHookName} Hook.`, + fix(fixer) { + return fixer.insertTextAfter( + node, + `, [${suggestedDependencies.join(', ')}]`, + ); + }, + }); + } return; } + const declaredDependencies = []; const externalDependencies = new Set(); if (declaredDependenciesNode.type !== 'ArrayExpression') { @@ -569,43 +661,6 @@ export default { }); } - // Warn about assigning to variables in the outer scope. - // Those are usually bugs. - let staleAssignments = new Set(); - function reportStaleAssignment(writeExpr, key) { - if (staleAssignments.has(key)) { - return; - } - staleAssignments.add(key); - context.report({ - node: writeExpr, - message: - `Assignments to the '${key}' variable from inside React Hook ` + - `${context.getSource(reactiveHook)} will be lost after each ` + - `render. To preserve the value over time, store it in a useRef ` + - `Hook and keep the mutable value in the '.current' property. ` + - `Otherwise, you can move this variable directly inside ` + - `${context.getSource(reactiveHook)}.`, - }); - } - - // Remember which deps are optional and report bad usage first. - const optionalDependencies = new Set(); - dependencies.forEach(({isStatic, references}, key) => { - if (isStatic) { - optionalDependencies.add(key); - } - references.forEach(reference => { - if (reference.writeExpr) { - reportStaleAssignment(reference.writeExpr, key); - } - }); - }); - if (staleAssignments.size > 0) { - // The intent isn't clear so we'll wait until you fix those first. - return; - } - let { suggestedDependencies, unnecessaryDependencies,