Skip to content

[ESLint] Clearer messages #15053

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 1 commit into from
Mar 7, 2019
Merged

[ESLint] Clearer messages #15053

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
112 changes: 59 additions & 53 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 @@ -1236,7 +1236,7 @@ const tests = {
errors: [
"React Hook useCallback has a missing dependency: 'local2'. " +
'Either include it or remove the dependency array. ' +
"Values like 'local1' aren't valid dependencies " +
"Outer scope values like 'local1' aren't valid dependencies " +
"because their mutation doesn't re-render the component.",
],
},
Expand Down Expand Up @@ -1302,7 +1302,7 @@ const tests = {
errors: [
"React Hook useCallback has an unnecessary dependency: 'window'. " +
'Either exclude it or remove the dependency array. ' +
"Values like 'window' aren't valid dependencies " +
"Outer scope values like 'window' aren't valid dependencies " +
"because their mutation doesn't re-render the component.",
],
},
Expand Down Expand Up @@ -2304,9 +2304,8 @@ const tests = {
errors: [
"React Hook useEffect has a missing dependency: 'state'. " +
'Either include it or remove the dependency array. ' +
`If 'state' is only necessary for calculating the next state, ` +
`consider refactoring to the setState(state => ...) form which ` +
`doesn't need to depend on the state from outside.`,
`You can also write 'setState(state => ...)' ` +
`if you only use 'state' for the 'setState' call.`,
],
},
{
Expand Down Expand Up @@ -2336,9 +2335,8 @@ const tests = {
errors: [
"React Hook useEffect has a missing dependency: 'state'. " +
'Either include it or remove the dependency array. ' +
`If 'state' is only necessary for calculating the next state, ` +
`consider refactoring to the setState(state => ...) form which ` +
`doesn't need to depend on the state from outside.`,
`You can also write 'setState(state => ...)' ` +
`if you only use 'state' for the 'setState' call.`,
],
},
{
Expand Down Expand Up @@ -3066,7 +3064,7 @@ const tests = {
errors: [
"React Hook useEffect has an unnecessary dependency: 'window'. " +
'Either exclude it or remove the dependency array. ' +
"Values like 'window' aren't valid dependencies " +
"Outer scope values like 'window' aren't valid dependencies " +
"because their mutation doesn't re-render the component.",
],
},
Expand All @@ -3092,7 +3090,7 @@ const tests = {
errors: [
"React Hook useEffect has an unnecessary dependency: 'MutableStore.hello'. " +
'Either exclude it or remove the dependency array. ' +
"Values like 'MutableStore.hello' aren't valid dependencies " +
"Outer scope values like 'MutableStore.hello' aren't valid dependencies " +
"because their mutation doesn't re-render the component.",
],
},
Expand Down Expand Up @@ -3129,7 +3127,7 @@ const tests = {
'React Hook useEffect has unnecessary dependencies: ' +
"'MutableStore.hello.world', 'global.stuff', and 'z'. " +
'Either exclude them or remove the dependency array. ' +
"Values like 'MutableStore.hello.world' aren't valid dependencies " +
"Outer scope values like 'MutableStore.hello.world' aren't valid dependencies " +
"because their mutation doesn't re-render the component.",
],
},
Expand Down Expand Up @@ -3168,7 +3166,7 @@ const tests = {
'React Hook useEffect has unnecessary dependencies: ' +
"'MutableStore.hello.world', 'global.stuff', and 'z'. " +
'Either exclude them or remove the dependency array. ' +
"Values like 'MutableStore.hello.world' aren't valid dependencies " +
"Outer scope values like 'MutableStore.hello.world' aren't valid dependencies " +
"because their mutation doesn't re-render the component.",
],
},
Expand Down Expand Up @@ -3205,7 +3203,7 @@ const tests = {
'React Hook useCallback has unnecessary dependencies: ' +
"'MutableStore.hello.world', 'global.stuff', 'props.foo', 'x', 'y', and 'z'. " +
'Either exclude them or remove the dependency array. ' +
"Values like 'MutableStore.hello.world' aren't valid dependencies " +
"Outer scope values like 'MutableStore.hello.world' aren't valid dependencies " +
"because their mutation doesn't re-render the component.",
],
},
Expand Down Expand Up @@ -3468,8 +3466,7 @@ const tests = {
errors: [
`The 'handleNext' function makes the dependencies of ` +
`useEffect Hook (at line 11) change on every render. ` +
`To fix this, move the 'handleNext' function ` +
`inside the useEffect callback (at line 9). Alternatively, ` +
`Move it inside the useEffect callback. Alternatively, ` +
`wrap the 'handleNext' definition into its own useCallback() Hook.`,
],
},
Expand Down Expand Up @@ -3507,8 +3504,7 @@ const tests = {
errors: [
`The 'handleNext' function makes the dependencies of ` +
`useEffect Hook (at line 11) change on every render. ` +
`To fix this, move the 'handleNext' function ` +
`inside the useEffect callback (at line 9). Alternatively, ` +
`Move it inside the useEffect callback. Alternatively, ` +
`wrap the 'handleNext' definition into its own useCallback() Hook.`,
],
},
Expand Down Expand Up @@ -3605,17 +3601,14 @@ const tests = {
`,
errors: [
"The 'handleNext1' function makes the dependencies of useEffect Hook " +
"(at line 14) change on every render. To fix this, move the 'handleNext1' " +
'function inside the useEffect callback (at line 12). Alternatively, wrap the ' +
"'handleNext1' definition into its own useCallback() Hook.",
'(at line 14) change on every render. Move it inside the useEffect callback. ' +
"Alternatively, wrap the 'handleNext1' definition into its own useCallback() Hook.",
"The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " +
"(at line 17) change on every render. To fix this, move the 'handleNext2' " +
'function inside the useLayoutEffect callback (at line 15). Alternatively, wrap the ' +
"'handleNext2' definition into its own useCallback() Hook.",
'(at line 17) change on every render. Move it inside the useLayoutEffect callback. ' +
"Alternatively, wrap the 'handleNext2' definition into its own useCallback() Hook.",
"The 'handleNext3' function makes the dependencies of useMemo Hook " +
"(at line 20) change on every render. To fix this, move the 'handleNext3' " +
'function inside the useMemo callback (at line 18). Alternatively, wrap the ' +
"'handleNext3' definition into its own useCallback() Hook.",
'(at line 20) change on every render. Move it inside the useMemo callback. ' +
"Alternatively, wrap the 'handleNext3' definition into its own useCallback() Hook.",
],
},
{
Expand Down Expand Up @@ -3673,17 +3666,14 @@ const tests = {
`,
errors: [
"The 'handleNext1' function makes the dependencies of useEffect Hook " +
"(at line 15) change on every render. To fix this, move the 'handleNext1' " +
'function inside the useEffect callback (at line 12). Alternatively, wrap the ' +
"'handleNext1' definition into its own useCallback() Hook.",
'(at line 15) change on every render. Move it inside the useEffect callback. ' +
"Alternatively, wrap the 'handleNext1' definition into its own useCallback() Hook.",
"The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " +
"(at line 19) change on every render. To fix this, move the 'handleNext2' " +
'function inside the useLayoutEffect callback (at line 16). Alternatively, wrap the ' +
"'handleNext2' definition into its own useCallback() Hook.",
'(at line 19) change on every render. Move it inside the useLayoutEffect callback. ' +
"Alternatively, wrap the 'handleNext2' definition into its own useCallback() Hook.",
"The 'handleNext3' function makes the dependencies of useMemo Hook " +
"(at line 23) change on every render. To fix this, move the 'handleNext3' " +
'function inside the useMemo callback (at line 20). Alternatively, wrap the ' +
"'handleNext3' definition into its own useCallback() Hook.",
'(at line 23) change on every render. Move it inside the useMemo callback. ' +
"Alternatively, wrap the 'handleNext3' definition into its own useCallback() Hook.",
],
},
{
Expand Down Expand Up @@ -3907,8 +3897,7 @@ const tests = {
errors: [
`The 'handleNext' function makes the dependencies of ` +
`useEffect Hook (at line 14) change on every render. ` +
`To fix this, move the 'handleNext' function inside ` +
`the useEffect callback (at line 12). Alternatively, wrap the ` +
`Move it inside the useEffect callback. Alternatively, wrap the ` +
`'handleNext' definition into its own useCallback() Hook.`,
],
},
Expand Down Expand Up @@ -3944,9 +3933,8 @@ const tests = {
errors: [
"React Hook useEffect has a missing dependency: 'count'. " +
'Either include it or remove the dependency array. ' +
`If 'count' is only necessary for calculating the next state, ` +
`consider refactoring to the setCount(count => ...) form which ` +
`doesn't need to depend on the state from outside.`,
`You can also write 'setCount(count => ...)' if you ` +
`only use 'count' for the 'setCount' call.`,
],
},
{
Expand Down Expand Up @@ -3983,9 +3971,8 @@ const tests = {
errors: [
"React Hook useEffect has missing dependencies: 'count' and 'increment'. " +
'Either include them or remove the dependency array. ' +
`If 'count' is only necessary for calculating the next state, ` +
`consider refactoring to the setCount(count => ...) form which ` +
`doesn't need to depend on the state from outside.`,
`You can also write 'setCount(count => ...)' if you ` +
`only use 'count' for the 'setCount' call.`,
],
},
{
Expand Down Expand Up @@ -4022,9 +4009,8 @@ const tests = {
errors: [
"React Hook useEffect has a missing dependency: 'increment'. " +
'Either include it or remove the dependency array. ' +
`If 'increment' is only necessary for calculating the next state, ` +
`consider refactoring to the useReducer Hook. This ` +
`lets you move the calculation of next state outside the effect.`,
`You can also replace multiple useState variables with useReducer ` +
`if 'setCount' needs the current value of 'increment'.`,
],
},
{
Expand Down Expand Up @@ -4150,8 +4136,7 @@ const tests = {
`,
errors: [
`The 'increment' function makes the dependencies of useEffect Hook ` +
`(at line 14) change on every render. To fix this, move the ` +
`'increment' function inside the useEffect callback (at line 9). ` +
`(at line 14) change on every render. Move it inside the useEffect callback. ` +
`Alternatively, wrap the \'increment\' definition into its own ` +
`useCallback() Hook.`,
],
Expand Down Expand Up @@ -4188,11 +4173,8 @@ const tests = {
errors: [
"React Hook useEffect has a missing dependency: 'increment'. " +
'Either include it or remove the dependency array. ' +
`If 'increment' is only necessary for calculating the next state, ` +
`consider refactoring to the useReducer Hook. This lets you move ` +
`the calculation of next state outside the effect. ` +
`You can then read 'increment' from the reducer ` +
`by putting it directly in your component.`,
'You can also replace useState with an inline useReducer ' +
`if 'setCount' needs the current value of 'increment'.`,
],
},
{
Expand Down Expand Up @@ -4373,6 +4355,30 @@ const tests = {
`find the parent component that defines it and wrap that definition in useCallback.`,
],
},
{
// The mistake here is that it was moved inside the effect
// so it can't be referenced in the deps array.
code: `
function Thing() {
useEffect(() => {
const fetchData = async () => {};
fetchData();
}, [fetchData]);
}
`,
output: `
function Thing() {
useEffect(() => {
const fetchData = async () => {};
fetchData();
}, []);
}
`,
errors: [
`React Hook useEffect has an unnecessary dependency: 'fetchData'. ` +
`Either exclude it or remove the dependency array.`,
],
},
],
};

Expand Down
47 changes: 21 additions & 26 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 @@ -617,10 +617,7 @@ export default {
}' definition into its own useCallback() Hook.`;
} else {
message +=
` To fix this, move the '${fn.name.name}' function ` +
`inside the ${reactiveHookName} callback (at line ${
node.loc.start.line
}). ` +
` Move it inside the ${reactiveHookName} callback. ` +
`Alternatively, wrap the '${
fn.name.name
}' definition into its own useCallback() Hook.`;
Expand Down Expand Up @@ -715,9 +712,13 @@ export default {
"because their mutation doesn't re-render the component.";
} else if (externalDependencies.size > 0) {
const dep = Array.from(externalDependencies)[0];
extraWarning =
` Values like '${dep}' aren't valid dependencies ` +
`because their mutation doesn't re-render the component.`;
// Don't show this warning for things that likely just got moved *inside* the callback
// because in that case they're clearly not referring to globals.
if (!scope.set.has(dep)) {
extraWarning =
` Outer scope values like '${dep}' aren't valid dependencies ` +
`because their mutation doesn't re-render the component.`;
}
}
}

Expand Down Expand Up @@ -874,36 +875,30 @@ export default {
}
});
if (setStateRecommendation !== null) {
let suggestion;
switch (setStateRecommendation.form) {
case 'reducer':
suggestion =
'useReducer Hook. This lets you move the calculation ' +
'of next state outside the effect.';
extraWarning =
` You can also replace multiple useState variables with useReducer ` +
`if '${setStateRecommendation.setter}' needs the ` +
`current value of '${setStateRecommendation.missingDep}'.`;
break;
case 'inlineReducer':
suggestion =
'useReducer Hook. This lets you move the calculation ' +
'of next state outside the effect. You can then ' +
`read '${
setStateRecommendation.missingDep
}' from the reducer ` +
`by putting it directly in your component.`;
extraWarning =
` You can also replace useState with an inline useReducer ` +
`if '${setStateRecommendation.setter}' needs the ` +
`current value of '${setStateRecommendation.missingDep}'.`;
break;
case 'updater':
suggestion =
`${setStateRecommendation.setter}(${
extraWarning =
` You can also write '${setStateRecommendation.setter}(${
setStateRecommendation.missingDep
} => ...) form ` +
`which doesn't need to depend on the state from outside.`;
} => ...)' if you only use '${
setStateRecommendation.missingDep
}'` + ` for the '${setStateRecommendation.setter}' call.`;
break;
default:
throw new Error('Unknown case.');
}
extraWarning =
` If '${setStateRecommendation.missingDep}'` +
` is only necessary for calculating the next state, ` +
`consider refactoring to the ${suggestion}`;
}
}

Expand Down