From 67f600df1379b725cc1d3e97b837533de34124a5 Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Wed, 19 Oct 2022 12:55:10 -0400 Subject: [PATCH 1/3] [useEvent] Lint for presence of useEvent functions in dependency lists With #25473, the identity of useEvent's return value is no longer stable across renders. Previously, the ExhaustiveDeps lint rule would only allow the omission of the useEvent function, but you could still add it as a dependency. This PR updates the ExhaustiveDeps rule to explicitly check for the presence of useEvent functions in dependency lists, and emits a warning and suggestion/autofixer for removing the dependency. --- .../ESLintRuleExhaustiveDeps-test.js | 57 ++++++++++++++++--- .../src/ExhaustiveDeps.js | 27 ++++++++- 2 files changed, 74 insertions(+), 10 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index fc343705a6922..c4c379e8b4e63 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -7635,15 +7635,54 @@ if (__EXPERIMENTAL__) { ...tests.valid, { code: normalizeIndent` - function MyComponent({ theme }) { - const onStuff = useEvent(() => { - showNotification(theme); - }); - useEffect(() => { - onStuff(); - }, []); - } - `, + function MyComponent({ theme }) { + const onStuff = useEvent(() => { + showNotification(theme); + }); + useEffect(() => { + onStuff(); + }, []); + } + `, + }, + ]; + + tests.invalid = [ + ...tests.invalid, + { + code: normalizeIndent` + function MyComponent({ theme }) { + const onStuff = useEvent(() => { + showNotification(theme); + }); + useEffect(() => { + onStuff(); + }, [onStuff]); + } + `, + errors: [ + { + message: + '`useEvent` functions always return a new identity for every render. This means that ' + + 'it should not be included in dependency lists, as it would cause the callback to be ' + + 'run on every render. You can safely remove this.', + suggestions: [ + { + desc: 'Remove the dependency `onStuff`', + output: normalizeIndent` + function MyComponent({ theme }) { + const onStuff = useEvent(() => { + showNotification(theme); + }); + useEffect(() => { + onStuff(); + }, []); + } + `, + }, + ], + }, + ], }, ]; } diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 6f99e6ef4369e..fc5f2d0127478 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -74,6 +74,7 @@ export default { const stateVariables = new WeakSet(); const stableKnownValueCache = new WeakMap(); const functionWithoutCapturedValueCache = new WeakMap(); + const useEventVariables = new WeakSet(); function memoizeWithWeakMap(fn, map) { return function(arg) { if (map.has(arg)) { @@ -226,7 +227,12 @@ export default { // useRef() return value is stable. return true; } else if (isUseEventIdentifier(callee) && id.type === 'Identifier') { - // useEvent() return value is stable. + for (const ref of resolved.references) { + if (ref !== id) { + useEventVariables.add(ref.identifier); + } + } + // useEvent() return value is always unstable. return true; } else if (name === 'useState' || name === 'useReducer') { // Only consider second value in initializing tuple stable. @@ -639,6 +645,25 @@ export default { }); return; } + if (useEventVariables.has(declaredDependencyNode)) { + reportProblem({ + node: declaredDependencyNode, + message: + '`useEvent` functions always return a new identity for every render. This means ' + + 'that it should not be included in dependency lists, as it would cause the ' + + 'callback to be run on every render. You can safely remove this.', + suggest: [ + { + desc: `Remove the dependency \`${context.getSource( + declaredDependencyNode, + )}\``, + fix(fixer) { + return fixer.removeRange(declaredDependencyNode.range); + }, + }, + ], + }); + } // Try to normalize the declared dependency. If we can't then an error // will be thrown. We will catch that error and report an error. let declaredDependency; From 29a09d78304e627775b72424a4abeda8a18faae3 Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Wed, 19 Oct 2022 13:08:51 -0400 Subject: [PATCH 2/3] Address feedback on lint message --- .../__tests__/ESLintRuleExhaustiveDeps-test.js | 5 ++--- packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js | 7 ++++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index c4c379e8b4e63..f51384b4db875 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -7663,9 +7663,8 @@ if (__EXPERIMENTAL__) { errors: [ { message: - '`useEvent` functions always return a new identity for every render. This means that ' + - 'it should not be included in dependency lists, as it would cause the callback to be ' + - 'run on every render. You can safely remove this.', + 'Functions wrapped in `useEvent` must not be included in the dependency array. ' + + 'Remove `onStuff` from the list.', suggestions: [ { desc: 'Remove the dependency `onStuff`', diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index fc5f2d0127478..2217200f12a50 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -649,9 +649,10 @@ export default { reportProblem({ node: declaredDependencyNode, message: - '`useEvent` functions always return a new identity for every render. This means ' + - 'that it should not be included in dependency lists, as it would cause the ' + - 'callback to be run on every render. You can safely remove this.', + 'Functions wrapped in `useEvent` must not be included in the dependency array. ' + + `Remove \`${context.getSource( + declaredDependencyNode, + )}\` from the list.`, suggest: [ { desc: `Remove the dependency \`${context.getSource( From d2eed48d724004735ade8e0d5fc6cdf4ca88f21e Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Wed, 19 Oct 2022 14:54:02 -0400 Subject: [PATCH 3/3] s/wrapped in/returned from/ --- .../__tests__/ESLintRuleExhaustiveDeps-test.js | 2 +- packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index f51384b4db875..2de2daa85d656 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -7663,7 +7663,7 @@ if (__EXPERIMENTAL__) { errors: [ { message: - 'Functions wrapped in `useEvent` must not be included in the dependency array. ' + + 'Functions returned from `useEvent` must not be included in the dependency array. ' + 'Remove `onStuff` from the list.', suggestions: [ { diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 2217200f12a50..0885d268367f8 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -649,7 +649,7 @@ export default { reportProblem({ node: declaredDependencyNode, message: - 'Functions wrapped in `useEvent` must not be included in the dependency array. ' + + 'Functions returned from `useEvent` must not be included in the dependency array. ' + `Remove \`${context.getSource( declaredDependencyNode, )}\` from the list.`,