Merge pull request #125 from solidjs-community/fix/custom-hook-tracking

Tighten up checking for tracked scopes in custom hook arguments.
solidjs-community · Jan 1, 2024 · 02d7879 · 02d7879
2 parents 7f340e1 + ce5606e
commit 02d7879
Show file tree

Hide file tree

Showing 5 changed files with 117 additions and 20 deletions.
diff --git a/docs/reactivity.md b/docs/reactivity.md
@@ -218,6 +218,10 @@ const Component = (props) => {
   return <div>{value()}</div>;
 };
 
+const Component = (props) => {
+  const [value] = createSignal(props.value);
+};
+
 const Component = (props) => {
   const derived = () => props.value;
   const oops = derived();
@@ -421,6 +425,18 @@ const result = indexArray(array, (item) => {
 const [signal] = createSignal();
 let el = <Component staticProp={signal()} />;
 
+const [signal] = createSignal(0);
+useExample(signal());
+
+const [signal] = createSignal(0);
+useExample([signal()]);
+
+const [signal] = createSignal(0);
+useExample({ value: signal() });
+
+const [signal] = createSignal(0);
+useExample((() => signal())());
+
 ```
 
 ### Valid Examples
@@ -613,6 +629,22 @@ function createFoo(v) {}
 const [bar, setBar] = createSignal();
 createFoo({ onBar: () => bar() });
 
+function createFoo(v) {}
+const [bar, setBar] = createSignal();
+createFoo({
+  onBar() {
+    bar();
+  },
+});
+
+function createFoo(v) {}
+const [bar, setBar] = createSignal();
+createFoo(bar);
+
+function createFoo(v) {}
+const [bar, setBar] = createSignal();
+createFoo([bar]);
+
 const [bar, setBar] = createSignal();
 X.createFoo(() => bar());
 

diff --git a/package.json b/package.json
@@ -38,6 +38,7 @@
   ],
   "dependencies": {
     "@typescript-eslint/utils": "^6.4.0",
+    "estraverse": "^5.3.0",
     "is-html": "^2.0.0",
     "kebab-case": "^1.0.2",
     "known-css-properties": "^0.24.0",
@@ -52,6 +53,7 @@
     "@rollup/plugin-node-resolve": "^14.1.0",
     "@tsconfig/node16": "^16.1.0",
     "@types/eslint": "^8.40.2",
+    "@types/estraverse": "^5.1.7",
     "@types/fs-extra": "^9.0.13",
     "@types/is-html": "^2.0.0",
     "@types/jest": "^27.5.2",

diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml
diff --git a/src/rules/reactivity.ts b/src/rules/reactivity.ts
@@ -4,6 +4,7 @@
  */
 
 import { TSESTree as T, TSESLint, ESLintUtils, ASTUtils } from "@typescript-eslint/utils";
+import { traverse } from "estraverse";
 import {
   findParent,
   findInScope,
@@ -17,6 +18,7 @@ import {
   ignoreTransparentWrappers,
   getFunctionName,
   isJSXElementOrFragment,
+  trace,
 } from "../utils";
 
 const { findVariable, getFunctionHeadLocation } = ASTUtils;
@@ -823,6 +825,27 @@ export default createRule<Options, MessageIds>({
           });
         }
       };
+      // given some expression, mark any functions within it as tracking scopes, and do not traverse
+      // those functions
+      const permissivelyTrackNode = (node: T.Node) => {
+        traverse(node as any, {
+          enter(cn) {
+            const childNode = cn as T.Node;
+            const traced = trace(childNode, context.getScope());
+            // when referencing a function or something that could be a derived signal, track it
+            if (
+              isFunctionNode(traced) ||
+              (traced.type === "Identifier" &&
+                traced.parent.type !== "MemberExpression" &&
+                !(traced.parent.type === "CallExpression" && traced.parent.callee === traced))
+            ) {
+              pushTrackedScope(childNode, "called-function");
+              this.skip(); // poor-man's `findInScope`: don't enter child scopes
+            }
+          },
+        });
+      };
+
       if (node.type === "JSXExpressionContainer") {
         if (
           node.parent?.type === "JSXAttribute" &&
@@ -1037,15 +1060,7 @@ export default createRule<Options, MessageIds>({
             // Assume all identifier/function arguments are tracked scopes, and use "called-function"
             // to allow async handlers (permissive). Assume non-resolvable args are reactive expressions.
             for (const arg of node.arguments) {
-              if (isFunctionNode(arg)) {
-                pushTrackedScope(arg, "called-function");
-              } else if (
-                arg.type === "Identifier" ||
-                arg.type === "ObjectExpression" ||
-                arg.type === "ArrayExpression"
-              ) {
-                pushTrackedScope(arg, "expression");
-              }
+              permissivelyTrackNode(arg);
             }
           }
         } else if (node.callee.type === "MemberExpression") {
@@ -1064,15 +1079,7 @@ export default createRule<Options, MessageIds>({
           ) {
             // Handle custom hook parameters for property access custom hooks
             for (const arg of node.arguments) {
-              if (isFunctionNode(arg)) {
-                pushTrackedScope(arg, "called-function");
-              } else if (
-                arg.type === "Identifier" ||
-                arg.type === "ObjectExpression" ||
-                arg.type === "ArrayExpression"
-              ) {
-                pushTrackedScope(arg, "expression");
-              }
+              permissivelyTrackNode(arg);
             }
           }
         }

diff --git a/test/rules/reactivity.test.ts b/test/rules/reactivity.test.ts
@@ -149,6 +149,18 @@ export const cases = run("reactivity", rule, {
     `function createFoo(v) {}
     const [bar, setBar] = createSignal();
     createFoo({ onBar: () => bar() });`,
+    `function createFoo(v) {}
+    const [bar, setBar] = createSignal();
+    createFoo({ onBar() { bar() } });`,
+    `function createFoo(v) {}
+    const [bar, setBar] = createSignal();
+    createFoo(bar);`,
+    `function createFoo(v) {}
+    const [bar, setBar] = createSignal();
+    createFoo([bar]);`,
+    // `function createFoo(v) {}
+    // const [bar, setBar] = createSignal();
+    // createFoo((() => () => bar())());`,
     `const [bar, setBar] = createSignal();
     X.createFoo(() => bar());`,
     `const [bar, setBar] = createSignal();
@@ -390,6 +402,13 @@ export const cases = run("reactivity", rule, {
         },
       ],
     },
+    {
+      code: `
+      const Component = props => {
+        const [value] = createSignal(props.value);
+      }`,
+      errors: [{ messageId: "untrackedReactive", type: T.MemberExpression }],
+    },
     // mark `props` as props by name before we've determined if Component is a component in :exit
     {
       code: `
@@ -808,5 +827,30 @@ export const cases = run("reactivity", rule, {
       let el = <Component staticProp={signal()} />;`,
       errors: [{ messageId: "untrackedReactive" }],
     },
+    // custom hooks
+    {
+      code: `
+      const [signal] = createSignal(0);
+      useExample(signal())`,
+      errors: [{ messageId: "untrackedReactive" }],
+    },
+    {
+      code: `
+      const [signal] = createSignal(0);
+      useExample([signal()])`,
+      errors: [{ messageId: "untrackedReactive" }],
+    },
+    {
+      code: `
+      const [signal] = createSignal(0);
+      useExample({ value: signal() })`,
+      errors: [{ messageId: "untrackedReactive" }],
+    },
+    {
+      code: `
+      const [signal] = createSignal(0);
+      useExample((() => signal())())`,
+      errors: [{ messageId: "expectedFunctionGotExpression" }],
+    },
   ],
 });