diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index 1cef754c752cf..8dd36917b0679 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -2224,7 +2224,189 @@ const tests = { } `, errors: [ - // TODO: make this message clearer since it's not obvious why. + "React Hook useEffect has a missing dependency: 'props'. " + + 'Either include it or remove the dependency array. ' + + 'Alternatively, destructure the necessary props outside the callback.', + ], + }, + { + code: ` + function MyComponent(props) { + useEffect(() => { + function play() { + props.onPlay(); + } + function pause() { + props.onPause(); + } + }, []); + } + `, + output: ` + function MyComponent(props) { + useEffect(() => { + function play() { + props.onPlay(); + } + function pause() { + props.onPause(); + } + }, [props]); + } + `, + errors: [ + "React Hook useEffect has a missing dependency: 'props'. " + + 'Either include it or remove the dependency array. ' + + 'Alternatively, destructure the necessary props outside the callback.', + ], + }, + { + code: ` + function MyComponent(props) { + useEffect(() => { + if (props.foo.onChange) { + props.foo.onChange(); + } + }, []); + } + `, + output: ` + function MyComponent(props) { + useEffect(() => { + if (props.foo.onChange) { + props.foo.onChange(); + } + }, [props.foo]); + } + `, + errors: [ + "React Hook useEffect has a missing dependency: 'props.foo'. " + + 'Either include it or remove the dependency array.', + ], + }, + { + code: ` + function MyComponent(props) { + useEffect(() => { + props.onChange(); + if (props.foo.onChange) { + props.foo.onChange(); + } + }, []); + } + `, + output: ` + function MyComponent(props) { + useEffect(() => { + props.onChange(); + if (props.foo.onChange) { + props.foo.onChange(); + } + }, [props]); + } + `, + errors: [ + "React Hook useEffect has a missing dependency: 'props'. " + + 'Either include it or remove the dependency array. ' + + 'Alternatively, destructure the necessary props outside the callback.', + ], + }, + { + code: ` + function MyComponent(props) { + const [skillsCount] = useState(); + useEffect(() => { + if (skillsCount === 0 && !props.isEditMode) { + props.toggleEditMode(); + } + }, [skillsCount, props.isEditMode, props.toggleEditMode]); + } + `, + output: ` + function MyComponent(props) { + const [skillsCount] = useState(); + useEffect(() => { + if (skillsCount === 0 && !props.isEditMode) { + props.toggleEditMode(); + } + }, [skillsCount, props.isEditMode, props.toggleEditMode, props]); + } + `, + errors: [ + "React Hook useEffect has a missing dependency: 'props'. " + + 'Either include it or remove the dependency array. ' + + 'Alternatively, destructure the necessary props outside the callback.', + ], + }, + { + code: ` + function MyComponent(props) { + const [skillsCount] = useState(); + useEffect(() => { + if (skillsCount === 0 && !props.isEditMode) { + props.toggleEditMode(); + } + }, []); + } + `, + output: ` + function MyComponent(props) { + const [skillsCount] = useState(); + useEffect(() => { + if (skillsCount === 0 && !props.isEditMode) { + props.toggleEditMode(); + } + }, [props, skillsCount]); + } + `, + errors: [ + "React Hook useEffect has missing dependencies: 'props' and 'skillsCount'. " + + 'Either include them or remove the dependency array. ' + + 'Alternatively, destructure the necessary props outside the callback.', + ], + }, + { + code: ` + function MyComponent(props) { + useEffect(() => { + externalCall(props); + props.onChange(); + }, []); + } + `, + output: ` + function MyComponent(props) { + useEffect(() => { + externalCall(props); + props.onChange(); + }, [props]); + } + `, + // Don't suggest to destructure props here since you can't. + errors: [ + "React Hook useEffect has a missing dependency: 'props'. " + + 'Either include it or remove the dependency array.', + ], + }, + { + code: ` + function MyComponent(props) { + useEffect(() => { + props.onChange(); + externalCall(props); + }, []); + } + `, + output: ` + function MyComponent(props) { + useEffect(() => { + props.onChange(); + externalCall(props); + }, [props]); + } + `, + // Don't suggest to destructure props here since you can't. + errors: [ "React Hook useEffect has a missing dependency: 'props'. " + 'Either include it or remove the dependency array.', ], diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 7db679d0cc225..9fd60d960e45c 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -449,6 +449,46 @@ export default { } } + // `props.foo()` marks `props` as a dependency because it has + // a `this` value. This warning can be confusing. + // So if we're going to show it, append a clarification. + if (!extraWarning && missingDependencies.has('props')) { + let propDep = dependencies.get('props'); + if (propDep == null) { + return; + } + const refs = propDep.reference.resolved.references; + if (!Array.isArray(refs)) { + return; + } + let isPropsOnlyUsedInMembers = true; + for (let i = 0; i < refs.length; i++) { + const ref = refs[i]; + const id = fastFindReferenceWithParent( + componentScope.block, + ref.identifier, + ); + if (!id) { + isPropsOnlyUsedInMembers = false; + break; + } + const parent = id.parent; + if (parent == null) { + isPropsOnlyUsedInMembers = false; + break; + } + if (parent.type !== 'MemberExpression') { + isPropsOnlyUsedInMembers = false; + break; + } + } + if (isPropsOnlyUsedInMembers) { + extraWarning = + ' Alternatively, destructure the necessary props ' + + 'outside the callback.'; + } + } + context.report({ node: declaredDependenciesNode, message: