[ESLint] Check useEvent references instead (#25319)
* [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 <dan.abramov@gmail.com> * Add comment Co-authored-by: Dan Abramov <dan.abramov@gmail.com>
Lauren Tan committed
Sep 23, 2022 at 17:34 UTC
cb5084d1c4dc0693e7dcd8af7e921a0aa771914b
2 files changed
+54
-59
packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js
+18
-5
@@ -1090,7 +1090,7 @@ if (__EXPERIMENTAL__) {
1090
return <Child onClick={() => onClick()} />
1091
}
1092
`,
1093
- errors: [{...useEventError('onClick'), line: 4}],
1093
+ errors: [{...useEventError('onClick'), line: 7}],
1094
},
1095
{
1096
code: `
@@ -1110,13 +1110,26 @@ if (__EXPERIMENTAL__) {
1110
const onClick = useEvent(() => {
1111
showNotification(theme);
1112
});
1113
- let foo;
1114
- useEffect(() => {
1115
- foo = onClick;
1116
- });
1113
+ let foo = onClick;
1114
return <Bar onClick={foo} />
1115
}
1116
`,
1117
+ errors: [{...useEventError('onClick'), line: 7}],
1118
+ },
1119
+ {
1120
+ code: `
1121
+ // Should error because it's being passed down to JSX, although it's been referenced once
1122
+ // in an effect
1123
+ function MyComponent({ theme }) {
1124
+ const onClick = useEvent(() => {
1125
+ showNotification(them);
1126
+ });
1127
+ useEffect(() => {
1128
+ setTimeout(onClick, 100);
1129
+ });
1130
+ return <Child onClick={onClick} />
1131
+ }
1132
+ `,
1133
errors: [useEventError('onClick')],
1134
},
1135
];
packages/eslint-plugin-react-hooks/src/RulesOfHooks.js
+36
-54
@@ -120,42 +120,30 @@ export default {
120
let lastEffect = null;
121
const codePathReactHooksMapStack = [];
122
const codePathSegmentStack = [];
123
- const useEventViolations = new Set();
124
-
125
- // For a given AST node, iterate through the top level statements and add all useEvent
126
- // definitions. We can do this in non-Program nodes because we can rely on the assumption that
127
- // useEvent functions can only be declared within a component or hook at its top level.
128
- function addAllUseEventViolations(node) {
129
- if (node.body.type !== 'BlockStatement') return;
130
- for (const statement of node.body.body) {
131
- if (statement.type !== 'VariableDeclaration') continue;
132
- for (const declaration of statement.declarations) {
133
- if (
134
- declaration.type === 'VariableDeclarator' &&
135
- declaration.init &&
136
- declaration.init.type === 'CallExpression' &&
137
- declaration.init.callee &&
138
- isUseEventIdentifier(declaration.init.callee)
139
- ) {
140
- useEventViolations.add(declaration.id);
123
+ const useEventFunctions = new WeakSet();
124
+
125
+ // For a given scope, iterate through the references and add all useEvent definitions. We can
126
+ // do this in non-Program nodes because we can rely on the assumption that useEvent functions
127
+ // can only be declared within a component or hook at its top level.
128
+ function recordAllUseEventFunctions(scope) {
129
+ for (const reference of scope.references) {
130
+ const parent = reference.identifier.parent;
131
+ if (
132
+ parent.type === 'VariableDeclarator' &&
133
+ parent.init &&
134
+ parent.init.type === 'CallExpression' &&
135
+ parent.init.callee &&
136
+ isUseEventIdentifier(parent.init.callee)
137
+ ) {
138
+ for (const ref of reference.resolved.references) {
139
+ if (ref !== reference) {
140
+ useEventFunctions.add(ref.identifier);
141
+ }
142
}
143
}
144
}
145
}
146
146
- // Resolve a useEvent violation, ie the useEvent created function was called.
147
- function resolveUseEventViolation(scope, ident) {
148
- if (scope.references == null || useEventViolations.size === 0) return;
149
- for (const ref of scope.references) {
150
- if (ref.resolved == null) continue;
151
- const [useEventFunctionIdentifier] = ref.resolved.identifiers;
152
- if (ident.name === useEventFunctionIdentifier.name) {
153
- useEventViolations.delete(useEventFunctionIdentifier);
154
- break;
155
- }
156
- }
157
- }
158
-
147
return {
148
// Maintain code segment path stack as we traverse.
149
onCodePathSegmentStart: segment => codePathSegmentStack.push(segment),
@@ -567,11 +555,6 @@ export default {
555
reactHooks.push(node.callee);
556
}
557
570
- const scope = context.getScope();
571
- // useEvent: Resolve a function created with useEvent that is invoked locally at least once.
572
- // OK - onClick();
573
- resolveUseEventViolation(scope, node.callee);
574
-
558
// useEvent: useEvent functions can be passed by reference within useEffect as well as in
559
// another useEvent
560
if (
@@ -587,9 +570,21 @@ export default {
570
},
571
572
Identifier(node) {
590
- // OK - useEffect(() => { setInterval(onClick, ...) }, []);
591
- if (lastEffect != null && node.parent.type === 'CallExpression') {
592
- resolveUseEventViolation(context.getScope(), node);
573
+ // This identifier resolves to a useEvent function, but isn't being referenced in an
574
+ // effect or another event function. It isn't being called either.
575
+ if (
576
+ lastEffect == null &&
577
+ useEventFunctions.has(node) &&
578
+ node.parent.type !== 'CallExpression'
579
+ ) {
580
+ context.report({
581
+ node,
582
+ message:
583
+ `\`${context.getSource(
584
+ node,
585
+ )}\` is a function created with React Hook "useEvent", and can only be called from ` +
586
+ 'the same component. They cannot be assigned to variables or passed down.',
587
+ });
588
}
589
},
590
@@ -602,27 +597,14 @@ export default {
597
FunctionDeclaration(node) {
598
// function MyComponent() { const onClick = useEvent(...) }
599
if (isInsideComponentOrHook(node)) {
605
- addAllUseEventViolations(node);
600
+ recordAllUseEventFunctions(context.getScope());
601
}
602
},
603
604
ArrowFunctionExpression(node) {
605
// const MyComponent = () => { const onClick = useEvent(...) }
606
if (isInsideComponentOrHook(node)) {
612
- addAllUseEventViolations(node);
613
- }
614
- },
615
-
616
- 'Program:exit'(_node) {
617
- for (const node of useEventViolations.values()) {
618
- context.report({
619
- node,
620
- message:
621
- `\`${context.getSource(
622
- node,
623
- )}\` is a function created with React Hook "useEvent", and can only be called from ` +
624
- 'the same component. They cannot be assigned to variables or passed down.',
625
- });
607
+ recordAllUseEventFunctions(context.getScope());
608
}
609
},
610
};