Skip to content

Commit

Permalink
[ESLint] Check useEvent references instead (#25319)
Browse files Browse the repository at this point in the history
* [ESLint] Check useEvent references instead

Previously the useEvent check in RulesOfHooks would collect all
definitions of useEvent functions at the top level, record them as
violations, then clear those violations if the useEvent function was
later called or referened inside of an effect or another event.

The flaw with this approach was in the special case where useEvent
functions could be passed by reference inside of effects or events. The
violation would be cleared here (since it was called at least once)
and subsequent usages of the useEvent function would not be properly
checked.

This PR changes it so we check all identifiers that resolve to a
useEvent function, and if they are not in an effect or event must be
called or a lint error is emitted.

Co-authored-by: Dan Abramov <[email protected]>

* Add comment

Co-authored-by: Dan Abramov <[email protected]>
  • Loading branch information
poteto and gaearon authored Sep 24, 2022
1 parent c89a836 commit cb5084d
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 59 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1090,7 +1090,7 @@ if (__EXPERIMENTAL__) {
return <Child onClick={() => onClick()} />
}
`,
errors: [{...useEventError('onClick'), line: 4}],
errors: [{...useEventError('onClick'), line: 7}],
},
{
code: `
Expand All @@ -1110,13 +1110,26 @@ if (__EXPERIMENTAL__) {
const onClick = useEvent(() => {
showNotification(theme);
});
let foo;
useEffect(() => {
foo = onClick;
});
let foo = onClick;
return <Bar onClick={foo} />
}
`,
errors: [{...useEventError('onClick'), line: 7}],
},
{
code: `
// Should error because it's being passed down to JSX, although it's been referenced once
// in an effect
function MyComponent({ theme }) {
const onClick = useEvent(() => {
showNotification(them);
});
useEffect(() => {
setTimeout(onClick, 100);
});
return <Child onClick={onClick} />
}
`,
errors: [useEventError('onClick')],
},
];
Expand Down
90 changes: 36 additions & 54 deletions packages/eslint-plugin-react-hooks/src/RulesOfHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,42 +120,30 @@ export default {
let lastEffect = null;
const codePathReactHooksMapStack = [];
const codePathSegmentStack = [];
const useEventViolations = new Set();

// For a given AST node, iterate through the top level statements and add all useEvent
// definitions. We can do this in non-Program nodes because we can rely on the assumption that
// useEvent functions can only be declared within a component or hook at its top level.
function addAllUseEventViolations(node) {
if (node.body.type !== 'BlockStatement') return;
for (const statement of node.body.body) {
if (statement.type !== 'VariableDeclaration') continue;
for (const declaration of statement.declarations) {
if (
declaration.type === 'VariableDeclarator' &&
declaration.init &&
declaration.init.type === 'CallExpression' &&
declaration.init.callee &&
isUseEventIdentifier(declaration.init.callee)
) {
useEventViolations.add(declaration.id);
const useEventFunctions = new WeakSet();

// For a given scope, iterate through the references and add all useEvent definitions. We can
// do this in non-Program nodes because we can rely on the assumption that useEvent functions
// can only be declared within a component or hook at its top level.
function recordAllUseEventFunctions(scope) {
for (const reference of scope.references) {
const parent = reference.identifier.parent;
if (
parent.type === 'VariableDeclarator' &&
parent.init &&
parent.init.type === 'CallExpression' &&
parent.init.callee &&
isUseEventIdentifier(parent.init.callee)
) {
for (const ref of reference.resolved.references) {
if (ref !== reference) {
useEventFunctions.add(ref.identifier);
}
}
}
}
}

// Resolve a useEvent violation, ie the useEvent created function was called.
function resolveUseEventViolation(scope, ident) {
if (scope.references == null || useEventViolations.size === 0) return;
for (const ref of scope.references) {
if (ref.resolved == null) continue;
const [useEventFunctionIdentifier] = ref.resolved.identifiers;
if (ident.name === useEventFunctionIdentifier.name) {
useEventViolations.delete(useEventFunctionIdentifier);
break;
}
}
}

return {
// Maintain code segment path stack as we traverse.
onCodePathSegmentStart: segment => codePathSegmentStack.push(segment),
Expand Down Expand Up @@ -567,11 +555,6 @@ export default {
reactHooks.push(node.callee);
}

const scope = context.getScope();
// useEvent: Resolve a function created with useEvent that is invoked locally at least once.
// OK - onClick();
resolveUseEventViolation(scope, node.callee);

// useEvent: useEvent functions can be passed by reference within useEffect as well as in
// another useEvent
if (
Expand All @@ -587,9 +570,21 @@ export default {
},

Identifier(node) {
// OK - useEffect(() => { setInterval(onClick, ...) }, []);
if (lastEffect != null && node.parent.type === 'CallExpression') {
resolveUseEventViolation(context.getScope(), node);
// This identifier resolves to a useEvent function, but isn't being referenced in an
// effect or another event function. It isn't being called either.
if (
lastEffect == null &&
useEventFunctions.has(node) &&
node.parent.type !== 'CallExpression'
) {
context.report({
node,
message:
`\`${context.getSource(
node,
)}\` is a function created with React Hook "useEvent", and can only be called from ` +
'the same component. They cannot be assigned to variables or passed down.',
});
}
},

Expand All @@ -602,27 +597,14 @@ export default {
FunctionDeclaration(node) {
// function MyComponent() { const onClick = useEvent(...) }
if (isInsideComponentOrHook(node)) {
addAllUseEventViolations(node);
recordAllUseEventFunctions(context.getScope());
}
},

ArrowFunctionExpression(node) {
// const MyComponent = () => { const onClick = useEvent(...) }
if (isInsideComponentOrHook(node)) {
addAllUseEventViolations(node);
}
},

'Program:exit'(_node) {
for (const node of useEventViolations.values()) {
context.report({
node,
message:
`\`${context.getSource(
node,
)}\` is a function created with React Hook "useEvent", and can only be called from ` +
'the same component. They cannot be assigned to variables or passed down.',
});
recordAllUseEventFunctions(context.getScope());
}
},
};
Expand Down

0 comments on commit cb5084d

Please sign in to comment.