Flag using state as event handler

NickvanDyke · NickvanDyke · commit 3edcf3bf4f99 · 2025-06-29T10:59:55.000-06:00
#17
diff --git a/src/messages.js b/src/messages.js
@@ -5,11 +5,7 @@ export const messageIds = {
   avoidChainingState: "avoidChainingState",
   avoidParentChildCoupling: "avoidParentChildCoupling",
   avoidResettingStateFromProps: "avoidResettingStateFromProps",
-  // TODO: This would be nice, but I'm not sure it can be done accurately
-  // Maybe we can accurately warn about this when the state being reacted to is one of our own `useState`s?
-  // Because if we have a setter then we have a callback.
-  // But, I think that would also warn about valid uses that synchronize internal state to external state.
-  // avoidEventHandler: "avoidEventHandler",
+  avoidEventHandler: "avoidEventHandler",
   // TODO: Possible to detect when `useSyncExternalStore` should be preferred?
 };
 
@@ -26,6 +22,6 @@ export const messages = {
     "Avoid coupling parent behavior or state to a child component. Instead, lift shared logic or state up to the parent.",
   [messageIds.avoidResettingStateFromProps]:
     'Avoid resetting state from props. If "{{prop}}" is a key, pass it as `key` instead so React will reset the component.',
-  // [messages.avoidEventHandler]:
-  //   "Avoid using state as an event handler. Instead, call the event handler directly.",
+  [messageIds.avoidEventHandler]:
+    "Avoid using state as an event handler. Instead, call the event handler directly.",
 };
diff --git a/src/rule.js b/src/rule.js
@@ -1,5 +1,9 @@
 import { messageIds, messages } from "./messages.js";
-import { getCallExpr, getDownstreamRefs } from "./util/ast.js";
+import {
+  findDownstreamIfs,
+  getCallExpr,
+  getDownstreamRefs,
+} from "./util/ast.js";
 import {
   findPropUsedToResetAllState,
   isUseEffect,
@@ -14,6 +18,7 @@ import {
   isProp,
   isHOCProp,
   countStateSetterCalls,
+  isFnRef,
 } from "./util/react.js";
 import { arraysEqual } from "./util/javascript.js";
 
@@ -67,14 +72,38 @@ export const rule = {
         return;
       }
 
-      effectFnRefs
-        .filter(
-          (ref) =>
-            isStateSetter(context, ref) ||
-            (isPropCallback(context, ref) &&
-              // Don't analyze HOC prop callbacks -- we don't have control over them to lift state or logic
-              !isHOCProp(ref.resolved)),
+      const isAllDepsInternal = depsRefs
+        .flatMap((ref) => getUpstreamReactVariables(context, ref.identifier))
+        .notEmptyEvery(
+          (variable) =>
+            isState(variable) || (isProp(variable) && !isHOCProp(variable)),
+        );
+
+      findDownstreamIfs(context, node)
+        // An event-handling effect (invalid) and a synchronizing effect (valid)
+        // look quite similar. But the latter should act on *all* possible states,
+        // whereas the former waits for a specific state (from the event).
+        // Technically synchronizing effects can be inlined too.
+        // But an effect is arguably more readable (for once), and recommended by the React docs.
+        .filter((ifNode) => !ifNode.alternate)
+        .filter((ifNode) =>
+          getDownstreamRefs(context, ifNode.test)
+            .flatMap((ref) =>
+              getUpstreamReactVariables(context, ref.identifier),
+            )
+            // TODO: Include non-HOC props, but probably with a different message -
+            // the state would need to be lifted to inline the effect logic
+            .notEmptyEvery((variable) => isState(variable)),
         )
+        .forEach((ifNode) => {
+          context.report({
+            node: ifNode.test,
+            messageId: messageIds.avoidEventHandler,
+          });
+        });
+
+      effectFnRefs
+        .filter(isFnRef)
         // Non-direct calls are likely inside a callback passed to an external system like `window.addEventListener`,
         // or a Promise chain that (probably) retrieves external data.
         // Note we'll still analyze derived setters because isStateSetter considers that.
@@ -135,15 +164,6 @@ export const rule = {
                   ),
                 ),
               );
-            const isAllDepsInternal = depsRefs
-              .flatMap((ref) =>
-                getUpstreamReactVariables(context, ref.identifier),
-              )
-              .notEmptyEvery(
-                (variable) =>
-                  isState(variable) ||
-                  (isProp(variable) && !isHOCProp(variable)),
-              );
 
             if (
               isAllArgsInternal ||
@@ -170,20 +190,15 @@ export const rule = {
                 messageId: messageIds.avoidChainingState,
               });
             }
