Skip to content
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

Jump to bottom

[ESLint] Add more cases to exhaustive-deps rule #14930

Merged
merged 4 commits into from
Feb 25, 2019
Merged

[ESLint] Add more cases to exhaustive-deps rule #14930

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4a90ed6
Add better message for literal dependencies
gaearon Feb 22, 2019
7831b3c
Warn about ref.current in cleanup phase
gaearon Feb 22, 2019
ae6c4c7
Fix wrong comment
gaearon Feb 25, 2019
e9ea642
Tweak wording
gaearon Feb 25, 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
299 changes: 299 additions & 0 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 @@ -446,6 +446,122 @@ const tests = {
}
`,
},
{
// Valid because we assign ref.current
// ourselves. Therefore it's likely not
// a ref managed by React.
code: `
function MyComponent() {
const myRef = useRef();
useEffect(() => {
const handleMove = () => {};
myRef.current = 42;
return () => {
console.log(myRef.current.toString())
};
}, []);
return <div />;
}
`,
},
{
// Valid because we assign ref.current
// ourselves. Therefore it's likely not
// a ref managed by React.
code: `
function useMyThing(myRef) {
useEffect(() => {
const handleMove = () => {};
myRef.current = 42;
return () => {
console.log(myRef.current.toString())
};
}, [myRef]);
}
`,
},
{
// Valid because the ref is captured.
code: `
function MyComponent() {
const myRef = useRef();
useEffect(() => {
const handleMove = () => {};
const node = myRef.current;
node.addEventListener('mousemove', handleMove);
return () => node.removeEventListener('mousemove', handleMove);
}, []);
return <div ref={myRef} />;
}
`,
},
{
// Valid because we assign ref.current
// ourselves. Therefore it's likely not
// a ref managed by React.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment and the code below seem out of sync. Maybe bad copy-pasta? Maybe should mirror the comment above?

Valid because the ref is captured.

code: `
function useMyThing(myRef) {
const myRef = useRef();
useEffect(() => {
const handleMove = () => {};
const node = myRef.current;
node.addEventListener('mousemove', handleMove);
return () => node.removeEventListener('mousemove', handleMove);
}, [myRef]);
return <div ref={myRef} />;
}
`,
},
{
// Valid because it's not an effect.
code: `
function useMyThing(myRef) {
useCallback(() => {
const handleMouse = () => {};
myRef.current.addEventListener('mousemove', handleMouse);
myRef.current.addEventListener('mousein', handleMouse);
return function() {
setTimeout(() => {
myRef.current.removeEventListener('mousemove', handleMouse);
myRef.current.removeEventListener('mousein', handleMouse);
});
}
}, [myRef]);
}
`,
},
{
// Valid because we read ref.current in a function that isn't cleanup.
code: `
function useMyThing() {
const myRef = useRef();
useEffect(() => {
const handleMove = () => {
console.log(myRef.current)
};
window.addEventListener('mousemove', handleMove);
return () => window.removeEventListener('mousemove', handleMove);
}, []);
return <div ref={myRef} />;
}
`,
},
{
// Valid because we read ref.current in a function that isn't cleanup.
code: `
function useMyThing() {
const myRef = useRef();
useEffect(() => {
const handleMove = () => {
return () => window.removeEventListener('mousemove', handleMove);
};
window.addEventListener('mousemove', handleMove);
return () => {};
}, []);
return <div ref={myRef} />;
}
`,
},
],
invalid: [
{
Expand Down Expand Up @@ -779,6 +895,53 @@ const tests = {
'Either include it or remove the dependency array.',
],
},
{
code: `
function MyComponent({ foo, bar, baz }) {
useEffect(() => {
console.log(foo, bar, baz);
}, ['foo', 'bar']);
}
`,
output: `
function MyComponent({ foo, bar, baz }) {
useEffect(() => {
console.log(foo, bar, baz);
}, [bar, baz, foo]);
}
`,
errors: [
"React Hook useEffect has missing dependencies: 'bar', 'baz', and 'foo'. " +
'Either include them or remove the dependency array.',
"The 'foo' string literal is not a valid dependency because it never changes. " +
'Did you mean to include foo in the array instead?',
"The 'bar' string literal is not a valid dependency because it never changes. " +
'Did you mean to include bar in the array instead?',
],
},
{
code: `
function MyComponent({ foo, bar, baz }) {
useEffect(() => {
console.log(foo, bar, baz);
}, [42, false, null]);
}
`,
output: `
function MyComponent({ foo, bar, baz }) {
useEffect(() => {
console.log(foo, bar, baz);
}, [bar, baz, foo]);
}
`,
errors: [
"React Hook useEffect has missing dependencies: 'bar', 'baz', and 'foo'. " +
'Either include them or remove the dependency array.',
"The '42' literal is not a valid dependency because it never changes. You can safely remove it.",
"The 'false' literal is not a valid dependency because it never changes. You can safely remove it.",
"The 'null' literal is not a valid dependency because it never changes. You can safely remove it.",
],
},
{
code: `
function MyComponent() {
Expand Down Expand Up @@ -1742,6 +1905,142 @@ const tests = {
`and keep the mutable value in its 'current' property.`,
],
},
{
code: `
function MyComponent() {
const myRef = useRef();
useEffect(() => {
const handleMove = () => {};
myRef.current.addEventListener('mousemove', handleMove);
return () => myRef.current.removeEventListener('mousemove', handleMove);
}, []);
return <div ref={myRef} />;
}
`,
output: `
function MyComponent() {
const myRef = useRef();
useEffect(() => {
const handleMove = () => {};
myRef.current.addEventListener('mousemove', handleMove);
return () => myRef.current.removeEventListener('mousemove', handleMove);
}, []);
return <div ref={myRef} />;
}
`,
errors: [
`Accessing 'myRef.current' during ` +
`the effect cleanup is likely a mistake because by this time React ` +
`has already updated the ref. If this ref is managed by React, store ` +
`'myRef.current' in a variable inside ` +
`the effect itself and refer to that variable from the cleanup function.`,
],
},
{
code: `
function useMyThing(myRef) {
useEffect(() => {
const handleMove = () => {};
myRef.current.addEventListener('mousemove', handleMove);
return () => myRef.current.removeEventListener('mousemove', handleMove);
}, [myRef]);
}
`,
output: `
function useMyThing(myRef) {
useEffect(() => {
const handleMove = () => {};
myRef.current.addEventListener('mousemove', handleMove);
return () => myRef.current.removeEventListener('mousemove', handleMove);
}, [myRef]);
}
`,
errors: [
`Accessing 'myRef.current' during ` +
`the effect cleanup is likely a mistake because by this time React ` +
`has already updated the ref. If this ref is managed by React, store ` +
`'myRef.current' in a variable inside ` +
`the effect itself and refer to that variable from the cleanup function.`,
],
},
{
code: `
function useMyThing(myRef) {
useEffect(() => {
const handleMouse = () => {};
myRef.current.addEventListener('mousemove', handleMouse);
myRef.current.addEventListener('mousein', handleMouse);
return function() {
setTimeout(() => {
myRef.current.removeEventListener('mousemove', handleMouse);
myRef.current.removeEventListener('mousein', handleMouse);
});
}
}, [myRef]);
}
`,
output: `
function useMyThing(myRef) {
useEffect(() => {
const handleMouse = () => {};
myRef.current.addEventListener('mousemove', handleMouse);
myRef.current.addEventListener('mousein', handleMouse);
return function() {
setTimeout(() => {
myRef.current.removeEventListener('mousemove', handleMouse);
myRef.current.removeEventListener('mousein', handleMouse);
});
}
}, [myRef]);
}
`,
errors: [
`Accessing 'myRef.current' during ` +
`the effect cleanup is likely a mistake because by this time React ` +
`has already updated the ref. If this ref is managed by React, store ` +
`'myRef.current' in a variable inside ` +
`the effect itself and refer to that variable from the cleanup function.`,
],
},
{
code: `
function useMyThing(myRef, active) {
useEffect(() => {
const handleMove = () => {};
if (active) {
myRef.current.addEventListener('mousemove', handleMove);
return function() {
setTimeout(() => {
myRef.current.removeEventListener('mousemove', handleMove);
});
}
}
}, [myRef, active]);
}
`,
output: `
function useMyThing(myRef, active) {
useEffect(() => {
const handleMove = () => {};
if (active) {
myRef.current.addEventListener('mousemove', handleMove);
return function() {
setTimeout(() => {
myRef.current.removeEventListener('mousemove', handleMove);
});
}
}
}, [myRef, active]);
}
`,
errors: [
`Accessing 'myRef.current' during ` +
`the effect cleanup is likely a mistake because by this time React ` +
`has already updated the ref. If this ref is managed by React, store ` +
`'myRef.current' in a variable inside ` +
`the effect itself and refer to that variable from the cleanup function.`,
],
},
],
};

Expand Down
Loading