Skip to content

Commit

Permalink
[useEvent] Lint for presence of useEvent functions in dependency lists
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
poteto committed Oct 19, 2022
1 parent 9872928 commit 67f600d
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}, []);
}
`,
},
],
},
],
},
];
}
Expand Down
27 changes: 26 additions & 1 deletion packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit 67f600d

Please sign in to comment.