Detect chained state in non-internal effect. Clarify internal vs external.

NickvanDyke · NickvanDyke · commit 6a42f6b5d0ea · 2025-06-05T16:05:48.000-06:00
Update isHOCProp to support React.memo

fix: allow isHOCProp to accept both React.memo and memo

feat: add isReactFunctionalHOC utility function for HOC detection

simplify
diff --git a/src/rule.js b/src/rule.js
@@ -1,18 +1,20 @@
 import { messageIds, messages } from "./messages.js";
-import { getCallExpr } from "./util/ast.js";
+import { getCallExpr, getDownstreamRefs } from "./util/ast.js";
 import {
   isFnRef,
   findPropUsedToResetAllState,
   isUseEffect,
   getUseStateNode,
   getEffectFnRefs,
   getDependenciesRefs,
-  isArgsInternal,
   isStateSetter,
   isPropCallback,
-  isInternal,
   getEffectFn,
   isDirectCall,
+  getUpstreamReactVariables,
+  isState,
+  isProp,
+  isHOCProp,
 } from "./util/react.js";
 
 export const name = "you-might-not-need-an-effect";
@@ -49,7 +51,11 @@ export const rule = {
         .filter((ref) => isFnRef(ref))
         .filter((ref) => isDirectCall(getEffectFn(node), ref))
         .concat(depsRefs)
-        .every((ref) => isInternal(context, ref));
+        .flatMap((ref) => getUpstreamReactVariables(context, ref.identifier))
+        .notEmptyEvery(
+          (variable) =>
+            isState(variable) || (isProp(variable) && !isHOCProp(variable)),
+        );
 
       if (isInternalEffect) {
         context.report({
@@ -80,7 +86,11 @@ export const rule = {
 
       effectFnRefs
         .filter(
-          (ref) => isStateSetter(context, ref) || isPropCallback(context, ref),
+          (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)),
         )
         .filter((ref) => isDirectCall(getEffectFn(node), ref))
         .forEach((ref) => {
@@ -100,7 +110,38 @@ export const rule = {
               });
             }
 
-            if (isArgsInternal(context, callExpr.arguments)) {
+            // TODO: Make more readable
+            const isArgsInternal = callExpr.arguments
+              .flatMap((arg) => getDownstreamRefs(context, arg))
+              .flatMap((ref) =>
+                getUpstreamReactVariables(context, ref.identifier),
+              )
+              .notEmptyEvery(
+                (variable) =>
+                  isState(variable) ||
+                  (isProp(variable) && !isHOCProp(variable)),
+              );
+            const isArgsExternal = callExpr.arguments
+              .flatMap((arg) => getDownstreamRefs(context, arg))
+              .flatMap((ref) =>
+                getUpstreamReactVariables(context, ref.identifier),
+              )
+              .some(
+                (variable) =>
+                  (!isState(variable) && !isProp(variable)) ||
+                  isHOCProp(variable),
+              );
+            const isDepsInternal = depsRefs
+              .flatMap((ref) =>
+                getUpstreamReactVariables(context, ref.identifier),
+              )
+              .notEmptyEvery(
+                (variable) =>
+                  isState(variable) ||
+                  (isProp(variable) && !isHOCProp(variable)),
+              );
+
+            if (isArgsInternal) {
               // TODO: Can also warn if this is the only call to the setter,
               // even if the arg is external (and not retrieved in the effect)...
               // Does it matter whether the args are in the deps array?
@@ -111,30 +152,28 @@ export const rule = {
                 messageId: messageIds.avoidDerivedState,
                 data: { state: stateName },
               });
-            } else if (isInternalEffect) {
-              if (depsRefs.some((ref) => isInternal(context, ref))) {
-                context.report({
-                  node: callExpr,
-                  messageId: messageIds.avoidChainingState,
-                });
-              }
             }
-          }
 
-          // 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?
-          if (isPropCallback(context, ref)) {
+            if (!isArgsInternal && !isArgsExternal && isDepsInternal) {
+              context.report({
+                node: callExpr,
+                messageId: messageIds.avoidChainingState,
+              });
+            }
+          } else if (isPropCallback(context, ref)) {
+            // 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/react.js b/src/util/react.js
@@ -5,17 +5,26 @@ import {
   getCallExpr,
 } from "./ast.js";
 
-// NOTE: Returns false for HOC'd components aside from `memo`.
-// Which is good? Because the developer may not have control over that to e.g. lift state.
-// So we should treat it as external state.
-// TODO: Will not detect when they define the component normally and then export it wrapped in the HOC.
 export const isReactFunctionalComponent = (node) =>
   (node.type === "FunctionDeclaration" ||
     (node.type === "VariableDeclarator" &&
       (node.init.type === "ArrowFunctionExpression" ||
-        (node.init.type === "CallExpression" &&
-          node.init.callee.type === "Identifier" &&
-          node.init.callee.name === "memo")))) &&
+        node.init.type === "CallExpression"))) &&
+  node.id.type === "Identifier" &&
+  node.id.name[0].toUpperCase() === node.id.name[0];
+
+// NOTE: Returns false for known pure HOCs -- `memo` and `forwardRef`.
+// TODO: Will not detect when they define the component normally and then export it wrapped in the HOC.
+// e.g. `const MyComponent = (props) => {...}; export default memo(MyComponent);`
+export const isReactFunctionalHOC = (node) =>
+  node.type === "VariableDeclarator" &&
+  node.init &&
+  node.init.type === "CallExpression" &&
+  node.init.callee.type === "Identifier" &&
+  !["memo", "forwardRef"].includes(node.init.callee.name) &&
+  node.init.arguments.length > 0 &&
+  (node.init.arguments[0].type === "ArrowFunctionExpression" ||
+    node.init.arguments[0].type === "FunctionExpression") &&
   node.id.type === "Identifier" &&
   node.id.name[0].toUpperCase() === node.id.name[0];
 
@@ -97,38 +106,29 @@ export const isPropCallback = (context, ref) =>
     isProp(variable),
   );
 