-          } else if (isPropCallback(context, ref)) {
+          } else if (
+            isPropCallback(context, ref) &&
+            // Don't analyze HOC prop callbacks -- we don't have control over them to lift state or logic
+            !isHOCProp(ref.resolved)
+          ) {
             // I'm pretty sure we can flag this regardless of the arguments, including none...
-            //
             // Because we are either:
             // 1. Passing live state updates to the parent
             // 2. Using state as an event handler to pass final state to the parent
-            //
-            // Both are bad. However I'm not yet sure how we could differentiate #2 to give a better warning.
-            //
-            // TODO: Can we thus safely assume that state is used as an event handler when the ref is a prop?
-            // Normally we can't warn about that because we don't know what the event handler does externally.
-            // But when it's a prop, it's internal.
-            // I guess it could still be valid when the dep is external state? Or in that case,
-            // the issue is the state should be lifted to the parent?
             context.report({
               node: callExpr,
               messageId: messageIds.avoidParentChildCoupling,
diff --git a/src/util/ast.js b/src/util/ast.js
@@ -31,6 +31,16 @@ const getDownstreamIdentifiers = (context, rootNode) => {
   return identifiers;
 };
 
+export const findDownstreamIfs = (context, node) => {
+  const ifs = [];
+  traverse(context, node, (n) => {
+    if (n.type === "IfStatement") {
+      ifs.push(n);
+    }
+  });
+  return ifs;
+};
+
 export const getUpstreamVariables = (
   context,
   node,
diff --git a/test/chaining-state.test.js b/test/chaining-state.test.js
@@ -48,6 +48,9 @@ new MyRuleTester().run("/chaining-state", {
         }
       `,
       errors: [
+        {
+          messageId: messageIds.avoidEventHandler,
+        },
         {
           messageId: messageIds.avoidChainingState,
         },
diff --git a/test/deriving-state.test.js b/test/deriving-state.test.js
@@ -75,9 +75,6 @@ new MyRuleTester().run("/deriving-state", {
     },
     {
       name: "Sync external state",
-      // Technically we could trigger the network call in `input.onChange`,
-      // but the use of an effect to sync state is arguably more readable and a valid use.
-      // Especially when we already store the input's controlled state.
       code: js`
         function Search() {
           const [query, setQuery] = useState();
diff --git a/test/real-world.test.js b/test/real-world.test.js
@@ -178,40 +178,6 @@ new MyRuleTester().run("/real-world", {
         }
       `,
     },
-    {
-      // https://github.com/NickvanDyke/eslint-plugin-react-you-might-not-need-an-effect/issues/7
-      name: "Klarna",
-      code: js`
-        function Klarna({ klarnaAppId }) {
-          const [countryCode] = useState(qs.parse('countryCode=meow'));
-          const [result, setResult] = useState();
-          const klarnaEnabled = useSelector('idk') && shouldKlarnaBeEnabled(countryCode);
-          const currentLocale = getCurrentLocale(useGetCurrentLanguage());
-
-          const loadSignInWithKlarna = (klarnaAppId, klarnaEnvironment, countryCode, currentLocale) => {
-            const klarnaResult = doSomething();
-            setResult(klarnaResult);
-          };
-
-          useEffect(() => {
-            if (klarnaEnabled) {
-              return loadSignInWithKlarna(
-                  klarnaAppId,
-                  klarnaEnvironment,
-                  countryCode?.toUpperCase(),
-                  currentLocale,
-              );
-            }
-          }, [
-            countryCode,
-            klarnaAppId,
-            klarnaEnabled,
-            klarnaEnvironment,
-            currentLocale,
-          ]);
-        }
-      `,
-    },
     {
       // https://github.com/NickvanDyke/eslint-plugin-react-you-might-not-need-an-effect/issues/10
       name: "navigation.setOptions",
@@ -335,5 +301,44 @@ new MyRuleTester().run("/real-world", {
         },
       ],
     },
+    {
+      // https://github.com/NickvanDyke/eslint-plugin-react-you-might-not-need-an-effect/issues/7
+      name: "Klarna",
+      code: js`
+        function Klarna({ klarnaAppId }) {
+          const [countryCode] = useState(qs.parse('countryCode=meow'));
+          const [result, setResult] = useState();
+          const klarnaEnabled = useSelector('idk') && shouldKlarnaBeEnabled(countryCode);
+          const currentLocale = getCurrentLocale(useGetCurrentLanguage());
+
+          const loadSignInWithKlarna = (klarnaAppId, klarnaEnvironment, countryCode, currentLocale) => {
+            const klarnaResult = doSomething();
+            setResult(klarnaResult);
+          };
+
+          useEffect(() => {
+            if (klarnaEnabled) {
+              return loadSignInWithKlarna(
+                  klarnaAppId,
+                  klarnaEnvironment,
+                  countryCode?.toUpperCase(),
+                  currentLocale,
+              );
+            }
+          }, [
+            countryCode,
+            klarnaAppId,
+            klarnaEnabled,
+            klarnaEnvironment,
+            currentLocale,
+          ]);
+        }
+      `,
+      errors: [
+        {
+          messageId: messageIds.avoidEventHandler,
+        },
+      ],
+    },
   ],
 });
diff --git a/test/using-state-as-event-handler.test.js b/test/using-state-as-event-handler.test.js
@@ -2,19 +2,108 @@ import { MyRuleTester, js } from "./rule-tester.js";
 import { messageIds } from "../src/messages.js";
 
 new MyRuleTester().run("/using-state-as-event-handler", {
+  valid: [
+    {
+      name: "Sychronizing with external system",
+      code: js`
+        function Search() {
+          const [query, setQuery] = useState();
+          const [results, setResults] = useState();
+
+          useEffect(() => {
+            fetch('/search?query=' + query).then((data) => {
+              setResults(data);
+            });
+          }, [query]);
+
+          return (
+            <div>
+              <input
+                name="query"
+                type="text"
+                value={query}
+                onChange={(e) => setQuery(e.target.value)}
+              />
+              <ul>
+                {results.map((result) => (
+                  <li key={result.id}>{result.title}</li>
+                ))}
+              </ul>
+            </div>
+          )
+        }
+      `,
+    },
+    {
+      name: "If test includes non-state",
+      code: js`
+        function Form() {
+          const [name, setName] = useState();
+          const [dataToSubmit, setDataToSubmit] = useState();
+
+          useEffect(() => {
+            if (dataToSubmit && Date.now() % 2 === 0) {
+              submitData(dataToSubmit);
+            }
+          }, [dataToSubmit]);
+
+          return (
+            <div>
+              <input
+                name="name"
+                type="text"
+                onChange={(e) => setName(e.target.value)}
+              />
+              <button onClick={() => setDataToSubmit({ name })}>Submit</button>
+            </div>
+          )
+        }
+      `,
+    },
+  ],
   invalid: [
     {
-      //  TODO: How to detect this though? Not sure it's discernable from legit synchronization effects.
-      //  Maybe when the setter is only called in this one place? Meaning we could instead inline the effect.
       name: "Using state to handle an event",
-      todo: true,
       code: js`
         function Form() {
           const [name, setName] = useState();
           const [dataToSubmit, setDataToSubmit] = useState();
 
           useEffect(() => {
-            submitData(dataToSubmit);
+            if (dataToSubmit) {
+              submitData(dataToSubmit);
+            }
+          }, [dataToSubmit]);
+
+          return (
+            <div>
+              <input
+                name="name"
+                type="text"
+                onChange={(e) => setName(e.target.value)}
+              />
+              <button onClick={() => setDataToSubmit({ name })}>Submit</button>
+            </div>
+          )
+        }
+      `,
+      errors: [
+        {
+          messageId: messageIds.avoidEventHandler,
+        },
+      ],
+    },
+    {
+      name: "More complex if test",
+      code: js`
+        function Form() {
+          const [name, setName] = useState();
+          const [dataToSubmit, setDataToSubmit] = useState();
+
+          useEffect(() => {
+            if (dataToSubmit.name && dataToSubmit.name.length > 0) {
+              submitData(dataToSubmit);
+            }
           }, [dataToSubmit]);
 
           return (

Original file line number	Diff line number	Diff line change
`@@ -48,6 +48,9 @@ new MyRuleTester().run("/chaining-state", {`
`48`	`48`	`}`
`49`	`49`	`,
`50`	`50`	`errors: [`
	`51`	`+ {`
	`52`	`+ messageId: messageIds.avoidEventHandler,`
	`53`	`+ },`
`51`	`54`	`{`
`52`	`55`	`messageId: messageIds.avoidChainingState,`
`53`	`56`	`},`