diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDependencies-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDependencies-test.js index fb66b55fe99f0..7ec8c90c17c67 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDependencies-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDependencies-test.js @@ -133,6 +133,15 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, [foo]); } `, + ` + // Regression test + function MyComponent({ foo }) { + useEffect(() => { + console.log(foo.length); + console.log(foo.slice(0)); + }, [foo]); + } + `, ` // Regression test function MyComponent({ history }) { @@ -169,6 +178,19 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { }, [props.foo, props.bar, local]); } `, + { + code: ` + // TODO: we might want to warn "props.foo" + // is extraneous because we already have "props". + function MyComponent(props) { + const local = 42; + useEffect(() => { + console.log(props.foo); + console.log(props.bar); + }, [props, props.foo]); + } + `, + }, { code: ` function MyComponent(props) { @@ -492,6 +514,19 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, { missingError('local'), ], }, + { + code: ` + function MyComponent(props) { + const local = 42; + useEffect(() => { + console.log(props.foo); + console.log(props.bar); + console.log(local); + }, [props]); + } + `, + errors: [missingError('local')], + }, { code: ` function MyComponent(props) { diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js b/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js index b3c3a220874ea..6da40cce853cd 100644 --- a/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js +++ b/packages/eslint-plugin-react-hooks/src/ReactiveDependencies.js @@ -272,6 +272,8 @@ export default { } } + let usedDependencies = new Set(); + // Loop through all our dependencies to make sure they have been declared. // If the dependency has not been declared we need to report some errors. for (const [dependency, dependencyNodes] of dependencies) { @@ -281,11 +283,10 @@ export default { let candidate = dependency; while (candidate) { if (declaredDependencies.has(candidate)) { - // Yay! Our dependency has been declared. Delete it from - // `declaredDependencies` since we will report all remaining unused - // declared dependencies. + // Yay! Our dependency has been declared. + // Record this so we don't report is unused. isDeclared = true; - declaredDependencies.delete(candidate); + usedDependencies.add(candidate); break; } const lastDotAccessIndex = candidate.lastIndexOf('.'); @@ -316,11 +317,15 @@ export default { // Loop through all the unused declared dependencies and report a warning // so the programmer removes the unused dependency from their list. - for (const declaredDependencyNode of declaredDependencies.values()) { + for (const [dependency, dependencyNode] of declaredDependencies) { + if (usedDependencies.has(dependency)) { + continue; + } + context.report( - declaredDependencyNode, + dependencyNode, `React Hook ${context.getSource(reactiveHook)} has an extra ` + - `dependency "${context.getSource(declaredDependencyNode)}" which ` + + `dependency "${context.getSource(dependencyNode)}" which ` + 'is not used in its callback. Removing this dependency may mean ' + 'the hook needs to execute fewer times.', );