-// NOTE: Literals are discarded (because they have no variable) and thus do not count against this.
-// TODO: Not entirely sure where in all these every or notEmptyEvery is best...
-export const isInternal = (context, ref) =>
-  getUpstreamReactVariables(context, ref.identifier).notEmptyEvery(
-    (variable) => isState(variable) || isProp(variable),
-  );
-
-export const isArgsInternal = (context, args) =>
-  args.notEmptyEvery((arg) =>
-    getDownstreamRefs(context, arg)
-      // TODO: Why do we need to filter this out prior?
-      // isInternal uses getUpstreamReactVariables which also does.
-      .filter(
-        (ref) =>
-          isProp(ref.resolved) ||
-          ref.resolved.defs.every(
-            // Discount non-prop parameters
-            (def) => def.type !== "Parameter",
-          ),
-      )
-      .notEmptyEvery((ref) => isInternal(context, ref)),
-  );
-
 // NOTE: Global variables (like `JSON` in `JSON.stringify()`) have an empty `defs`; fortunately `[].some() === false`.
 // Also, I'm not sure so far when `defs.length > 1`... haven't seen it with shadowed variables or even redeclared variables with `var`.
-const isState = (variable) => variable.defs.some((def) => isUseState(def.node));
-const isProp = (variable) =>
+export const isState = (variable) =>
+  variable.defs.some((def) => isUseState(def.node));
+export const isProp = (variable) =>
   variable.defs.some(
     (def) =>
       def.type === "Parameter" &&
       isReactFunctionalComponent(
-        // TODO: Simplify this
+        // TODO: Simplify
+        def.node.type === "ArrowFunctionExpression"
+          ? def.node.parent.type === "CallExpression"
+            ? def.node.parent.parent
+            : def.node.parent
+          : def.node,
+      ),
+  );
+export const isHOCProp = (variable) =>
+  variable.defs.some(
+    (def) =>
+      def.type === "Parameter" &&
+      isReactFunctionalHOC(
+        // TODO: Simplify
         def.node.type === "ArrowFunctionExpression"
           ? def.node.parent.type === "CallExpression"
             ? def.node.parent.parent
@@ -232,19 +232,19 @@ const findContainingNode = (useEffectNode) => {
   return useEffectNode.parent.parent;
 };
 
-const getUpstreamReactVariables = (context, ref) =>
+export const getUpstreamReactVariables = (context, node) =>
   getUpstreamVariables(
     context,
-    ref,
+    node,
     // Stop at the *usage* of `useState` - don't go up to the `useState` variable.
     // Not needed for props - they don't go "too far".
     // We could remove this and check for the `useState` variable instead,
     // but then all our tests need to import it so we can traverse up to it.
-    // TODO: Probably some better way to combine these filters.
+    // And would need to change `getUseStateNode()` too?
+    // TODO: Could probably organize these filters better.
     (node) => !isUseState(node),
   ).filter(
     (variable) =>
-      // Discount non-prop parameters
       isProp(variable) ||
       variable.defs.every((def) => def.type !== "Parameter"),
   );
diff --git a/test/chaining-state.test.js b/test/chaining-state.test.js
@@ -59,5 +59,25 @@ new MyRuleTester().run("/chaining-state", {
         },
       ],
     },
+    {
+      name: "In an otherwise valid effect",
+      code: js`
+        function MyComponent() {
+          const [state, setState] = useState();
+          const [otherState, setOtherState] = useState('Meow');
+
+          useEffect(() => {
+            console.log('Meow');
+            setState('Hello World');
+          }, [otherState]);
+        }
+      `,
+      errors: [
+        {
+          messageId: messageIds.avoidChainingState,
+          data: { state: "state" },
+        },
+      ],
+    }
   ],
 });
diff --git a/test/deriving-state.test.js b/test/deriving-state.test.js
@@ -477,6 +477,7 @@ new MyRuleTester().run("/deriving-state", {
             // thus they will always be in sync and it could be computed during render
             // Difficult bit is that a single state setter call is legit when the 
             // external state is initialized inside the effect (i.e. retrieved from external system)
+            // Hopefully 'isDirectCall' will mostly catch that now.
             setSelectedPost(posts[0]);
           }, [posts]);
         }
@@ -509,6 +510,27 @@ new MyRuleTester().run("/deriving-state", {
         },
       ],
     },
+    {
+      name: "From HOC prop with single setter call",
+      todo: true,
+      code: js`
+        import { withRouter } from 'react-router-dom';
+
+        const MyComponent = withRouter(({ history }) => {
+          const [location, setLocation] = useState();
+
+          useEffect(() => {
+            setLocation(history.location);
+          }, [history.location]);
+        });
+      `,
+      errors: [
+        {
+          messageId: messageIds.avoidDerivedState,
+          data: { state: "fullName" },
+        },
+      ],
+    },
     {
       name: "From props via callback setter",
       code: js`
