feat(no-pass-ref-to-parent): new rule, and skip ref event handlers in no-pass-data-to-parent

#47

Author: NickvanDyke

README.md

@@ -174,6 +174,31 @@ function Child({ onDataFetched }) {
 }
 ```
 
+### `no-pass-ref-to-parent` — [docs](https://react.dev/reference/react/forwardRef)
+
+Disallow passing refs, or data from callbacks registered on them, to parents in an effect. Use `forwardRef` instead:
+
+```js
+function Child({ onRef }) {
+  const ref = useRef();
+
+  useEffect(() => {
+    onRef(ref.current);
+  }, [onRef, ref.current]);
+}
+```
+
+```js
+const Child = ({ onClicked }) => {
+  const ref = useRef();
+  useEffect(() => {
+    ref.current.addEventListener('click', (event) => {
+      onClicked(event);
+    });
+  }, [onClicked]);
+}
+```
+
 ### `no-initialize-state`
 
 Disallow initializing state in an effect:

src/index.js

@@ -8,6 +8,7 @@ import noChainStateUpdates from "./rules/no-chain-state-updates.js";
 import noDerivedState from "./rules/no-derived-state.js";
 import noPassDataToParent from "./rules/no-pass-data-to-parent.js";
 import noManageParent from "./rules/no-manage-parent.js";
+import noPassRefToParent from "./rules/no-pass-ref-to-parent.js";
 import globals from "globals";
 
 /**
@@ -26,6 +27,7 @@ const plugin = {
     "no-pass-live-state-to-parent": noPassLiveStateToParent,
     "no-pass-data-to-parent": noPassDataToParent,
     "no-manage-parent": noManageParent,
+    "no-pass-ref-to-parent": noPassRefToParent,
     "no-initialize-state": noInitializeState,
     "no-chain-state-updates": noChainStateUpdates,
     "no-derived-state": noDerivedState,

src/rules/no-pass-data-to-parent.js

@@ -8,6 +8,7 @@ import {
   hasCleanup,
   isUseEffect,
   getUpstreamRefs,
+  isImmediateCall,
 } from "../util/ast.js";
 import { getCallExpr, getDownstreamRefs } from "../util/ast.js";
 
@@ -36,8 +37,7 @@ export default {
 
       effectFnRefs
         .filter((ref) => isPropCallback(context, ref))
-        // NOTE: We don't check `isDirectCall` because passing data to the parent is passing data to the parent.
-        // And misuses are often indirect, e.g. retrieving and passing up external data in a Promise chain.
+        .filter((ref) => isImmediateCall(ref.identifier))
         .forEach((ref) => {
           const callExpr = getCallExpr(ref);
 
@@ -46,19 +46,8 @@ export default {
             callExpr.arguments
               .flatMap((arg) => getDownstreamRefs(context, arg))
               .flatMap((ref) => getUpstreamRefs(context, ref))
-              // `every` instead of the usual `notEmptyEvery` because `getUpstreamRefs` filters out
-              // parameters, e.g. in Promise chains or callbacks, but we want to flag passing those.
-              // Ideally we'd identify and check the parameter's "source" though...
-              .every(
-                (ref) =>
-                  !isState(ref) &&
-                  !isProp(ref) &&
-                  // TODO: Should advise to use `forwardRef` instead?
-                  // Not always the best solution, but usually, and outliers can silence the warning.
-                  // Could possibly check for refs on the "path" to this callback too.
-                  // https://github.com/NickvanDyke/eslint-plugin-react-you-might-not-need-an-effect/issues/22
-                  // https://github.com/NickvanDyke/eslint-plugin-react-you-might-not-need-an-effect/issues/37
-                  !isRef(ref),
+              .notEmptyEvery(
+                (ref) => !isState(ref) && !isProp(ref) && !isRef(ref),
               );
 
           if (isAllData) {

src/rules/no-pass-ref-to-parent.js

@@ -0,0 +1,76 @@
+import {
+  getEffectFnRefs,
+  getEffectDepsRefs,
+  isPropCallback,
+  isRef,
+  hasCleanup,
+  isUseEffect,
+  getUpstreamRefs,
+  isRefCall,
+} from "../util/ast.js";
+import { getCallExpr, getDownstreamRefs } from "../util/ast.js";
+
+/**
+ * @type {import("eslint").Rule.RuleModule}
+ */
+export default {
+  meta: {
+    type: "suggestion",
+    docs: {
+      description:
+        "Disallow passing refs, or data from callbacks registered on them, to parents in an effect. Use `forwardRef` instead.",
+      url: "https://react.dev/reference/react/forwardRef",
+    },
+    schema: [],
+    messages: {
+      avoidPassingRefToParent:
+        "Avoid passing refs to parents in an effect. Use `forwardRef` instead.",
+      avoidPropCallbackInRefCallback:
+        "Avoid calling props inside callbacks registered on refs in an effect. Use `forwardRef` to register the callback in the parent instead.",
+    },
+  },
+  create: (context) => ({
+    CallExpression: (node) => {
+      if (!isUseEffect(node) || hasCleanup(node)) return;
+      const effectFnRefs = getEffectFnRefs(context, node);
+      const depsRefs = getEffectDepsRefs(context, node);
+      if (!effectFnRefs || !depsRefs) return;
+
+      effectFnRefs
+        .filter((ref) => isPropCallback(context, ref))
+        .forEach((ref) => {
+          const callExpr = getCallExpr(ref);
+
+          const hasRefArg = callExpr.arguments
+            .flatMap((arg) => getDownstreamRefs(context, arg))
+            .flatMap((ref) => getUpstreamRefs(context, ref))
+            .some((ref) => isRef(ref));
+
+          if (hasRefArg) {
+            context.report({
+              node: callExpr,
+              messageId: "avoidPassingRefToParent",
+            });
+          }
+        });
+
+      effectFnRefs
+        .filter((ref) => isRefCall(context, ref))
+        .forEach((ref) => {
+          const callExpr = getCallExpr(ref);
+
+          const passesCallbackDataToParent = callExpr.arguments
+            .flatMap((arg) => getDownstreamRefs(context, arg))
+            .flatMap((ref) => getUpstreamRefs(context, ref))
+            .some((ref) => isPropCallback(context, ref));
+
+          if (passesCallbackDataToParent) {
+            context.report({
+              node: getCallExpr(ref),
+              messageId: "avoidPropCallbackInRefCallback",
+            });
+          }
+        });
+    },
+  }),
+};

src/util/ast.js

@@ -134,6 +134,9 @@ export const isStateSetter = (context, ref) =>
 export const isPropCallback = (context, ref) =>
   getCallExpr(ref) !== undefined &&
   getUpstreamRefs(context, ref).some((ref) => isProp(ref));
+export const isRefCall = (context, ref) =>
+  getCallExpr(ref) !== undefined &&
+  getUpstreamRefs(context, ref).some((ref) => isRef(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`.

