Skip to content

[ESLint] Tweak setState updater message and add useEffect(async) warning #15055

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

Merged
merged 2 commits into from
Mar 7, 2019
Merged

[ESLint] Tweak setState updater message and add useEffect(async) warning #15055

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f913b24
Use first letter in setCount(c => ...) suggestion
gaearon Mar 7, 2019
ee45567
Warn about async effects
gaearon Mar 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 56 additions & 4 deletions packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -949,6 +949,29 @@ const tests = {
}
`,
},
{
code: `
function App() {
const [query, setQuery] = useState('react');
const [state, setState] = useState(null);
useEffect(() => {
let ignore = false;
fetchSomething();
async function fetchSomething() {
const result = await (await fetch('http://hn.algolia.com/api/v1/search?query=' + query)).json();
if (!ignore) setState(result);
}
return () => { ignore = true; };
}, [query]);
return (
<>
<input value={query} onChange={e => setQuery(e.target.value)} />
{JSON.stringify(state)}
</>
);
}
`,
},
],
invalid: [
{
Expand Down Expand Up @@ -2304,7 +2327,7 @@ const tests = {
errors: [
"React Hook useEffect has a missing dependency: 'state'. " +
'Either include it or remove the dependency array. ' +
`You can also write 'setState(state => ...)' ` +
`You can also write 'setState(s => ...)' ` +
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you decide to do a single character here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Showed it to people and they were confused seeing the same variable name because it wasn't obvious it's being shadowed. This explanation clicked tho.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess I'm open to other changes, just want to avoid the shadowing. If people think this is confusing we can brainstorm more options.

Copy link
Contributor

Choose a reason for hiding this comment

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

No I trust your judgement, I was just curious. It makes sense. Thanks!
(ashamed to say I usually use x =>..., which people around me hate)

`if you only use 'state' for the 'setState' call.`,
],
},
Expand Down Expand Up @@ -2335,7 +2358,7 @@ const tests = {
errors: [
"React Hook useEffect has a missing dependency: 'state'. " +
'Either include it or remove the dependency array. ' +
`You can also write 'setState(state => ...)' ` +
`You can also write 'setState(s => ...)' ` +
`if you only use 'state' for the 'setState' call.`,
],
},
Expand Down Expand Up @@ -3933,7 +3956,7 @@ const tests = {
errors: [
"React Hook useEffect has a missing dependency: 'count'. " +
'Either include it or remove the dependency array. ' +
`You can also write 'setCount(count => ...)' if you ` +
`You can also write 'setCount(c => ...)' if you ` +
`only use 'count' for the 'setCount' call.`,
],
},
Expand Down Expand Up @@ -3971,7 +3994,7 @@ const tests = {
errors: [
"React Hook useEffect has missing dependencies: 'count' and 'increment'. " +
'Either include them or remove the dependency array. ' +
`You can also write 'setCount(count => ...)' if you ` +
`You can also write 'setCount(c => ...)' if you ` +
`only use 'count' for the 'setCount' call.`,
],
},
Expand Down Expand Up @@ -4379,6 +4402,35 @@ const tests = {
`Either exclude it or remove the dependency array.`,
],
},
{
code: `
function Thing() {
useEffect(async () => {}, []);
}
`,
output: `
function Thing() {
useEffect(async () => {}, []);
}
`,
errors: [
`Effect callbacks are synchronous to prevent race conditions. ` +
`Put the async function inside:\n\n` +
`useEffect(() => {\n` +
` let ignore = false;\n` +
` fetchSomething();\n` +
`\n` +
` async function fetchSomething() {\n` +
` const result = await ...\n` +
` if (!ignore) setState(result);\n` +
` }\n` +
`\n` +
` return () => { ignore = true; };\n` +
`}, []);\n` +
`\n` +
`This lets you handle multiple requests without bugs.`,
],
},
],
};

Expand Down
31 changes: 28 additions & 3 deletions packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,28 @@ export default {
return;
}

if (isEffect && node.async) {
context.report({
node: node,
message:
`Effect callbacks are synchronous to prevent race conditions. ` +
`Put the async function inside:\n\n` +
`useEffect(() => {\n` +
` let ignore = false;\n` +
` fetchSomething();\n` +
`\n` +
` async function fetchSomething() {\n` +
` const result = await ...\n` +
` if (!ignore) setState(result);\n` +
` }\n` +
`\n` +
` return () => { ignore = true; };\n` +
`}, []);\n` +
`\n` +
`This lets you handle multiple requests without bugs.`,
});
}

// Get the current scope.
const scope = context.getScope();

Expand Down Expand Up @@ -890,9 +912,12 @@ export default {
break;
case 'updater':
extraWarning =
` You can also write '${setStateRecommendation.setter}(${
setStateRecommendation.missingDep
} => ...)' if you only use '${
` You can also write '${
setStateRecommendation.setter
}(${setStateRecommendation.missingDep.substring(
0,
1,
)} => ...)' if you only use '${
setStateRecommendation.missingDep
}'` + ` for the '${setStateRecommendation.setter}' call.`;
break;
Expand Down