feat(no-pass-live-state-to-parent): narrow check to passing state

#22

Author: NickvanDyke

src/no-chain-state-updates.js

@@ -53,12 +53,12 @@ export const rule = {
           // But the upstream bit also catches intermediate variables that are ultimately literals.
           // We could either remove that and just check `arg.type === "Literal"`,
           // or could make this more readable by returning 'literal' | 'internal' | 'external' from the util functions...
-          const argsRefs = callExpr.arguments.flatMap((arg) =>
-            getDownstreamRefs(context, arg),
-          );
-          const argsUpstreamVariables = argsRefs.flatMap((ref) =>
-            getUpstreamReactVariables(context, ref.identifier),
-          );
+
+          const argsUpstreamVariables = callExpr.arguments
+            .flatMap((arg) => getDownstreamRefs(context, arg))
+            .flatMap((ref) =>
+              getUpstreamReactVariables(context, ref.identifier),
+            );
           const isAllArgsInternal = argsUpstreamVariables.notEmptyEvery(
             (variable) =>
               isState(variable) || (isProp(variable) && !isHOCProp(variable)),

src/no-pass-live-state-to-parent.js

@@ -6,8 +6,10 @@ import {
   isDirectCall,
   isPropCallback,
   isHOCProp,
+  getUpstreamReactVariables,
+  isState,
 } from "./util/react.js";
-import { getCallExpr } from "./util/ast.js";
+import { getCallExpr, getDownstreamRefs } from "./util/ast.js";
 
 export const name = "no-pass-live-state-to-parent";
 export const messages = {
@@ -24,7 +26,7 @@ export const rule = {
     schema: [],
     messages: {
       [messages.avoidPassingLiveStateToParent]:
-        "Avoid passing live state to parent components in an effect. Instead, lift the state to the parent component and pass it down as a prop.",
+        "Avoid passing live state to parents in an effect. Instead, lift the state to the parent and pass it down to the child as a prop.",
     },
   },
   create: (context) => ({
@@ -40,11 +42,15 @@ export const rule = {
         .filter(
           (ref) => isPropCallback(context, ref) && !isHOCProp(ref.resolved),
         )
-        .filter((ref) => getCallExpr(ref).arguments.length > 0)
         .forEach((ref) => {
           const callExpr = getCallExpr(ref);
-          // TODO: Unsure whether we should care about other things, like whether they're in deps...
-          if (callExpr.arguments.some((arg) => arg.type !== "Literal")) {
+          const argsUpstreamVariables = callExpr.arguments
+            .flatMap((arg) => getDownstreamRefs(context, arg))
+            .flatMap((ref) =>
+              getUpstreamReactVariables(context, ref.identifier),
+            );
+
+          if (argsUpstreamVariables.some((variable) => isState(variable))) {
             context.report({
               node: callExpr,
               messageId: messages.avoidPassingLiveStateToParent,

test/no-pass-live-state-to-parent.test.js

@@ -14,7 +14,31 @@ new MyRuleTester().run(name, rule, {
       `,
     },
     {
-      name: "No-arg prop callback in response to internal state change",
+      name: "Pass live external state",
+      code: js`
+        const Child = ({ onFetched }) => {
+          const data = useSomeAPI();
+
+          useEffect(() => {
+            onFetched(data);
+          }, [onFetched, data]);
+        }
+      `,
+    },
+    {
+      // No idea why someone would do this, so we only check for passing state, but maybe there's a less contrived pattern.
+      // `no-manage-parent` would catch this anyway.
+      name: "Pass prop to parent",
+      code: js`
+        const Child = ({ text, onTextChanged }) => {
+          useEffect(() => {
+            onTextChanged(text);
+          }, [text, onTextChanged]);
+        }
+      `,
+    },
+    {
+      name: "No-arg prop callback",
       code: js`
         function Form({ onClose }) {
           const [name, setName] = useState();
@@ -49,7 +73,7 @@ new MyRuleTester().run(name, rule, {
       `,
     },
     {
-      name: "Prop from library HOC used internally",
+      name: "Pass internal state to HOC prop",
       code: js`
         import { withRouter } from 'react-router-dom';
 
@@ -63,7 +87,7 @@ new MyRuleTester().run(name, rule, {
       `,
     },
     {
-      name: "Prop from library HOC used externally",
+      name: "Pass external state to HOC prop",
       code: js`
         import { withRouter } from 'react-router-dom';
 
@@ -72,7 +96,7 @@ new MyRuleTester().run(name, rule, {
 
           useEffect(() => {
             if (data.error) {
-              history.push('/error-page');
+              history.push(data.error);
             }
           }, [data]);
         });
@@ -81,14 +105,21 @@ new MyRuleTester().run(name, rule, {
   ],
   invalid: [
     {
-      name: "Pass live internal fetched state",
+      name: "Pass live internal state",
       code: js`
-        const Child = ({ onFetched }) => {
-          const [data, setData] = useState();
+        const Child = ({ onTextChanged }) => {
+          const [text, setText] = useState();
 
           useEffect(() => {
-            onFetched(data);
-          }, [onFetched, data]);
+            onTextChanged(text);
+          }, [onTextChanged, text]);
+
+          return (
+            <input
+              type="text"
+              onChange={(e) => setText(e.target.value)}
+            />
+          );
         }
       `,
       errors: [
@@ -98,14 +129,15 @@ new MyRuleTester().run(name, rule, {
       ],
     },
     {
-      name: "Pass live internal state",
+      name: "Pass live internal state AND external state",
       code: js`
         const Child = ({ onTextChanged }) => {
           const [text, setText] = useState();
+          const data = useSomeAPI();
 
           useEffect(() => {
-            onTextChanged(text);
-          }, [onTextChanged, text]);
+            onTextChanged(text, data);
+          }, [onTextChanged, text, data]);
 
           return (
             <input
@@ -124,13 +156,20 @@ new MyRuleTester().run(name, rule, {
     {
       name: "Pass live derived internal state",
       code: js`
-        const Child = ({ onFetched }) => {
-          const [data, setData] = useState();
+        const Child = ({ onTextChanged }) => {
+          const [text, setText] = useState();
 
           useEffect(() => {
-            const firstElement = data[0];
-            onFetched(firstElement);
-          }, [onFetched, data]);
+            const firstChar = text[0];
+            onTextChanged(firstChar);
+          }, [onTextChanged, text]);
+
+          return (
+            <input
+              type="text"
+              onChange={(e) => setText(e.target.value)}
+            />
+          );
         }
       `,
       errors: [
@@ -159,42 +198,7 @@ new MyRuleTester().run(name, rule, {
       ],
     },
     {
-      name: "Pass live external state",
-      code: js`
-        const Child = ({ onFetched }) => {
-          const data = useSomeAPI();
-
-          useEffect(() => {
-            onFetched(data);
-          }, [onFetched, data]);
-        }
-      `,
-      errors: [
-        {
-          messageId: messages.avoidPassingLiveStateToParent,
-        },
-      ],
-    },
-    {
-      name: "Pass derived live external state",
-      code: js`
-        const Child = ({ onFetched }) => {
-          const data = useSomeAPI();
-          const firstElement = data[0];
-
-          useEffect(() => {
-            onFetched(firstElement);
-          }, [onFetched, firstElement]);
-        }
-      `,
-      errors: [
-        {
-          messageId: messages.avoidPassingLiveStateToParent,
-        },
-      ],
-    },
-    {
-      name: "Pass final external state",
+      name: "Pass final internal state",
       code: js`
         function Form({ onSubmit }) {
           const [name, setName] = useState();
@@ -224,23 +228,5 @@ new MyRuleTester().run(name, rule, {
         },
       ],
     },
-    {
-      name: "From props via member function",
-      code: js`
-        function DoubleList({ list }) {
-          const [doubleList, setDoubleList] = useState([]);
-
-          useEffect(() => {
-            setDoubleList(list.concat(list));
-          }, [list]);
-        }
-      `,
-      errors: [
-        {
-          // We consider `list.concat` to essentially be a prop callback
-          messageId: messages.avoidPassingLiveStateToParent,
-        },
-      ],
-    },
   ],
 });