-
Notifications
You must be signed in to change notification settings - Fork 47k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update RulesOfHooks with useEvent rules #25285
Changes from 1 commit
ec0e8e8
9804d1a
16e669b
9550edb
76c096c
54765ee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,6 +100,13 @@ function isInsideComponentOrHook(node) { | |
return false; | ||
} | ||
|
||
function isUseEventIdentifier(node) { | ||
return ( | ||
node.type === 'Identifier' && | ||
(node.name === 'useEvent' || node.name === 'experimental_useEvent') | ||
gaearon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
); | ||
} | ||
|
||
export default { | ||
meta: { | ||
type: 'problem', | ||
|
@@ -110,8 +117,45 @@ export default { | |
}, | ||
}, | ||
create(context) { | ||
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 ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we think of any case where we'd need to go deeper or recurse? I think the only valid one I can come up with is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think one generalized case where we might need to go deeper is if the event function is reassigned to a variable instead of being initialized in a variable declaration: function Component() {
let onClick;
{
onClick = useEvent(...);
}
(function weird() {
onClick = useEvent(...);
})();
// etc
} But I don't know if this is something we want to support? I may have misunderstood the overall goal of the lint rules: do we aim to be correct in all cases, or only in the "idiomatic" case with some small exceptions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea this is not worth supporting. I think only idiomatic usage is relevant, and I guess people don't use raw blocks much either. |
||
declaration.type === 'VariableDeclarator' && | ||
declaration.init && | ||
declaration.init.type === 'CallExpression' && | ||
declaration.init.callee && | ||
isUseEventIdentifier(declaration.init.callee) | ||
) { | ||
useEventViolations.add(declaration.id); | ||
} | ||
} | ||
} | ||
} | ||
|
||
// 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), | ||
|
@@ -522,6 +566,62 @@ 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 | ||
if ( | ||
node.callee.type === 'Identifier' && | ||
node.callee.name === 'useEffect' && | ||
node.arguments.length > 0 | ||
) { | ||
// Denote that we have traversed into a useEffect call, and stash the CallExpr for | ||
// comparison later when we exit | ||
lastEffect = node; | ||
} | ||
}, | ||
|
||
Identifier(node) { | ||
// OK - useEffect(() => { setInterval(onClick, ...) }, []); | ||
if (lastEffect != null && node.parent.type === 'CallExpression') { | ||
resolveUseEventViolation(context.getScope(), node); | ||
} | ||
}, | ||
|
||
'CallExpression:exit'(node) { | ||
if (node === lastEffect) { | ||
lastEffect = null; | ||
} | ||
}, | ||
|
||
FunctionDeclaration(node) { | ||
// function MyComponent() { const onClick = useEvent(...) } | ||
if (isInsideComponentOrHook(node)) { | ||
addAllUseEventViolations(node); | ||
} | ||
}, | ||
|
||
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.', | ||
}); | ||
} | ||
}, | ||
}; | ||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: shall we just call this "event functions" like the doc (https://beta.reactjs.org/learn/separating-events-from-effects)? It's conciser and feels first classy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what exactly we want to call it (after all the recent bikeshedding) since
useEvent
might be renamed, hence why I've been avoiding giving it a name. Maybe @acdlite can weigh in on what to call it in the meantime?