From a1a7dd73c82ef7a7c7653512b0df54dd087b7db8 Mon Sep 17 00:00:00 2001 From: Josh Wilson Date: Mon, 1 Jan 2024 11:44:29 -0500 Subject: [PATCH 1/2] Tighten up checking for tracked scopes in custom hook arguments. --- docs/reactivity.md | 28 +++++++++++++++++++++++ package.json | 2 ++ pnpm-lock.yaml | 16 +++++++++++-- src/rules/reactivity.ts | 42 ++++++++++++++++++++--------------- test/rules/reactivity.test.ts | 37 ++++++++++++++++++++++++++++++ 5 files changed, 105 insertions(+), 20 deletions(-) diff --git a/docs/reactivity.md b/docs/reactivity.md index cd50ae7..010015e 100644 --- a/docs/reactivity.md +++ b/docs/reactivity.md @@ -421,6 +421,18 @@ const result = indexArray(array, (item) => { const [signal] = createSignal(); let el = ; +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 +625,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 index 41507b6..c6a8140 100644 --- 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 index aa0a685..0e07dec 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -8,6 +8,9 @@ dependencies: '@typescript-eslint/utils': specifier: ^6.4.0 version: 6.4.0(eslint@8.43.0)(typescript@5.1.6) + estraverse: + specifier: ^5.3.0 + version: 5.3.0 is-html: specifier: ^2.0.0 version: 2.0.0 @@ -46,6 +49,9 @@ devDependencies: '@types/eslint': specifier: ^8.40.2 version: 8.40.2 + '@types/estraverse': + specifier: ^5.1.7 + version: 5.1.7 '@types/fs-extra': specifier: ^9.0.13 version: 9.0.13 @@ -146,7 +152,7 @@ packages: /@babel/code-frame@7.12.11: resolution: {integrity: sha512-Zt1yodBx1UcyiePMSkWnU4hPqhwq7hGi2nFL1LeA3EUl+q2LQx16MISgJ0+z7dnmgvP9QtIleuETGOiOH1RcIw==} dependencies: - '@babel/highlight': 7.22.10 + '@babel/highlight': 7.22.20 dev: true /@babel/code-frame@7.18.6: @@ -1163,6 +1169,12 @@ packages: '@types/json-schema': 7.0.11 dev: true + /@types/estraverse@5.1.7: + resolution: {integrity: sha512-JRVtdKYZz7VkNp7hMC/WKoiZ8DS3byw20ZGoMZ1R8eBrBPIY7iBaDAS1zcrnXQCwK44G4vbXkimeU7R0VLG8UQ==} + dependencies: + '@types/estree': 1.0.0 + dev: true + /@types/estree@0.0.39: resolution: {integrity: sha512-EYNwp3bU+98cpU4lAWYYL7Zz+2gryWH1qbdDTidVd6hkiR6weksdbMadyXKXNPEkQFhXM+hVO9ZygomHXp+AIw==} dev: true @@ -2350,7 +2362,7 @@ packages: engines: {node: ^8.10.0 || ^10.13.0 || >=11.10.1} hasBin: true dependencies: - '@babel/code-frame': 7.22.10 + '@babel/code-frame': 7.22.13 ajv: 6.12.6 chalk: 2.4.2 cross-spawn: 6.0.5 diff --git a/src/rules/reactivity.ts b/src/rules/reactivity.ts index 0226fe7..2942c62 100644 --- 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,26 @@ export default createRule({ }); } }; + // 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 === "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 +1059,7 @@ export default createRule({ // 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 +1078,7 @@ export default createRule({ ) { // 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 index ae90419..ab07d2a 100644 --- 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(); @@ -808,5 +820,30 @@ export const cases = run("reactivity", rule, { let el = ;`, 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" }], + }, ], }); From ce5606e36793cd6c06e377c366d3f73879d80e33 Mon Sep 17 00:00:00 2001 From: Josh Wilson Date: Mon, 1 Jan 2024 12:02:57 -0500 Subject: [PATCH 2/2] Exclude member expressions from permissive tracking. --- docs/reactivity.md | 4 ++++ src/rules/reactivity.ts | 1 + test/rules/reactivity.test.ts | 7 +++++++ 3 files changed, 12 insertions(+) diff --git a/docs/reactivity.md b/docs/reactivity.md index 010015e..9fdee08 100644 --- a/docs/reactivity.md +++ b/docs/reactivity.md @@ -218,6 +218,10 @@ const Component = (props) => { return
{value()}
; }; +const Component = (props) => { + const [value] = createSignal(props.value); +}; + const Component = (props) => { const derived = () => props.value; const oops = derived(); diff --git a/src/rules/reactivity.ts b/src/rules/reactivity.ts index 2942c62..624ecd3 100644 --- a/src/rules/reactivity.ts +++ b/src/rules/reactivity.ts @@ -836,6 +836,7 @@ export default createRule({ if ( isFunctionNode(traced) || (traced.type === "Identifier" && + traced.parent.type !== "MemberExpression" && !(traced.parent.type === "CallExpression" && traced.parent.callee === traced)) ) { pushTrackedScope(childNode, "called-function"); diff --git a/test/rules/reactivity.test.ts b/test/rules/reactivity.test.ts index ab07d2a..d0c25f1 100644 --- a/test/rules/reactivity.test.ts +++ b/test/rules/reactivity.test.ts @@ -402,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: `