Skip to content

Commit

Permalink
Allow extraneous effect dependencies (#14967)
Browse files Browse the repository at this point in the history 
This makes cases like

  useEffect(() => {
    window.scrollTo(0, 0);
  }, [activeTab]);

not warn.

However, it still warns for unused useCallback/useMemo deps.
  • Loading branch information
@gaearon
gaearon authored Feb 27, 2019
1 parent 00748c5 commit 3ada82b
Show file tree
Hide file tree
Showing 2 changed files with 99 additions and 22 deletions.
107 changes: 87 additions & 20 deletions packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ const tests = {
const local1 = {};
{
const local2 = {};
useEffect(() => {
useCallback(() => {
console.log(local1);
console.log(local2);
}, [local1, local2]);
Expand All @@ -109,7 +109,7 @@ const tests = {
const local1 = {};
function MyNestedComponent() {
const local2 = {};
useEffect(() => {
useCallback(() => {
console.log(local1);
console.log(local2);
}, [local2]);
Expand Down Expand Up @@ -590,6 +590,17 @@ const tests = {
}
`,
},
{
// Valid even though activeTab is "unused".
// We allow that in effects, but not callbacks or memo.
code: `
function Foo({ activeTab }) {
useEffect(() => {
window.scrollTo(0, 0);
}, [activeTab]);
}
`,
},
],
invalid: [
{
Expand Down Expand Up @@ -809,7 +820,7 @@ const tests = {
function MyComponent() {
const local1 = {};
const local2 = {};
useEffect(() => {
useMemo(() => {
console.log(local1);
}, [local1, local2]);
}
Expand All @@ -818,13 +829,13 @@ const tests = {
function MyComponent() {
const local1 = {};
const local2 = {};
useEffect(() => {
useMemo(() => {
console.log(local1);
}, [local1]);
}
`,
errors: [
"React Hook useEffect has an unnecessary dependency: 'local2'. " +
"React Hook useMemo has an unnecessary dependency: 'local2'. " +
'Either exclude it or remove the dependency array.',
],
},
Expand All @@ -836,7 +847,7 @@ const tests = {
const local1 = {};
function MyNestedComponent() {
const local2 = {};
useEffect(() => {
useCallback(() => {
console.log(local1);
console.log(local2);
}, [local1]);
Expand All @@ -848,7 +859,7 @@ const tests = {
const local1 = {};
function MyNestedComponent() {
const local2 = {};
useEffect(() => {
useCallback(() => {
console.log(local1);
console.log(local2);
}, [local2]);
Expand All @@ -857,7 +868,7 @@ const tests = {
`,
errors: [
// Focus on the more important part first (missing dep)
"React Hook useEffect has a missing dependency: 'local2'. " +
"React Hook useCallback has a missing dependency: 'local2'. " +
'Either include it or remove the dependency array.',
],
},
Expand Down Expand Up @@ -912,16 +923,16 @@ const tests = {
{
code: `
function MyComponent() {
useEffect(() => {}, [local]);
useCallback(() => {}, [local]);
}
`,
output: `
function MyComponent() {
useEffect(() => {}, []);
useCallback(() => {}, []);
}
`,
errors: [
"React Hook useEffect has an unnecessary dependency: 'local'. " +
"React Hook useCallback has an unnecessary dependency: 'local'. " +
'Either exclude it or remove the dependency array.',
],
},
Expand Down Expand Up @@ -1229,7 +1240,6 @@ const tests = {
],
},
{
// TODO: need to think more about this case.
code: `
function MyComponent() {
const local = {id: 42};
Expand All @@ -1238,13 +1248,14 @@ const tests = {
}, [local.id]);
}
`,
// TODO: this may not be a good idea.
// TODO: autofix should be smart enough
// to remove local.id from the list.
output: `
function MyComponent() {
const local = {id: 42};
useEffect(() => {
console.log(local);
}, [local]);
}, [local, local.id]);
}
`,
errors: [
Expand Down Expand Up @@ -1278,7 +1289,7 @@ const tests = {
code: `
function MyComponent() {
const local1 = {};
useEffect(() => {
useCallback(() => {
const local1 = {};
console.log(local1);
}, [local1]);
Expand All @@ -1287,32 +1298,32 @@ const tests = {
output: `
function MyComponent() {
const local1 = {};
useEffect(() => {
useCallback(() => {
const local1 = {};
console.log(local1);
}, []);
}
`,
errors: [
"React Hook useEffect has an unnecessary dependency: 'local1'. " +
"React Hook useCallback has an unnecessary dependency: 'local1'. " +
'Either exclude it or remove the dependency array.',
],
},
{
code: `
function MyComponent() {
const local1 = {};
useEffect(() => {}, [local1]);
useCallback(() => {}, [local1]);
}
`,
output: `
function MyComponent() {
const local1 = {};
useEffect(() => {}, []);
useCallback(() => {}, []);
}
`,
errors: [
"React Hook useEffect has an unnecessary dependency: 'local1'. " +
"React Hook useCallback has an unnecessary dependency: 'local1'. " +
'Either exclude it or remove the dependency array.',
],
},
Expand Down Expand Up @@ -1785,6 +1796,62 @@ const tests = {
"because their mutation doesn't re-render the component.",
],
},
{
code: `
function MyComponent({ activeTab }) {
const ref1 = useRef();
const ref2 = useRef();
useEffect(() => {
ref1.current.scrollTop = 0;
ref2.current.scrollTop = 0;
}, [ref1.current, ref2.current, activeTab]);
}
`,
output: `
function MyComponent({ activeTab }) {
const ref1 = useRef();
const ref2 = useRef();
useEffect(() => {
ref1.current.scrollTop = 0;
ref2.current.scrollTop = 0;
}, [activeTab]);
}
`,
errors: [
"React Hook useEffect has unnecessary dependencies: 'ref1.current' and 'ref2.current'. " +
'Either exclude them or remove the dependency array. ' +
"Mutable values like 'ref1.current' aren't valid dependencies " +
"because their mutation doesn't re-render the component.",
],
},
{
code: `
function MyComponent({ activeTab, initY }) {
const ref1 = useRef();
const ref2 = useRef();
const fn = useCallback(() => {
ref1.current.scrollTop = initY;
ref2.current.scrollTop = initY;
}, [ref1.current, ref2.current, activeTab, initY]);
}
`,
output: `
function MyComponent({ activeTab, initY }) {
const ref1 = useRef();
const ref2 = useRef();
const fn = useCallback(() => {
ref1.current.scrollTop = initY;
ref2.current.scrollTop = initY;
}, [initY]);
}
`,
errors: [
"React Hook useCallback has unnecessary dependencies: 'activeTab', 'ref1.current', and 'ref2.current'. " +
'Either exclude them or remove the dependency array. ' +
"Mutable values like 'ref1.current' aren't valid dependencies " +
"because their mutation doesn't re-render the component.",
],
},
{
code: `
function MyComponent() {
Expand Down
14 changes: 12 additions & 2 deletions packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -330,8 +330,18 @@ export default {
duplicateDependencies.add(key);
}
} else {
// Unnecessary dependency. Do nothing.
unnecessaryDependencies.add(key);
if (isEffect && !key.endsWith('.current')) {
// Effects may have extra "unnecessary" deps.
// Such as resetting scroll when ID changes.
// The exception is ref.current which is always wrong.
// Consider them legit.
if (suggestedDependencies.indexOf(key) === -1) {
suggestedDependencies.push(key);
}
} else {
// Unnecessary dependency. Remember to report it.
unnecessaryDependencies.add(key);
}
}
});

Expand Down

0 comments on commit 3ada82b

Please sign in to comment.