Skip to content

Commit

Permalink
[ESLint] Tweak setState updater message and add useEffect(async) warn…
Browse files Browse the repository at this point in the history
…ing (#15055)

* Use first letter in setCount(c => ...) suggestion

In-person testing showed using original variable name confuses people.

* Warn about async effects
  • Loading branch information
gaearon authored Mar 7, 2019
1 parent eb6247a commit 03ad9c7
Show file tree
Hide file tree
Showing 2 changed files with 84 additions and 7 deletions.
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 => ...)' ` +
`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
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

0 comments on commit 03ad9c7

Please sign in to comment.