Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add eslint-plugin-react-hooks/exhaustive-deps rule to check stale closure dependencies #14636

Merged
merged 33 commits into from Feb 20, 2019
Merged

Add eslint-plugin-react-hooks/exhaustive-deps rule to check stale closure dependencies #14636

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
c0eafd8
Add ESLint rule for useEffect/useCallback/useMemo Hook dependencies
calebmer Oct 31, 2018
6e2ba67
Fix ReactiveDependencies rule
jamiebuilds Oct 31, 2018
4704a98
fix lint errors
jamiebuilds Oct 31, 2018
6f67ef1
Support useLayoutEffect
gaearon Jan 10, 2019
da75f31
Add some failing tests and comments
gaearon Jan 10, 2019
58183fe
Gather dependencies in child scopes too
gaearon Jan 11, 2019
d1515f4
If we don't find foo.bar.baz in deps, try foo.bar, then foo
gaearon Jan 11, 2019
57ebe56
foo is enough for both foo.bar and foo.baz
gaearon Jan 11, 2019
369aa61
Shorter rule name
gaearon Jan 17, 2019
2df41d8
Add fixable meta
gaearon Jan 18, 2019
3b3d7da
Remove a bunch of code and start from scratch
gaearon Jan 19, 2019
d4d501b
[WIP] Only report errors from dependency array
gaearon Jan 19, 2019
695693c
Fix typo
gaearon Jan 19, 2019
340bffb
[Temp] Skip all tests
gaearon Feb 13, 2019
3832eb3
Fix the first test
gaearon Feb 13, 2019
244a1ff
Revamp the test suite
gaearon Feb 14, 2019
58e38ca
Fix [foo] to include foo.bar
gaearon Feb 15, 2019
0aea58e
Don't suggest call expressions
gaearon Feb 15, 2019
14b2229
Special case 'current' for refs
gaearon Feb 15, 2019
366d5df
Don't complain about known static deps
gaearon Feb 18, 2019
3d46a1e
Support useImperativeHandle
gaearon Feb 18, 2019
3679627
Better punctuation and formatting
gaearon Feb 18, 2019
5d2c991
More uniform message format
gaearon Feb 19, 2019
e0203cb
Treat React.useRef/useState/useReducer as static too
gaearon Feb 19, 2019
54c2e16
Add special message for ref.current
gaearon Feb 19, 2019
6fd2d12
Add a TODO case
gaearon Feb 19, 2019
0c722bb
Alphabetize the autofix
gaearon Feb 20, 2019
12aeaca
Only alphabetize if it already was
gaearon Feb 20, 2019
c40bcaa
Don't add static deps by default
gaearon Feb 20, 2019
56785b5
Add an undefined variable case
gaearon Feb 20, 2019
294b474
Tweak wording
gaearon Feb 20, 2019
2f86a69
Rename to exhaustive-deps
gaearon Feb 20, 2019
8b554ca
Clean up / refactor a little bit
gaearon Feb 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
54 changes: 44 additions & 10 deletions packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js
View file
Open in desktop
Expand Up @@ -174,6 +174,17 @@ const tests = {
}
`,
},
{
// TODO: we might want to forbid dot-access in deps.
code: `
function MyComponent(props) {
useEffect(() => {
console.log(props.foo);
console.log(props.bar);
}, [props.bar, props.foo]);
}
`,
},
{
// TODO: we might want to forbid dot-access in deps.
code: `
Expand Down Expand Up @@ -991,11 +1002,34 @@ const tests = {
useEffect(() => {
console.log(props.foo);
console.log(props.bar);
}, [props.foo, props.bar]);
}, [props.bar, props.foo]);
}
`,
errors: [
"React Hook useEffect has missing dependencies: 'props.bar' and 'props.foo'. " +
'Either include them or remove the dependency array.',
],
},
{
code: `
function MyComponent(props) {
let a, b, c, d, e, f, g;
useEffect(() => {
console.log(b, e, d, c, a, g, f);
}, [c, a, g]);
}
`,
// Alphabetize during the autofix.
output: `
function MyComponent(props) {
let a, b, c, d, e, f, g;
useEffect(() => {
console.log(b, e, d, c, a, g, f);
}, [a, b, c, d, e, f, g]);
}
`,
errors: [
"React Hook useEffect has missing dependencies: 'props.foo' and 'props.bar'. " +
"React Hook useEffect has missing dependencies: 'b', 'd', 'e', and 'f'. " +
'Either include them or remove the dependency array.',
],
},
Expand All @@ -1019,11 +1053,11 @@ const tests = {
console.log(props.foo);
console.log(props.bar);
console.log(local);
}, [props.foo, props.bar, local]);
}, [local, props.bar, props.foo]);
}
`,
errors: [
"React Hook useEffect has missing dependencies: 'props.foo', 'props.bar', and 'local'. " +
"React Hook useEffect has missing dependencies: 'local', 'props.bar', and 'props.foo'. " +
'Either include them or remove the dependency array.',
],
},
Expand All @@ -1045,7 +1079,7 @@ const tests = {
console.log(props.foo);
console.log(props.bar);
console.log(local);
}, [props, local]);
}, [local, props]);
}
`,
errors: [
Expand Down Expand Up @@ -1260,11 +1294,11 @@ const tests = {
console.log(ref2.current.textContent);
alert(props.someOtherRefs.current.innerHTML);
fetch(props.color);
}, [ref1, ref2, props.someOtherRefs, props.color]);
}, [props.color, props.someOtherRefs, ref1, ref2]);
}
`,
errors: [
"React Hook useEffect has missing dependencies: 'props.someOtherRefs' and 'props.color'. " +
"React Hook useEffect has missing dependencies: 'props.color' and 'props.someOtherRefs'. " +
'Either include them or remove the dependency array.',
],
},
Expand All @@ -1290,7 +1324,7 @@ const tests = {
console.log(ref2.current.textContent);
alert(props.someOtherRefs.current.innerHTML);
fetch(props.color);
}, [props.someOtherRefs, props.color, ref1, ref2]);
}, [props.color, props.someOtherRefs, ref1, ref2]);
}
`,
errors: [
Expand Down Expand Up @@ -1365,12 +1399,12 @@ const tests = {
if (props.onChange) {
props.onChange();
}
}, [props.onChange, props]);
}, [props, props.onChange]);
}
`,
errors: [
// TODO: reporting props separately is superfluous. Fix to just props.onChange.
"React Hook useEffect has missing dependencies: 'props.onChange' and 'props'. " +
"React Hook useEffect has missing dependencies: 'props' and 'props.onChange'. " +
'Either include them or remove the dependency array.',
],
},
Expand Down
8 changes: 7 additions & 1 deletion packages/eslint-plugin-react-hooks/src/ReactiveDeps.js
View file
Open in desktop
Expand Up @@ -341,6 +341,8 @@ export default {
// Already did that. Do nothing.
}
});
// Alphabetize for the autofix.
suggestedDependencies.sort();

const problemCount =
duplicateDependencies.size +
Expand Down Expand Up @@ -376,7 +378,11 @@ export default {
' ' +
(set.size > 1 ? 'dependencies' : 'dependency') +
': ' +
join(Array.from(set).map(quote)) +
join(
Array.from(set)
.sort()
.map(quote),
) +
Copy link
Contributor

@Jessidhia Jessidhia Feb 20, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this could also be space for a future "sorted-deps" rule? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One simple compromise would be to only add at the end by default, but alphabetize if the current array is empty.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually we could alphabetize if existing list is alphabetized. 😏

`. Either ${fixVerb} ${
set.size > 1 ? 'them' : 'it'
} or remove the dependency array.`
Expand Down