From f8b30f9b03b6b8e92d20def263854b0ad8413332 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 1 Mar 2019 18:58:29 +0000 Subject: [PATCH 1/3] Suggest to destructure props when they are only used as members --- .../ESLintRuleExhaustiveDeps-test.js | 107 +++++++++++++++++- .../src/ExhaustiveDeps.js | 37 ++++++ 2 files changed, 143 insertions(+), 1 deletion(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index 1cef754c752cf..da02d3c98ecaf 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -2224,7 +2224,112 @@ 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) { + 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.', ], diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 7db679d0cc225..5b23f29d8dafe 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -449,6 +449,43 @@ 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 = 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: From 75a5db9a7b443e7bbef4df7be9b5abedc7ac371e Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 1 Mar 2019 19:23:09 +0000 Subject: [PATCH 2/3] Add more tests --- .../ESLintRuleExhaustiveDeps-test.js | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index da02d3c98ecaf..0a1916a5abd72 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -2311,6 +2311,60 @@ const tests = { '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) { From 9e795c8293298210fa1c8e0f71c388c4eeb4bc5a Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 1 Mar 2019 19:47:21 +0000 Subject: [PATCH 3/3] Fix a bug --- .../ESLintRuleExhaustiveDeps-test.js | 23 +++++++++++++++++++ .../src/ExhaustiveDeps.js | 15 +++++++----- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index 0a1916a5abd72..8dd36917b0679 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -2388,6 +2388,29 @@ const tests = { '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.', + ], + }, { code: ` function MyComponent(props) { diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 5b23f29d8dafe..9fd60d960e45c 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -464,7 +464,10 @@ export default { let isPropsOnlyUsedInMembers = true; for (let i = 0; i < refs.length; i++) { const ref = refs[i]; - const id = ref.identifier; + const id = fastFindReferenceWithParent( + componentScope.block, + ref.identifier, + ); if (!id) { isPropsOnlyUsedInMembers = false; break; @@ -478,11 +481,11 @@ export default { isPropsOnlyUsedInMembers = false; break; } - if (isPropsOnlyUsedInMembers) { - extraWarning = - ' Alternatively, destructure the necessary props ' + - 'outside the callback.'; - } + } + if (isPropsOnlyUsedInMembers) { + extraWarning = + ' Alternatively, destructure the necessary props ' + + 'outside the callback.'; } }