Skip to content

Commit 04eccd1

Browse files
gaearongaearon
committed
Suggest useCallback when bare function is a dependency
1 parent ef382a2 commit 04eccd1

File tree

2 files changed

+227
-29
lines changed

2 files changed

+227
-29
lines changed

‎packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js

Lines changed: 138 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -313,7 +313,7 @@ const tests = {
313313
},
314314
{
315315
code: `
316-
function MyComponent({ maybeRef2 }) {
316+
function MyComponent({ maybeRef2, foo }) {
317317
const definitelyRef1 = useRef();
318318
const definitelyRef2 = useRef();
319319
const maybeRef1 = useSomeOtherRefyThing();
@@ -323,8 +323,8 @@ const tests = {
323323
const [state4, dispatch2] = React.useReducer();
324324
const [state5, maybeSetState] = useFunnyState();
325325
const [state6, maybeDispatch] = useFunnyReducer();
326-
function mySetState() {}
327-
function myDispatch() {}
326+
const mySetState = useCallback(() => {}, []);
327+
let myDispatch = useCallback(() => {}, []);
328328
329329
useEffect(() => {
330330
// Known to be static
@@ -380,8 +380,8 @@ const tests = {
380380
const [state5, maybeSetState] = useFunnyState();
381381
const [state6, maybeDispatch] = useFunnyReducer();
382382
383-
function mySetState() {}
384-
function myDispatch() {}
383+
const mySetState = useCallback(() => {}, []);
384+
let myDispatch = useCallback(() => {}, []);
385385
386386
useEffect(() => {
387387
// Known to be static
@@ -662,30 +662,6 @@ const tests = {
662662
}
663663
`,
664664
},
665-
{
666-
code: `
667-
function MyComponent(props) {
668-
function handleNext1() {
669-
console.log('hello');
670-
}
671-
const handleNext2 = () => {
672-
console.log('hello');
673-
};
674-
let handleNext3 = function() {
675-
console.log('hello');
676-
};
677-
useEffect(() => {
678-
return Store.subscribe(handleNext1);
679-
}, [handleNext1]);
680-
useLayoutEffect(() => {
681-
return Store.subscribe(handleNext2);
682-
}, [handleNext2]);
683-
useMemo(() => {
684-
return Store.subscribe(handleNext3);
685-
}, [handleNext3]);
686-
}
687-
`,
688-
},
689665
{
690666
// Declaring handleNext is optional because
691667
// it doesn't use anything in the function scope.
@@ -3313,6 +3289,139 @@ const tests = {
33133289
'Either include it or remove the dependency array.',
33143290
],
33153291
},
3292+
{
3293+
// Even if the function only references static values,
3294+
// once you specify it in deps, it will invalidate them.
3295+
code: `
3296+
function MyComponent(props) {
3297+
let [, setState] = useState();
3298+
3299+
function handleNext(value) {
3300+
setState(value);
3301+
}
3302+
3303+
useEffect(() => {
3304+
return Store.subscribe(handleNext);
3305+
}, [handleNext]);
3306+
}
3307+
`,
3308+
output: `
3309+
function MyComponent(props) {
3310+
let [, setState] = useState();
3311+
3312+
const handleNext = useCallback(function (value) {
3313+
setState(value);
3314+
}, []);
3315+
3316+
useEffect(() => {
3317+
return Store.subscribe(handleNext);
3318+
}, [handleNext]);
3319+
}
3320+
`,
3321+
errors: [
3322+
`The 'handleNext' function is used in a React Hook dependencies array but ` +
3323+
`its identity changes on every render. As a result, the dependency array serves no ` +
3324+
`purpose. To fix this, try wrapping 'handleNext' into a useCallback() Hook.`,
3325+
],
3326+
},
3327+
{
3328+
code: `
3329+
function MyComponent(props) {
3330+
function handleNext1() {
3331+
console.log('hello');
3332+
}
3333+
const handleNext2 = () => {
3334+
console.log('hello');
3335+
};
3336+
let handleNext3 = function() {
3337+
console.log('hello');
3338+
};
3339+
useEffect(() => {
3340+
return Store.subscribe(handleNext1);
3341+
}, [handleNext1]);
3342+
useLayoutEffect(() => {
3343+
return Store.subscribe(handleNext2);
3344+
}, [handleNext2]);
3345+
useMemo(() => {
3346+
return Store.subscribe(handleNext3);
3347+
}, [handleNext3]);
3348+
}
3349+
`,
3350+
output: `
3351+
function MyComponent(props) {
3352+
const handleNext1 = useCallback(function () {
3353+
console.log('hello');
3354+
}, []);
3355+
const handleNext2 = useCallback(() => {
3356+
console.log('hello');
3357+
}, []);
3358+
let handleNext3 = useCallback(function() {
3359+
console.log('hello');
3360+
}, []);
3361+
useEffect(() => {
3362+
return Store.subscribe(handleNext1);
3363+
}, [handleNext1]);
3364+
useLayoutEffect(() => {
3365+
return Store.subscribe(handleNext2);
3366+
}, [handleNext2]);
3367+
useMemo(() => {
3368+
return Store.subscribe(handleNext3);
3369+
}, [handleNext3]);
3370+
}
3371+
`,
3372+
errors: [
3373+
`The 'handleNext1' function is used in a React Hook dependencies array but ` +
3374+
`its identity changes on every render. As a result, the dependency array serves no ` +
3375+
`purpose. To fix this, try wrapping 'handleNext1' into a useCallback() Hook.`,
3376+
`The 'handleNext2' function is used in a React Hook dependencies array but ` +
3377+
`its identity changes on every render. As a result, the dependency array serves no ` +
3378+
`purpose. To fix this, try wrapping 'handleNext2' into a useCallback() Hook.`,
3379+
`The 'handleNext3' function is used in a React Hook dependencies array but ` +
3380+
`its identity changes on every render. As a result, the dependency array serves no ` +
3381+
`purpose. To fix this, try wrapping 'handleNext3' into a useCallback() Hook.`,
3382+
],
3383+
},
3384+
{
3385+
code: `
3386+
function MyComponent(props) {
3387+
let [, setState] = useState();
3388+
let taint = props.foo;
3389+
3390+
function handleNext(value) {
3391+
let value2 = value * taint;
3392+
setState(value2);
3393+
console.log('hello');
3394+
}
3395+
3396+
useEffect(() => {
3397+
return Store.subscribe(handleNext);
3398+
}, [handleNext]);
3399+
}
3400+
`,
3401+
// TODO: can we include autofix for useCallback right away
3402+
// instead of emitting an empty array by default?
3403+
output: `
3404+
function MyComponent(props) {
3405+
let [, setState] = useState();
3406+
let taint = props.foo;
3407+
3408+
const handleNext = useCallback(function (value) {
3409+
let value2 = value * taint;
3410+
setState(value2);
3411+
console.log('hello');
3412+
}, []);
3413+
3414+
useEffect(() => {
3415+
return Store.subscribe(handleNext);
3416+
}, [handleNext]);
3417+
}
3418+
`,
3419+
errors: [
3420+
`The 'handleNext' function is used in a React Hook dependencies array but ` +
3421+
`its identity changes on every render. As a result, the dependency array serves no ` +
3422+
`purpose. To fix this, try wrapping 'handleNext' into a useCallback() Hook.`,
3423+
],
3424+
},
33163425
{
33173426
code: `
33183427
function Counter() {

‎packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,44 @@ export default {
211211
return false;
212212
}
213213

214+
// TODO: optimize this and remove duplicate code.
215+
function getFnScope(resolved) {
216+
if (!Array.isArray(resolved.defs)) {
217+
return null;
218+
}
219+
const def = resolved.defs[0];
220+
if (def == null) {
221+
return null;
222+
}
223+
if (def.node == null || def.node.id == null) {
224+
return null;
225+
}
226+
// Search the direct component subscopes for
227+
// top-level function definitions matching this reference.
228+
const fnNode = def.node;
229+
let childScopes = componentScope.childScopes;
230+
let fnScope = null;
231+
let i;
232+
for (i = 0; i < childScopes.length; i++) {
233+
let childScope = childScopes[i];
234+
let childScopeBlock = childScope.block;
235+
if (
236+
// function handleChange() {}
237+
(fnNode.type === 'FunctionDeclaration' &&
238+
childScopeBlock === fnNode) ||
239+
// const handleChange = () => {}
240+
// const handleChange = function() {}
241+
(fnNode.type === 'VariableDeclarator' &&
242+
childScopeBlock.parent === fnNode)
243+
) {
244+
// Found it!
245+
fnScope = childScope;
246+
break;
247+
}
248+
}
249+
return fnScope;
250+
}
251+
214252
// Some are just functions that don't reference anything dynamic.
215253
function isFunctionWithoutCapturedValues(resolved) {
216254
if (!Array.isArray(resolved.defs)) {
@@ -533,6 +571,54 @@ export default {
533571
return;
534572
}
535573

574+
// TODO: optimize this
575+
function scanForDeclaredBareFunctions() {
576+
const bareFunctions = declaredDependencies
577+
.map(({node, key}) => {
578+
const fn = componentScope.set.get(key);
579+
if (fn == null) {
580+
return null;
581+
}
582+
if (getFnScope(fn) == null) {
583+
return null;
584+
}
585+
return fn.defs[0];
586+
})
587+
.filter(Boolean);
588+
589+
bareFunctions.forEach(fn => {
590+
context.report({
591+
node: fn.node,
592+
message:
593+
`The '${
594+
fn.name.name
595+
}' function is used in a React Hook dependencies array but ` +
596+
`its identity changes on every render. As a result, the dependency array serves no ` +
597+
`purpose. To fix this, try wrapping '${
598+
fn.name.name
599+
}' into a useCallback() Hook.`,
600+
fix(fixer) {
601+
// TODO: import React? convert to arrow?
602+
if (fn.type === 'FunctionName') {
603+
return [
604+
fixer.insertTextBefore(
605+
fn.name.parent,
606+
'const ' + fn.name.name + ' = useCallback(',
607+
),
608+
fixer.replaceText(fn.name, ''),
609+
fixer.insertTextAfter(fn.name.parent, ', []);'),
610+
];
611+
} else if (fn.type === 'Variable') {
612+
return [
613+
fixer.insertTextBefore(fn.node.init, 'useCallback('),
614+
fixer.insertTextAfter(fn.node.init, ', [])'),
615+
];
616+
}
617+
},
618+
});
619+
});
620+
}
621+
536622
let {
537623
suggestedDependencies,
538624
unnecessaryDependencies,
@@ -551,6 +637,9 @@ export default {
551637
missingDependencies.size +
552638
unnecessaryDependencies.size;
553639
if (problemCount === 0) {
640+
// If nothing else to report, check if some callbacks
641+
// are bare and would invalidate on every render.
642+
scanForDeclaredBareFunctions();
554643
return;
555644
}
556645

0 commit comments

Comments
 (0)