test/no-pass-data-to-parent.test.js

@@ -3,6 +3,16 @@ import rule from "../src/rules/no-pass-data-to-parent.js";
 
 new MyRuleTester().run("no-pass-data-to-parent", rule, {
   valid: [
+    {
+      name: "Pass literal value",
+      code: js`
+        const Child = ({ onTextChanged }) => {
+          useEffect(() => {
+            onTextChanged("Hello World");
+          }, [onTextChanged]);
+        }
+      `,
+    },
     {
       name: "Pass internal state",
       code: js`
@@ -102,11 +112,6 @@ new MyRuleTester().run("no-pass-data-to-parent", rule, {
       `,
     },
     {
-      // TODO: Definitely an anti-pattern, but may fit better elsewhere
-      // because refs are not quite state, e.g. don't cause re-renders.
-      // However they *are* local to the component...
-      // At the least, could use a different message when flagged.
-      // Or maybe user should be advised to use `forwardRef` instead? I think that's idiomatic.
       name: "Pass ref to parent",
       code: js`
         const Child = ({ onRef }) => {
@@ -115,8 +120,6 @@ new MyRuleTester().run("no-pass-data-to-parent", rule, {
           useEffect(() => {
             onRef(ref.current);
           }, [onRef, ref.current]);
-
-          return <div ref={ref}>Child</div>;
         }
       `,
     },
@@ -165,57 +168,35 @@ new MyRuleTester().run("no-pass-data-to-parent", rule, {
         }
       `,
     },
-  ],
-  invalid: [
-    {
-      name: "Pass literal value",
-      code: js`
-        const Child = ({ onTextChanged }) => {
-          useEffect(() => {
-            onTextChanged("Hello World");
-          }, [onTextChanged]);
-        }
-      `,
-      errors: [
-        {
-          messageId: "avoidPassingDataToParent",
-        },
-      ],
-    },
     {
-      name: "Pass external state",
+      name: "Register callback on ref to pass data to parent",
       code: js`
-        const Child = ({ onFetched }) => {
-          const data = useSomeAPI();
+        const Child = ({ onClicked }) => {
+          const ref = useRef();
 
           useEffect(() => {
-            onFetched(data);
-          }, [onFetched, data]);
+            ref.current.addEventListener('click', (event) => {
+              onClicked(event);
+            });
+          }, [onFetched]);
         }
       `,
-      errors: [
-        {
-          messageId: "avoidPassingDataToParent",
-        },
-      ],
     },
     {
-      name: "Pass derived external state",
+      // We get lucky in this example that `getUpstreamRefs` ignores parameter-declared variables,
+      // and thus we don't flag `addEventListener`. But not certain that's totally reliable.
+      // TODO: We might want to catch this but it's tricky to identify the prop as a ref w/o type info.
+      // Probably with a different message or even rule, about the child controlling the parent and idiomatic React control flow.
+      name: "Register callback on ref prop",
       code: js`
-        const Child = ({ onFetched }) => {
-          const data = useSomeAPI();
-          const firstElement = data[0];
-
+        const Child = ({ ref }) => {
           useEffect(() => {
-            onFetched(firstElement);
-          }, [onFetched, firstElement]);
+            ref.current.addEventListener('click', (event) => {
+              console.log('Clicked', event);
+            });
+          }, [ref]);
         }
       `,
-      errors: [
-        {
-          messageId: "avoidPassingDataToParent",
-        },
-      ],
     },
     {
       name: "Pass external state that's retrieved in effect via .then",
@@ -227,11 +208,13 @@ new MyRuleTester().run("no-pass-data-to-parent", rule, {
           }, []);
         }
       `,
-      errors: [
-        {
-          messageId: "avoidPassingDataToParent",
-        },
-      ],
+      // TODO: Difficult because `getUpstreamRefs` ignores parameter-declared variables,
+      // and the above test case relies on that behavior.
+      // errors: [
+      //   {
+      //     messageId: "avoidPassingDataToParent",
+      //   },
+      // ],
     },
     {
       name: "Pass external state that's retrieved in effect via async/await",
@@ -245,27 +228,42 @@ new MyRuleTester().run("no-pass-data-to-parent", rule, {
           }, []);
         }
       `,
+      // TODO:
+      // errors: [
+      //   {
+      //     messageId: "avoidPassingDataToParent",
+      //   },
+      // ],
+    },
+  ],
+  invalid: [
+    {
+      name: "Pass external state",
+      code: js`
+        const Child = ({ onFetched }) => {
+          const data = useSomeAPI();
+
+          useEffect(() => {
+            onFetched(data);
+          }, [onFetched, data]);
+        }
+      `,
       errors: [
         {
           messageId: "avoidPassingDataToParent",
         },
       ],
     },
     {
-      // If parent needs to listen to child's DOM events, it should set up the listener itself.
-      // TODO: Better message? Advise to use `forwardRef`.
-      name: "Register callback on ref to pass data to parent",
+      name: "Pass derived external state",
       code: js`
-        const Child = ({ onClicked }) => {
-          const ref = useRef();
+        const Child = ({ onFetched }) => {
+          const data = useSomeAPI();
+          const firstElement = data[0];
 
           useEffect(() => {
-            ref.current.addEventListener('click', (event) => {
-              onClicked(event);
-            });
-          }, [onFetched]);
-
-          return <SomeComponent ref={ref} />;
+            onFetched(firstElement);
+          }, [onFetched, firstElement]);
         }
       `,
       errors: [