Skip to content

Commit

Permalink
Feature: Add support to exhaustive-deps rule for any hook ending wi…
Browse files Browse the repository at this point in the history 
…th `Effect` (#18580)

* Add test cases for support exhaustive deps ending in Effect

* Apply the exhaustive deps lint rule to any hook ending with Effect

* Add another test for supporting linting useXEffect hooks

Co-authored-by: Aaron Pettengill <aaron.pettengill@echoman.com>
  • Loading branch information
@airjp73
airjp73 and Aaron Pettengill committed May 1, 2020
1 parent eab9440 commit 5ac9ca7
Show file tree
Hide file tree
Showing 2 changed files with 156 additions and 8 deletions.
160 changes: 153 additions & 7 deletions packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
View file
Expand Up @@ -360,32 +360,50 @@ const tests = {
{
code: normalizeIndent`
function MyComponent(props) {
useCustomEffect(() => {
useCustomHook(() => {
console.log(props.foo);
});
}
`,
options: [{additionalHooks: 'useCustomEffect'}],
options: [{additionalHooks: 'useCustomHook'}],
},
{
code: normalizeIndent`
function MyComponent(props) {
useCustomEffect(() => {
useCustomHook(() => {
console.log(props.foo);
}, [props.foo]);
}
`,
options: [{additionalHooks: 'useCustomEffect'}],
options: [{additionalHooks: 'useCustomHook'}],
},
{
code: normalizeIndent`
function MyComponent(props) {
useCustomEffect(() => {
useCustomHook(() => {
console.log(props.foo);
}, []);
}
`,
options: [{additionalHooks: 'useAnotherEffect'}],
options: [{additionalHooks: 'useAnotherHook'}],
},
{
code: normalizeIndent`
function MyComponent(props) {
useCustomEffect(() => {
console.log(props.foo);
});
}
`,
},
{
code: normalizeIndent`
function MyComponent(props) {
useCustomEffect(() => {
console.log(props.foo);
}, [props.foo]);
}
`,
},
{
// Valid because we don't care about hooks outside of components.
Expand Down Expand Up @@ -3002,6 +3020,105 @@ const tests = {
},
],
},
{
code: normalizeIndent`
function MyComponent(props) {
useCustomHook(() => {
console.log(props.foo);
}, []);
useEffect(() => {
console.log(props.foo);
}, []);
React.useEffect(() => {
console.log(props.foo);
}, []);
React.useCustomHook(() => {
console.log(props.foo);
}, []);
}
`,
options: [{additionalHooks: 'useCustomHook'}],
errors: [
{
message:
"React Hook useCustomHook has a missing dependency: 'props.foo'. " +
'Either include it or remove the dependency array.',
suggestions: [
{
desc: 'Update the dependencies array to be: [props.foo]',
output: normalizeIndent`
function MyComponent(props) {
useCustomHook(() => {
console.log(props.foo);
}, [props.foo]);
useEffect(() => {
console.log(props.foo);
}, []);
React.useEffect(() => {
console.log(props.foo);
}, []);
React.useCustomHook(() => {
console.log(props.foo);
}, []);
}
`,
},
],
},
{
message:
"React Hook useEffect has a missing dependency: 'props.foo'. " +
'Either include it or remove the dependency array.',
suggestions: [
{
desc: 'Update the dependencies array to be: [props.foo]',
output: normalizeIndent`
function MyComponent(props) {
useCustomHook(() => {
console.log(props.foo);
}, []);
useEffect(() => {
console.log(props.foo);
}, [props.foo]);
React.useEffect(() => {
console.log(props.foo);
}, []);
React.useCustomHook(() => {
console.log(props.foo);
}, []);
}
`,
},
],
},
{
message:
"React Hook React.useEffect has a missing dependency: 'props.foo'. " +
'Either include it or remove the dependency array.',
suggestions: [
{
desc: 'Update the dependencies array to be: [props.foo]',
output: normalizeIndent`
function MyComponent(props) {
useCustomHook(() => {
console.log(props.foo);
}, []);
useEffect(() => {
console.log(props.foo);
}, []);
React.useEffect(() => {
console.log(props.foo);
}, [props.foo]);
React.useCustomHook(() => {
console.log(props.foo);
}, []);
}
`,
},
],
},
],
},
{
code: normalizeIndent`
function MyComponent(props) {
Expand All @@ -3019,7 +3136,6 @@ const tests = {
}, []);
}
`,
options: [{additionalHooks: 'useCustomEffect'}],
errors: [
{
message:
Expand Down Expand Up @@ -4054,6 +4170,36 @@ const tests = {
],
options: [{additionalHooks: 'useLayoutEffect_SAFE_FOR_SSR'}],
},
{
code: `
function MyComponent() {
const myRef = useRef();
useIsomorphicLayoutEffect(() => {
const handleMove = () => {};
myRef.current.addEventListener('mousemove', handleMove);
return () => myRef.current.removeEventListener('mousemove', handleMove);
});
return <div ref={myRef} />;
}
`,
output: `
function MyComponent() {
const myRef = useRef();
useIsomorphicLayoutEffect(() => {
const handleMove = () => {};
myRef.current.addEventListener('mousemove', handleMove);
return () => myRef.current.removeEventListener('mousemove', handleMove);
});
return <div ref={myRef} />;
}
`,
errors: [
`The ref value 'myRef.current' will likely have changed by the time ` +
`this effect cleanup function runs. If this ref points to a node ` +
`rendered by React, copy 'myRef.current' to a variable inside the effect, ` +
`and use that variable in the cleanup function.`,
],
},
{
// Autofix ignores constant primitives (leaving the ones that are there).
code: normalizeIndent`
Expand Down
4 changes: 3 additions & 1 deletion packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
View file
Expand Up @@ -1504,7 +1504,9 @@ function getReactiveHookCallbackIndex(calleeNode, options) {
// useImperativeHandle(ref, fn)
return 1;
default:
if (node === calleeNode && options && options.additionalHooks) {
if (node === calleeNode && node.name.match(/use.+Effect/)) {

This comment has been minimized.

Sign in to view

Copy link
@coolkev

coolkev May 4, 2020

Contributor

This is matching on any hook containing the word Effect, not just ending with Effect. Can this be changed to node.name.match(/use.+Effect$/) It is now causing false matches for hooks that I use named useXyzSideEffects()
return 0;
} else if (node === calleeNode && options && options.additionalHooks) {
// Allow the user to provide a regular expression which enables the lint to
// target custom reactive hooks.
let name;
Expand Down

0 comments on commit 5ac9ca7

Please sign in to comment.