Skip to content

Commit

Permalink
Warn about dependencies outside of render scope (#14990)
Browse files Browse the repository at this point in the history
  • Loading branch information
@gaearon
gaearon committed Mar 1, 2019
1 parent df7b476 commit 59ef284
Show file tree
Hide file tree
Showing 2 changed files with 189 additions and 15 deletions.
174 changes: 160 additions & 14 deletions packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -592,15 +592,6 @@ const tests = {
}
`,
},
{
// It is valid for effects to over-specify their deps.
// TODO: maybe only allow local ones, and disallow this example.
code: `
function MyComponent() {
useEffect(() => {}, [window]);
}
`,
},
{
// It is valid for effects to over-specify their deps.
code: `
Expand Down Expand Up @@ -897,8 +888,6 @@ const tests = {
],
},
{
// TODO: this case is weird.
// Maybe it should not consider local1 unused despite component nesting?
code: `
function MyComponent() {
const local1 = {};
Expand All @@ -924,9 +913,10 @@ const tests = {
}
`,
errors: [
// Focus on the more important part first (missing dep)
"React Hook useCallback has a missing dependency: 'local2'. " +
'Either include it or remove the dependency array.',
'Either include it or remove the dependency array. ' +
"Values like 'local1' aren't valid dependencies " +
"because their mutation doesn't re-render the component.",
],
},
{
Expand Down Expand Up @@ -990,7 +980,9 @@ const tests = {
`,
errors: [
"React Hook useCallback has an unnecessary dependency: 'window'. " +
'Either exclude it or remove the dependency array.',
'Either exclude it or remove the dependency array. ' +
"Values like 'window' aren't valid dependencies " +
"because their mutation doesn't re-render the component.",
],
},
{
Expand Down Expand Up @@ -2530,6 +2522,160 @@ const tests = {
'Either include it or remove the dependency array.',
],
},
{
code: `
function MyComponent() {
useEffect(() => {
window.scrollTo(0, 0);
}, [window]);
}
`,
errors: [
"React Hook useEffect has an unnecessary dependency: 'window'. " +
'Either exclude it or remove the dependency array. ' +
"Values like 'window' aren't valid dependencies " +
"because their mutation doesn't re-render the component.",
],
},
{
code: `
import MutableStore from 'store';
function MyComponent() {
useEffect(() => {
console.log(MutableStore.hello);
}, [MutableStore.hello]);
}
`,
output: `
import MutableStore from 'store';
function MyComponent() {
useEffect(() => {
console.log(MutableStore.hello);
}, []);
}
`,
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 " +
"because their mutation doesn't re-render the component.",
],
},
{
code: `
import MutableStore from 'store';
let z = {};
function MyComponent(props) {
let x = props.foo;
{
let y = props.bar;
useEffect(() => {
console.log(MutableStore.hello.world, props.foo, x, y, z, global.stuff);
}, [MutableStore.hello.world, props.foo, x, y, z, global.stuff]);
}
}
`,
output: `
import MutableStore from 'store';
let z = {};
function MyComponent(props) {
let x = props.foo;
{
let y = props.bar;
useEffect(() => {
console.log(MutableStore.hello.world, props.foo, x, y, z, global.stuff);
}, [props.foo, x, y]);
}
}
`,
errors: [
'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 " +
"because their mutation doesn't re-render the component.",
],
},
{
code: `
import MutableStore from 'store';
let z = {};
function MyComponent(props) {
let x = props.foo;
{
let y = props.bar;
useEffect(() => {
// nothing
}, [MutableStore.hello.world, props.foo, x, y, z, global.stuff]);
}
}
`,
// The output should contain the ones that are inside a component
// since there are legit reasons to over-specify them for effects.
output: `
import MutableStore from 'store';
let z = {};
function MyComponent(props) {
let x = props.foo;
{
let y = props.bar;
useEffect(() => {
// nothing
}, [props.foo, x, y]);
}
}
`,
errors: [
'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 " +
"because their mutation doesn't re-render the component.",
],
},
{
code: `
import MutableStore from 'store';
let z = {};
function MyComponent(props) {
let x = props.foo;
{
let y = props.bar;
const fn = useCallback(() => {
// nothing
}, [MutableStore.hello.world, props.foo, x, y, z, global.stuff]);
}
}
`,
output: `
import MutableStore from 'store';
let z = {};
function MyComponent(props) {
let x = props.foo;
{
let y = props.bar;
const fn = useCallback(() => {
// nothing
}, []);
}
}
`,
errors: [
'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 " +
"because their mutation doesn't re-render the component.",
],
},
],
};

Expand Down
30 changes: 29 additions & 1 deletion packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ export default {
// scope. We can't enforce this in a lint so we trust that all variables
// declared outside of pure scope are indeed frozen.
const pureScopes = new Set();
let componentScope = null;
{
let currentScope = scope.upper;
while (currentScope) {
Expand All @@ -101,6 +102,7 @@ export default {
if (!currentScope) {
return;
}
componentScope = currentScope;
}

// These are usually mistaken. Collect them.
Expand Down Expand Up @@ -224,6 +226,7 @@ export default {
);

const declaredDependencies = [];
const externalDependencies = new Set();
if (declaredDependenciesNode.type !== 'ArrayExpression') {
// If the declared dependencies are not an array expression then we
// can't verify that the user provided the correct dependencies. Tell
Expand Down Expand Up @@ -298,11 +301,24 @@ export default {
throw error;
}
}

let maybeID = declaredDependencyNode;
while (maybeID.type === 'MemberExpression') {
maybeID = maybeID.object;
}
const isDeclaredInComponent = !componentScope.through.some(
ref => ref.identifier === maybeID,
);

// Add the dependency to our declared dependency map.
declaredDependencies.push({
key: declaredDependency,
node: declaredDependencyNode,
});

if (!isDeclaredInComponent) {
externalDependencies.add(declaredDependency);
}
});
}

Expand Down Expand Up @@ -348,6 +364,7 @@ export default {
dependencies,
declaredDependencies,
optionalDependencies,
externalDependencies,
isEffect,
});

Expand All @@ -370,6 +387,7 @@ export default {
dependencies,
declaredDependencies: [], // Pretend we don't know
optionalDependencies,
externalDependencies,
isEffect,
}).suggestedDependencies;
}
Expand Down Expand Up @@ -423,6 +441,11 @@ export default {
extraWarning =
` Mutable values like '${badRef}' aren't valid dependencies ` +
"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.`;
}
}

Expand Down Expand Up @@ -462,6 +485,7 @@ function collectRecommendations({
dependencies,
declaredDependencies,
optionalDependencies,
externalDependencies,
isEffect,
}) {
// Our primary data structure.
Expand Down Expand Up @@ -584,7 +608,11 @@ function collectRecommendations({
duplicateDependencies.add(key);
}
} else {
if (isEffect && !key.endsWith('.current')) {
if (
isEffect &&
!key.endsWith('.current') &&
!externalDependencies.has(key)
) {
// Effects are allowed extra "unnecessary" deps.
// Such as resetting scroll when ID changes.
// Consider them legit.
Expand Down

0 comments on commit 59ef284

Please sign in to comment.