Only subtraverse within useEffect CallExprs

facebook · Sep 21, 2022 · f85de77 · f85de77
1 parent 1b0db92
commit f85de77
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 7 deletions.
diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js
@@ -481,6 +481,18 @@ const tests = {
         return <Child onClick={() => onClick()}></Child>;
       };
     `,
+    `
+      function MyComponent({ theme }) {
+        const notificationService = useNotifications();
+        const showNotification = useEvent((text) => {
+          notificationService.notify(theme, text);
+        });
+        const onClick = useEvent((text) => {
+          showNotification(text);
+        });
+        return <Child onClick={(text) => onClick(text)} />
+      }
+    `,
   ],
   invalid: [
     {
@@ -1115,6 +1127,22 @@ const tests = {
       `,
       errors: [useEventError('onClick')],
     },
+    {
+      code: `
+        // Invalid because onClick is being aliased to foo but not invoked
+        function MyComponent({ theme }) {
+          const onClick = useEvent(() => {
+            showNotification(theme);
+          });
+          let foo;
+          useEffect(() => {
+            foo = onClick;
+          });
+          return <Bar onClick={foo} />
+        }
+      `,
+      errors: [useEventError('onClick')],
+    },
   ],
 };
 

diff --git a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js
@@ -584,17 +584,24 @@ export default {
         const scope = context.getScope();
         // useEvent: Resolve a function created with useEvent that is invoked locally at least once.
         // OK - onClick();
-        if (!isUseEventIdentifier(node.callee)) {
-          resolveUseEventViolation(scope, node.callee);
-        }
+        resolveUseEventViolation(scope, node.callee);
 
-        // useEvent: useEvent functions can be passed by reference within useEffect.
+        // useEvent: useEvent functions can be passed by reference within useEffect
         // OK - useEffect(() => { ...onClick... }, []);
         if (isUseEffectIdentifier(node.callee) && node.arguments.length > 0) {
-          // Subtraverse here so we don't need to visit every Identifier node.
+          // Subtraverse here so we don't need to visit every Identifier node. Conservatively, we
+          // only resolve a useEventViolation if the useEvent function Identifier is consumed by a
+          // CallExpression (eg setInterval(onClick, 100)), with the assumption that something else
+          // will call the function later.
+          //
+          // We may want to revisit this and remove this shorthand entirely at some point.
           traverse(context, node, childNode => {
-            if (childNode.type === 'Identifier') {
-              resolveUseEventViolation(scope, childNode);
+            if (childNode.type === 'CallExpression') {
+              for (const argument of childNode.arguments) {
+                if (argument.type === 'Identifier') {
+                  resolveUseEventViolation(scope, argument);
+                }
+              }
             }
           });
         }