feat(eslint-plugin): implement ESLint plugin for Pinia best practices #3045
Conversation
- Add @pinia/eslint-plugin package with 4 comprehensive rules - require-setup-store-properties-export: ensures all variables and functions in setup stores are exported - no-circular-store-dependencies: prevents circular dependencies between stores - prefer-use-store-naming: enforces useXxxStore naming convention - no-store-in-computed: prevents store instantiation inside computed properties - Includes comprehensive test suites with 100% coverage - Provides auto-fix functionality where applicable - Supports configurable options for naming conventions - Built with TypeScript and modern ESLint flat config format Resolves vuejs#2612
✅ Deploy Preview for pinia-official canceled.
|
WalkthroughAdds a new @pinia/eslint-plugin package including package manifest, build/test configs, TypeScript types, AST and store utilities, four ESLint rules with tests and a README, plus a default plugin export that exposes a recommended flat config. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant ESLint as ESLint Runner
participant Plugin as @pinia/eslint-plugin
participant Rules as Plugin Rules
participant File as Source File (AST)
Dev->>ESLint: run lint with plugin configured
ESLint->>Plugin: load plugin & recommended config
Plugin-->>ESLint: register rules map
ESLint->>File: parse -> AST
ESLint->>Rules: execute rule visitors on AST nodes
Rules->>File: inspect defineStore / computed / returns / calls via utils
alt Violation found
Rules-->>ESLint: report diagnostics and autofix(s)
else No violation
Rules-->>ESLint: no report
end
ESLint-->>Dev: emit diagnostics and suggested fixes
sequenceDiagram
autonumber
participant Rule as no-circular-store-dependencies
participant Utils as ast-utils/store-utils
participant Graph as UsageGraph
rect rgb(235,245,255)
Rule->>Utils: detect defineStore nodes, push store context
Utils-->>Rule: provide store info & usage nodes
Rule->>Graph: record edges (A → B)
end
alt immediate direct cycle detected
Rule-->>ESLint: report direct circularDependency diagnostic
else on Program:exit
Rule->>Graph: traverse graph (DFS) to detect indirect cycles
Rule-->>ESLint: report representative circularDependency(s)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (25)
packages/eslint-plugin/tsup.config.ts (1)
8-8: Externalize TS-ESLint runtime deps (keep bundle lean).Consider adding '@typescript-eslint/utils' to external to avoid bundling rule type helpers (tree-shaking doesn’t matter here).
- external: ['eslint'], + external: ['eslint', '@typescript-eslint/utils'],packages/eslint-plugin/tsconfig.build.json (1)
6-8: Avoid double-emitting d.ts (tsup dts + tsc declarations).You have dts: true in tsup and declaration/declarationMap here. Keep one source of truth to prevent drift and speed builds.
Suggestion: remove "declaration" and "declarationMap" here and let tsup handle DTS, or set dts: false in tsup and rely on tsc.
packages/eslint-plugin/vitest.config.ts (1)
4-8: Stabilize RuleTester runs (optional).If you see flaky tests, disable worker threads for deterministic ESLint RuleTester behavior.
export default defineConfig({ test: { include: ['src/**/*.{test,spec}.ts'], environment: 'node', globals: true, + threads: false }, })packages/eslint-plugin/package.json (2)
58-60: Declare engines to match ESLint 9 requirement.ESLint 9 requires modern Node. Add an engines field to avoid unexpected installs on unsupported Node versions.
"peerDependencies": { "eslint": ">=8.0.0" }, + "engines": { + "node": ">=18.18.0" + },If you plan to support ESLint 8 on Node 16, adjust accordingly and test both matrices.
42-47: Changelog script scope.The conventional-changelog command sets --commit-path . but tags with -l @pinia/eslint-plugin. Ensure it runs from the package dir in workspaces.
Consider:
- "changelog": "conventional-changelog -p angular -i CHANGELOG.md -s --commit-path . -l @pinia/eslint-plugin -r 1" + "changelog": "conventional-changelog -p angular -i CHANGELOG.md -s --commit-path packages/eslint-plugin -l @pinia/eslint-plugin -r 1"Or run with -C packages/eslint-plugin.
packages/eslint-plugin/src/utils/ast-utils.ts (3)
55-75: Handle destructuring declarations.extractDeclarations ignores patterns like const { a, b } = x or const [a, b] = y, missing names that must be exported.
Minimal enhancement:
- Support ObjectPattern and ArrayPattern (walk elements/properties and collect Identifier names).
- Optionally handle aliases (Property with key Identifier and value Identifier).
I can provide a patch if desired.
80-102: Improve return property extraction for key variations.Currently only Identifier keys are collected. Real code may use:
- StringLiteral keys: return { 'user-name': userName }
- Shorthand: covered
- Aliases: return { userName: name } (should this count as exporting “name”?)
Define intended behavior and extend accordingly to reduce false negatives.
10-18: Support qualified calls if needed.isDefineStoreCall only matches Identifier "defineStore". If users call pinia.defineStore or aliased imports, it won’t match.
Option: also accept MemberExpression with property Identifier name === 'defineStore'.
packages/eslint-plugin/src/types.ts (2)
89-100: Avoid shadowing ESLint types with custom context.PiniaRuleContext loosely mirrors ESLint’s RuleContext. Prefer importing types from @typescript-eslint/utils/ts-eslint to keep parity and intellisense.
Replace PiniaRuleContext usages with RuleContext from TS-ESLint, or drop this interface if unused.
75-87: Config shape vs Flat Config.PluginConfig models string severities and tuple options, which aligns with eslintrc. For Flat Config, rules config is the same, but consuming “recommended” differs (array of config objects).
Document both consumption modes or export a dedicated flat config (see comment on index.ts).
packages/eslint-plugin/src/index.ts (1)
21-22: Avoid hardcoding version.Keep meta.version in sync with package.json to prevent drift.
Option: import package.json and set version dynamically, or drop the field.
packages/eslint-plugin/src/rules/__tests__/no-circular-store-dependencies.test.ts (2)
70-122: Add coverage for direct and indirect cycles.Current tests only cover setup-usage diagnostics. Please add cases that assert reporting for:
- Direct cycle (A ↔ B) via actions.
- Indirect cycle (A → B → C → A) via actions.
Example additions:
invalid: [ + // Direct circular dependency across actions + { + code: ` + export const useAStore = defineStore('a', () => { + function fa() { const b = useBStore(); b.gb?.() } + return { fa } + }) + export const useBStore = defineStore('b', () => { + function gb() { const a = useAStore(); a.fa?.() } + return { gb } + }) + `, + errors: [{ messageId: 'circularDependency' }], + }, + // Indirect circular dependency A -> B -> C -> A + { + code: ` + export const useAStore = defineStore('a', () => { + function fa() { useBStore() } + return { fa } + }) + export const useBStore = defineStore('b', () => { + function fb() { useCStore() } + return { fb } + }) + export const useCStore = defineStore('c', () => { + function fc() { useAStore() } + return { fc } + }) + `, + errors: [{ messageId: 'circularDependency' }], + }, ],
54-68: Avoid recommending computed in this rule’s tests/comments.This suite treats “use store in computed” as valid for this rule, but the plugin includes
@pinia/no-store-in-computed(recommended: error). To reduce mixed signals, prefer action-based examples here and leave computed to the dedicated rule’s tests.packages/eslint-plugin/README.md (1)
110-138: Align guidance withno-store-in-computed.The text says “Use them in actions or computed properties,” but
@pinia/no-store-in-computedforbids computed. Recommend actions only.- // Use stores in actions or computed properties + // Use stores in actionsAnd in the message description above:
-Warns about potential circular dependencies between stores and prevents store instantiation in setup function bodies. +Warns about potential circular dependencies between stores and prevents store instantiation in setup function bodies. Use stores in actions.packages/eslint-plugin/src/rules/__tests__/require-setup-store-properties-export.test.ts (1)
71-168: Add edge-case tests for renamed/aliased returns.To harden fixer/diagnostics, consider cases like
return { c: count }(alias),return { ...other, count }(spread), and destructured declarations not exported. This prevents false negatives.packages/eslint-plugin/src/rules/__tests__/prefer-use-store-naming.test.ts (1)
18-116: Expose the expected pattern variables in the rule message.The rule currently sets
data.nameto'{{Name}}'(see rule file), so the rendered message won’t include the actual core name. Tests don’t assert message text, but fixing improves UX.Proposed change in
prefer-use-store-naming.ts(rule file):// compute `displayName` next to `suggestedName` const displayNameFromId = (id: string) => id.split(/[-_]/).map(p => p[0]?.toUpperCase() + p.slice(1)).join('') let displayName = 'Store' // when storeId present: displayName = displayNameFromId(storeId) // else derive from `baseName` (capitalized) as you already do context.report({ node: node.id, messageId: 'invalidNaming', data: { prefix, name: displayName, suffix }, fix(fixer) { return fixer.replaceText(node.id, suggestedName) }, })packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts (2)
110-147: Remove unused parameter and narrow types.
currentStoreNameisn’t used incheckSetupFunctionForStoreUsage. Also avoidanyforcontext.-function checkSetupFunctionForStoreUsage( - setupFunction: TSESTree.FunctionExpression | TSESTree.ArrowFunctionExpression, - currentStoreName: string, - context: any -) { +function checkSetupFunctionForStoreUsage( + setupFunction: TSESTree.FunctionExpression | TSESTree.ArrowFunctionExpression, + context: TSESLint.RuleContext<'circularDependency' | 'setupCircularDependency', []> +) {
34-38: Message recommends computed; align with plugin policy.Update the guidance to avoid suggesting computed here.
- setupCircularDependency: - 'Avoid using other stores directly in setup function body. Use them in computed properties or actions instead.', + setupCircularDependency: + 'Avoid using other stores directly in setup function body. Use them in actions instead.',packages/eslint-plugin/src/rules/prefer-use-store-naming.ts (2)
45-47: Fix message interpolation; show the actual expected name.The reported message always prints "{{Name}}" literally. Use a concrete "expected" string derived from
suggestedName.Apply this diff:
@@ messages: { - invalidNaming: - 'Store function should follow the naming pattern "{{prefix}}{{name}}{{suffix}}"', + invalidNaming: + 'Store function should follow the naming pattern "{{expected}}"', @@ context.report({ node: node.id, messageId: 'invalidNaming', data: { - prefix, - name: '{{Name}}', - suffix, + expected: suggestedName, }, fix(fixer) { return fixer.replaceText(node.id, suggestedName) }, })Also applies to: 97-105
68-79: Handle object-formdefineStore({ id: '...' })and improve core-name derivation.Right now only string-literal first arg is supported. Pinia also accepts an options object with
id. Consider readingidfrom an ObjectExpression and using it to buildsuggestedName.I can provide a small helper to extract
idfrom either"id"or{ id: '...' }if you want it in this rule or shared inast-utils.Also applies to: 81-92
packages/eslint-plugin/src/rules/no-store-in-computed.ts (1)
74-77: Type the rule context.
context: anyloses type-safety. Type it as the rule’sRuleContextgeneric.I can wire the exact type via
ESLintUtils.RuleCreatorgenerics if you want.packages/eslint-plugin/src/rules/require-setup-store-properties-export.ts (2)
79-86: Spread elements in return should disable strict checking.With
return { ...exposed, count }, we can’t know what’s exported; current logic may falsely report. Skip reporting when a spread is present.Apply this diff:
@@ - // Extract exported properties - const exportedProperties = extractReturnProperties(returnStatement) + // Extract exported properties + const exportedProperties = extractReturnProperties(returnStatement) + // Be lenient when spreads are present to avoid false positives + const retArg = returnStatement.argument + if ( + retArg && + retArg.type === 'ObjectExpression' && + retArg.properties.some((p) => p.type === 'SpreadElement') + ) { + return + }Also applies to: 96-126
109-126: Fixer: preserve formatting/trailing commas.
insertTextAfter(lastProperty, ', foo, bar')can fight formatters and comma styles. Consider usingsourceCode.getText()to detect trailing comma and spacing, or insert after the closing brace when there is a trailing comma.I can provide a formatter-aware fixer if you want.
packages/eslint-plugin/src/utils/store-utils.ts (2)
89-95: Safer null-check forinit.Use truthy check for
initto avoid edge cases and keep code concise.Apply this diff:
export function isStoreDefinition(node: TSESTree.VariableDeclarator): boolean { return ( - node.init !== null && - node.init.type === 'CallExpression' && - isDefineStoreCall(node.init) + !!node.init && + node.init.type === 'CallExpression' && + isDefineStoreCall(node.init) ) }
60-67: Optional: support namespaced/aliased store calls.
isStoreUsage()only matches Identifier callees. If teams call viastores.useCartStore(), this will miss. Consider acceptingMemberExpressionwhose property matches the pattern.Happy to sketch a minimal MemberExpression-aware matcher if you want it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
packages/eslint-plugin/README.md(1 hunks)packages/eslint-plugin/package.json(1 hunks)packages/eslint-plugin/src/index.ts(1 hunks)packages/eslint-plugin/src/rules/__tests__/no-circular-store-dependencies.test.ts(1 hunks)packages/eslint-plugin/src/rules/__tests__/no-store-in-computed.test.ts(1 hunks)packages/eslint-plugin/src/rules/__tests__/prefer-use-store-naming.test.ts(1 hunks)packages/eslint-plugin/src/rules/__tests__/require-setup-store-properties-export.test.ts(1 hunks)packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts(1 hunks)packages/eslint-plugin/src/rules/no-store-in-computed.ts(1 hunks)packages/eslint-plugin/src/rules/prefer-use-store-naming.ts(1 hunks)packages/eslint-plugin/src/rules/require-setup-store-properties-export.ts(1 hunks)packages/eslint-plugin/src/types.ts(1 hunks)packages/eslint-plugin/src/utils/ast-utils.ts(1 hunks)packages/eslint-plugin/src/utils/store-utils.ts(1 hunks)packages/eslint-plugin/test_output.txt(1 hunks)packages/eslint-plugin/tsconfig.build.json(1 hunks)packages/eslint-plugin/tsup.config.ts(1 hunks)packages/eslint-plugin/vitest.config.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
packages/eslint-plugin/src/index.ts (4)
packages/eslint-plugin/src/rules/require-setup-store-properties-export.ts (1)
requireSetupStorePropertiesExport(26-132)packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts (1)
noCircularStoreDependencies(24-108)packages/eslint-plugin/src/rules/prefer-use-store-naming.ts (1)
preferUseStoreNaming(19-111)packages/eslint-plugin/src/rules/no-store-in-computed.ts (1)
noStoreInComputed(19-69)
packages/eslint-plugin/src/rules/__tests__/no-store-in-computed.test.ts (1)
packages/eslint-plugin/src/rules/no-store-in-computed.ts (1)
noStoreInComputed(19-69)
packages/eslint-plugin/src/rules/require-setup-store-properties-export.ts (1)
packages/eslint-plugin/src/utils/ast-utils.ts (6)
isDefineStoreCall(10-18)isSetupStore(23-29)getSetupFunction(34-50)extractDeclarations(55-75)findReturnStatement(107-116)extractReturnProperties(80-102)
packages/eslint-plugin/src/rules/__tests__/no-circular-store-dependencies.test.ts (1)
packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts (1)
noCircularStoreDependencies(24-108)
packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts (2)
packages/eslint-plugin/src/utils/ast-utils.ts (3)
isDefineStoreCall(10-18)isSetupStore(23-29)getSetupFunction(34-50)packages/eslint-plugin/src/utils/store-utils.ts (2)
isStoreUsage(60-67)getStoreNameFromUsage(72-84)
packages/eslint-plugin/src/rules/__tests__/require-setup-store-properties-export.test.ts (1)
packages/eslint-plugin/src/rules/require-setup-store-properties-export.ts (1)
requireSetupStorePropertiesExport(26-132)
packages/eslint-plugin/src/rules/prefer-use-store-naming.ts (1)
packages/eslint-plugin/src/utils/ast-utils.ts (1)
isDefineStoreCall(10-18)
packages/eslint-plugin/src/utils/store-utils.ts (3)
packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts (1)
CallExpression(46-100)packages/eslint-plugin/src/rules/no-store-in-computed.ts (1)
CallExpression(38-66)packages/eslint-plugin/src/utils/ast-utils.ts (3)
isDefineStoreCall(10-18)isSetupStore(23-29)getSetupFunction(34-50)
packages/eslint-plugin/src/rules/no-store-in-computed.ts (1)
packages/eslint-plugin/src/utils/store-utils.ts (1)
isStoreUsage(60-67)
packages/eslint-plugin/src/rules/__tests__/prefer-use-store-naming.test.ts (1)
packages/eslint-plugin/src/rules/prefer-use-store-naming.ts (1)
preferUseStoreNaming(19-111)
packages/eslint-plugin/src/types.ts (3)
packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts (1)
CallExpression(46-100)packages/eslint-plugin/src/rules/no-store-in-computed.ts (1)
CallExpression(38-66)packages/eslint-plugin/src/rules/require-setup-store-properties-export.ts (1)
CallExpression(46-129)
packages/eslint-plugin/src/utils/ast-utils.ts (3)
packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts (1)
CallExpression(46-100)packages/eslint-plugin/src/rules/no-store-in-computed.ts (1)
CallExpression(38-66)packages/eslint-plugin/src/rules/require-setup-store-properties-export.ts (1)
CallExpression(46-129)
🔇 Additional comments (3)
packages/eslint-plugin/test_output.txt (1)
1-191: Remove committed test artifacts.
- Delete packages/eslint-plugin/test_output.txt — local Vitest log (contains absolute paths) and a failing test; logs must not be versioned.
- Add to .gitignore: packages/eslint-plugin/test_output.txt
- Failing test: prefer-use-store-naming did not flag
userStore. Either update the rule to enforce prefixuse+ PascalCase core + suffixStore(e.g., requireuseUserStore) or adjust the test expectation. Share the rule file to receive a proposed patch.packages/eslint-plugin/package.json (1)
52-56: Document supported ESLint/TypeScript ranges; correct TypeScript claim
- File: packages/eslint-plugin/package.json (lines 52–56).
- ESLint: devDependency "^9.0.0" vs peer ">=8.0.0" is fine; npm shows [email protected]. If you intend to support both majors, run CI/tests on 8.x and 9.x and document the supported range.
- TypeScript: npm shows [email protected]; "typescript": "~5.8.3" does NOT look ahead (it pins to 5.8.x). Update the range if you want to allow 5.9.x or explicitly document the supported TS versions.
- Node: local node v24.3.0 from the verification run — ensure package.json "engines" matches the minimal Node required by ESLint/TypeScript.
Likely an incorrect or invalid review comment.
packages/eslint-plugin/src/rules/__tests__/no-store-in-computed.test.ts (1)
18-126: LGTM – good coverage paths.Covers top-level instantiation, multiple violations, and arrow-body cases. No changes needed.
packages/eslint-plugin/package.json
Outdated
| "main": "./dist/index.js", | ||
| "module": "./dist/index.mjs", | ||
| "types": "./dist/index.d.ts", | ||
| "files": [ | ||
| "dist/*.js", | ||
| "dist/*.mjs", | ||
| "dist/*.d.ts" | ||
| ], |
There was a problem hiding this comment.
Ensure dist filenames exist (align with tsup output).
"module" and "exports.import" reference ./dist/index.mjs. Without the tsup outExtension change, this file won’t exist.
If you don’t change tsup, point to ./dist/index.js instead. Otherwise keep as-is after applying the tsup outExtension fix.
🤖 Prompt for AI Agents
In packages/eslint-plugin/package.json around lines 34 to 41, the "module" (and
corresponding "exports.import" elsewhere) points to ./dist/index.mjs which won't
exist unless tsup is configured to emit .mjs via outExtension; fix by either
updating your tsup config to set outExtension for .js -> .mjs so index.mjs is
generated, or change "module" to "./dist/index.js" (and update exports.import to
the same) so it points to the actual tsup output; ensure the package.json
entries exactly match the emitted filenames.
packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/require-setup-store-properties-export.ts
Outdated
Show resolved
Hide resolved
- Fix AST parsing to handle destructuring patterns and aliasing - Add extractReturnIdentifiers function for better return property detection - Handle spread elements to avoid false positives - Improve type safety by removing any types - Support object-form defineStore calls with id property - Support qualified/namespaced store calls - Fix message interpolation in prefer-use-store-naming rule - Change autofixes to suggestions for safer refactoring - Update documentation to align with rule behavior
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/eslint-plugin/src/utils/ast-utils.ts (2)
85-106: Missing declarations from nested blocks and functions.The current implementation only extracts declarations from the top level of the function body, missing variables declared in nested blocks, conditionals, loops, or nested functions.
Apply this diff to handle nested declarations:
export function extractDeclarations(body: TSESTree.BlockStatement): { variables: string[] functions: string[] } { const variables: string[] = [] const functions: string[] = [] + const visited = new Set<TSESTree.Node>() - for (const statement of body.body) { - if (statement.type === 'VariableDeclaration') { - for (const declarator of statement.declarations) { - extractIdentifiersFromPattern(declarator.id, variables) - } - } else if (statement.type === 'FunctionDeclaration' && statement.id) { - functions.push(statement.id.name) + function visitNode(node: TSESTree.Node) { + if (visited.has(node)) return + visited.add(node) + + if (node.type === 'VariableDeclaration') { + for (const declarator of node.declarations) { + extractIdentifiersFromPattern(declarator.id, variables) + } + } else if (node.type === 'FunctionDeclaration' && node.id) { + functions.push(node.id.name) + } + + // Visit child nodes + for (const key in node) { + const child = (node as any)[key] + if (child && typeof child === 'object' && child !== node.parent) { + if (Array.isArray(child)) { + child.forEach(visitNode) + } else if (child.type) { + visitNode(child) + } + } } } + body.body.forEach(visitNode) return { variables, functions } }
224-233: Only finds the first return statement, missing early returns.The implementation returns the first
returnstatement but doesn't handle multiple returns in conditional blocks or nested returns.-export function findReturnStatement( - body: TSESTree.BlockStatement -): TSESTree.ReturnStatement | null { - for (const statement of body.body) { - if (statement.type === 'ReturnStatement') { - return statement - } - } - return null -} +export function findReturnStatement( + body: TSESTree.BlockStatement +): TSESTree.ReturnStatement | null { + const visited = new Set<TSESTree.Node>() + + function findReturn(node: TSESTree.Node): TSESTree.ReturnStatement | null { + if (visited.has(node) || node.type === 'FunctionExpression' || node.type === 'ArrowFunctionExpression') { + return null + } + visited.add(node) + + if (node.type === 'ReturnStatement') { + return node + } + + // Check child nodes + for (const key in node) { + const child = (node as any)[key] + if (child && typeof child === 'object' && child !== node.parent) { + if (Array.isArray(child)) { + for (const c of child) { + const result = findReturn(c) + if (result) return result + } + } else if (child.type) { + const result = findReturn(child) + if (result) return result + } + } + } + return null + } + + return findReturn(body) +}packages/eslint-plugin/src/rules/no-store-in-computed.ts (1)
113-122: Consider using ESLint's visitor pattern instead of manual traversal.The manual AST traversal with
visitNodecould miss certain node types and doesn't respect ESLint's traversal optimizations. However, since this is specifically checking within a computed getter function's limited scope, the current approach is acceptable.packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts (1)
189-190: Potential issue with DFS traversal order.The recursive call
dfs(dep)should only happen if the dependency is not already in the current path to avoid incorrectly flagging non-cyclic dependencies.- if (!visited.has(dep)) dfs(dep) - if (inPath.has(dep)) { + if (inPath.has(dep)) { // report the edge(s) participating in the cycle at least once const from = store const to = dep const key = `${from}->${to}` if (!reported.has(key)) { reported.add(key) const node = usageGraph.get(from)?.get(to)?.[0] if (node) { context.report({ node, messageId: 'circularDependency', data: { currentStore: from, usedStore: to }, }) } } + } else if (!visited.has(dep)) { + dfs(dep) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/eslint-plugin/README.md(1 hunks)packages/eslint-plugin/package.json(1 hunks)packages/eslint-plugin/src/index.ts(1 hunks)packages/eslint-plugin/src/rules/__tests__/prefer-use-store-naming.test.ts(1 hunks)packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts(1 hunks)packages/eslint-plugin/src/rules/no-store-in-computed.ts(1 hunks)packages/eslint-plugin/src/rules/prefer-use-store-naming.ts(1 hunks)packages/eslint-plugin/src/rules/require-setup-store-properties-export.ts(1 hunks)packages/eslint-plugin/src/utils/ast-utils.ts(1 hunks)packages/eslint-plugin/tsup.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/eslint-plugin/tsup.config.ts
- packages/eslint-plugin/src/rules/tests/prefer-use-store-naming.test.ts
- packages/eslint-plugin/src/rules/require-setup-store-properties-export.ts
- packages/eslint-plugin/package.json
- packages/eslint-plugin/README.md
- packages/eslint-plugin/src/rules/prefer-use-store-naming.ts
🧰 Additional context used
🧬 Code graph analysis (4)
packages/eslint-plugin/src/rules/no-store-in-computed.ts (1)
packages/eslint-plugin/src/utils/store-utils.ts (1)
isStoreUsage(60-67)
packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts (2)
packages/eslint-plugin/src/utils/ast-utils.ts (3)
isDefineStoreCall(10-22)isSetupStore(56-62)getSetupFunction(67-83)packages/eslint-plugin/src/utils/store-utils.ts (2)
isStoreUsage(60-67)getStoreNameFromUsage(72-84)
packages/eslint-plugin/src/utils/ast-utils.ts (3)
packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts (2)
node(113-117)CallExpression(51-111)packages/eslint-plugin/src/rules/no-store-in-computed.ts (1)
CallExpression(40-51)packages/eslint-plugin/src/rules/require-setup-store-properties-export.ts (1)
CallExpression(47-135)
packages/eslint-plugin/src/index.ts (4)
packages/eslint-plugin/src/rules/require-setup-store-properties-export.ts (1)
requireSetupStorePropertiesExport(27-138)packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts (1)
noCircularStoreDependencies(28-125)packages/eslint-plugin/src/rules/prefer-use-store-naming.ts (1)
preferUseStoreNaming(19-110)packages/eslint-plugin/src/rules/no-store-in-computed.ts (1)
noStoreInComputed(23-54)
🔇 Additional comments (4)
packages/eslint-plugin/src/index.ts (1)
1-48: LGTM! Well-structured plugin implementation.The plugin is properly implemented with correct flat config format, appropriate rule registrations, and clear separation of concerns. The recommended config uses appropriate severity levels for each rule.
packages/eslint-plugin/src/rules/no-store-in-computed.ts (1)
41-51: LGTM! Clean implementation of computed call detection.The rule properly handles both direct function arguments and object-style computed properties with getter functions.
packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts (1)
51-79: LGTM! Proper store context tracking with stack.The implementation correctly uses a stack to track nested store definitions and initializes the necessary tracking structures.
packages/eslint-plugin/src/utils/ast-utils.ts (1)
10-22: No aliaseddefineStoreimports found — no change requiredSearched for
import { defineStore as ... }andconst ... = ...defineStoreacross the repo; no matches — current isDefineStoreCall covers repository usages.
- Fix package.json files field to include .d.mts and .map files - Fix DFS algorithm in circular dependency detection by reordering conditional checks - Enhance AST parsing with recursive traversal for nested declarations in blocks, conditionals, loops - Improve findReturnStatement to handle multiple returns and find main object return - Add findAllReturnStatements function for comprehensive return statement detection These changes improve the robustness of AST parsing and fix potential issues with circular dependency detection while maintaining backward compatibility.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/eslint-plugin/package.json (1)
29-36: Fix ESM/CJS entry points to match actual build outputs (avoid broken package)."module" and "exports.import" point to ./dist/index.mjs. Unless tsup is configured with outExtension to emit .mjs (and likely .cjs), these files won’t exist. Pick one consistent strategy:
Option A — keep default .js outputs (no outExtension):
- Point both "module" and "exports.import" to ./dist/index.js.
Option B — recommended: emit dual .mjs/.cjs and wire exports accordingly:
- Configure tsup outExtension to write .mjs for ESM and .cjs for CJS.
- Update "exports", "main", and "module" to those filenames.
Apply this diff if you choose Option B (dual format with .mjs/.cjs):
- "exports": { - "types": "./dist/index.d.ts", - "require": "./dist/index.js", - "import": "./dist/index.mjs" - }, - "main": "./dist/index.js", - "module": "./dist/index.mjs", + "exports": { + ".": { + "types": "./dist/index.d.ts", + "import": "./dist/index.mjs", + "require": "./dist/index.cjs", + "default": "./dist/index.cjs" + } + }, + "main": "./dist/index.cjs", + "module": "./dist/index.mjs", "types": "./dist/index.d.ts",Companion tsup hint (tsup.config.ts):
export default defineConfig({ entry: ['src/index.ts'], format: ['esm', 'cjs'], dts: true, outDir: 'dist', outExtension({ format }) { return { js: format === 'esm' ? '.mjs' : '.cjs' } }, })
🧹 Nitpick comments (7)
packages/eslint-plugin/src/utils/ast-utils.ts (5)
10-22: BroadenisDefineStoreCallto cover optional chaining and chain expressions.Covers rare but valid patterns like
pinia?.defineStore(...)anddefineStore?.(...). Keeps behavior for direct identifiers and member calls.Apply this diff:
export function isDefineStoreCall( node: TSESTree.Node ): node is TSESTree.CallExpression { - return ( - node.type === 'CallExpression' && - ((node.callee.type === 'Identifier' && - node.callee.name === 'defineStore') || - (node.callee.type === 'MemberExpression' && - !node.callee.computed && - node.callee.property.type === 'Identifier' && - node.callee.property.name === 'defineStore')) - ) + if (node.type !== 'CallExpression') return false + const callee = + node.callee.type === 'ChainExpression' ? node.callee.expression : node.callee + + if (callee.type === 'Identifier') { + return callee.name === 'defineStore' + } + + const isMember = + callee.type === 'MemberExpression' || + // Optional chaining variant in newer parsers + // @ts-expect-error for cross-version compatibility + callee.type === 'OptionalMemberExpression' + + if ( + isMember && + // @ts-expect-error present on both member variants + !callee.computed && + // @ts-expect-error present on both member variants + callee.property.type === 'Identifier' + ) { + // @ts-expect-error present on both member variants + return callee.property.name === 'defineStore' + } + return false }
27-51: Recognize string template IDs ingetStoreId.Support
defineStore(user)and{ id:user}(no interpolations).Apply this diff:
const firstArg = node.arguments[0] if (firstArg?.type === 'Literal' && typeof firstArg.value === 'string') { return firstArg.value } + if ( + firstArg?.type === 'TemplateLiteral' && + firstArg.expressions.length === 0 && + firstArg.quasis.length === 1 + ) { + return firstArg.quasis[0].value.cooked ?? firstArg.quasis[0].value.raw + } if (firstArg?.type === 'ObjectExpression') { for (const prop of firstArg.properties) { if ( prop.type === 'Property' && !prop.computed && prop.key.type === 'Identifier' && prop.key.name === 'id' && - prop.value.type === 'Literal' && - typeof prop.value.value === 'string' + ( + (prop.value.type === 'Literal' && + typeof prop.value.value === 'string') || + (prop.value.type === 'TemplateLiteral' && + prop.value.expressions.length === 0 && + prop.value.quasis.length === 1) + ) ) { - return prop.value.value + return prop.value.type === 'Literal' + ? prop.value.value + : prop.value.quasis[0].value.cooked ?? + prop.value.quasis[0].value.raw } } }
88-156: Capture loop-initializer declarations and de-duplicate outputs inextractDeclarations.We currently miss
for (const x of y)/for (let i = 0; ...)declarations and can report duplicates. Add traversal for loop initializers/left and return unique names.Apply this diff:
case 'ForStatement': - case 'ForInStatement': - case 'ForOfStatement': - if (node.body) { - traverse(node.body) - } - break + if (node.init && node.init.type === 'VariableDeclaration') { + traverse(node.init) + } + if (node.body) traverse(node.body) + break + case 'ForInStatement': + case 'ForOfStatement': + if (node.left && node.left.type === 'VariableDeclaration') { + traverse(node.left) + } + if (node.body) traverse(node.body) + breakAnd de-duplicate before returning:
- traverse(body) - return { variables, functions } + traverse(body) + const uniqVars = [...new Set(variables)] + const uniqFns = [...new Set(functions)] + return { variables: uniqVars, functions: uniqFns }
196-218: Also collect quoted keys inextractReturnProperties.Handles
return { 'count': count }andreturn { ["count"]: count }(non-computed literal).Apply this diff:
for (const prop of returnStatement.argument.properties) { - if (prop.type === 'Property' && prop.key.type === 'Identifier') { - properties.push(prop.key.name) + if (prop.type === 'Property') { + if (prop.key.type === 'Identifier') { + properties.push(prop.key.name) + } else if ( + !prop.computed && + prop.key.type === 'Literal' && + typeof prop.key.value === 'string' + ) { + properties.push(prop.key.value) + } } else if (prop.type === 'SpreadElement') { // Handle spread elements - we can't easily determine what's being spread // so we'll be more lenient in this case } }
224-250: Optionally treatMemberExpressionreturns as exporting the base identifier.Example:
return { total: count.value }could be interpreted as exportingcount. This is subjective; consider gating behind a rule option to avoid false positives.Apply this diff if desired:
if (prop.type === 'Property') { if (prop.shorthand && prop.key.type === 'Identifier') { // Shorthand property: { count } -> count identifiers.push(prop.key.name) } else if (prop.value.type === 'Identifier') { // Aliased property: { total: count } -> count identifiers.push(prop.value.name) + } else if ( + prop.value.type === 'MemberExpression' && + prop.value.object.type === 'Identifier' + ) { + // Return of a sub-property: { total: count.value } -> count (optional) + identifiers.push(prop.value.object.name) } }packages/eslint-plugin/package.json (2)
29-33: Optional: nest exports under "." and add a default fallback.Nesting under "." is the most interoperable shape and adding "default" helps older resolvers.
If you stay on Option A (all .js), minimally:
- "exports": { - "types": "./dist/index.d.ts", - "require": "./dist/index.js", - "import": "./dist/index.mjs" - }, + "exports": { + ".": { + "types": "./dist/index.d.ts", + "import": "./dist/index.js", + "require": "./dist/index.js", + "default": "./dist/index.js" + } + },Also applies to: 36-36
50-55: Align ESLint peer range with actual compatibility.Dev uses eslint ^9.0.0 but peer allows >=8.0.0. If the plugin requires flat config (ESLint 9), narrow the peer to ^9. If it supports ESLint 8.57+ (last 8.x) and 9, declare a union.
Suggested (pick one):
- "^9.0.0"
- ">=8.57.0 || ^9.0.0"
Also consider documenting the minimum supported ESLint in README.
- "peerDependencies": { - "eslint": ">=8.0.0" - }, + "peerDependencies": { + "eslint": ">=8.57.0 || ^9.0.0" + },Also applies to: 63-65
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/eslint-plugin/package.json(1 hunks)packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts(1 hunks)packages/eslint-plugin/src/utils/ast-utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/eslint-plugin/src/utils/ast-utils.ts (3)
packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts (2)
node(113-117)CallExpression(51-111)packages/eslint-plugin/src/rules/no-store-in-computed.ts (1)
CallExpression(40-51)packages/eslint-plugin/src/rules/require-setup-store-properties-export.ts (1)
CallExpression(47-135)
🔇 Additional comments (9)
packages/eslint-plugin/src/utils/ast-utils.ts (6)
255-268: LGTM: spread detection is clear and conservative.
273-345: LGTM:findAllReturnStatementscorrectly avoids nested function scopes.
350-369: LGTM: picking the last object return is a sensible default.
56-62: LGTM: setup‑store detection is accurate for direct function/arrow args.
67-83: LGTM:getSetupFunctionis precise and side‑effect free.
160-191: LGTM: destructuring coverage is thorough (objects, arrays, rest, defaults).packages/eslint-plugin/package.json (3)
60-62: Engines look good.Node >=18.18.0 aligns with modern ESLint and tsup baselines. No changes needed.
37-43: Keep "dist/*.js" — tsup emits CJS as .js (do not switch to .cjs).packages/eslint-plugin/tsup.config.ts builds ["cjs","esm"] and maps the JS extension to ".js" for CJS and ".mjs" for ESM, so the current "files" whitelist in packages/eslint-plugin/package.json is correct.
Likely an incorrect or invalid review comment.
50-59: Verified — devDependencies exist and match the Node 18 target
- typescript "~5.8.3" is published and falls inside typescript-eslint v8's supported TypeScript range (>=4.8.4 <6.0.0). (typescript-eslint.io)
- @typescript-eslint (8.x, your ^8.35.1) is the v8 line which supports ESLint v9. (typescript-eslint.io)
- tsup@^8.5.0 supports format/dts/outExtension; packages/eslint-plugin/tsup.config.ts targets node18 and maps ESM -> .mjs, so the build config matches Node 18 expectations. (blog.logrocket.com)
- vitest@^3.2.4 (Vitest 3.x) expects Node 18+; combined with typescript-eslint's Node requirement (^18.18.0) your toolset is compatible with Node >=18.18. (vitest.dev)
- Add unit tests for isDefineStoreCall, getStoreId, extractDeclarations, and extractReturnProperties - Ensure support for optional chaining, template literals, and nested structures in AST utilities - Improve test coverage to handle edge cases and various argument forms
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
packages/eslint-plugin/src/utils/ast-utils.ts (4)
25-49: Broaden isDefineStoreCallee to cover computed member access and TS wrappers.Support pinia'defineStore', pinia
defineStore, and unwrapping of TS wrappers (non-null, as, satisfies). This reduces false negatives.function isDefineStoreCallee( callee: TSESTree.CallExpression['callee'] ): boolean { // Direct call: defineStore(...) if (callee.type === 'Identifier' && callee.name === 'defineStore') { return true } - // Member expression: pinia.defineStore(...) - if ( - callee.type === 'MemberExpression' && - !callee.computed && - callee.property.type === 'Identifier' && - callee.property.name === 'defineStore' - ) { - return true - } + // Member expression: pinia.defineStore(...), pinia['defineStore'](...), pinia[`defineStore`](...) + if (callee.type === 'MemberExpression') { + if ( + !callee.computed && + callee.property.type === 'Identifier' && + callee.property.name === 'defineStore' + ) { + return true + } + if ( + callee.computed && + callee.property.type === 'Literal' && + callee.property.value === 'defineStore' + ) { + return true + } + if ( + callee.computed && + callee.property.type === 'TemplateLiteral' && + callee.property.expressions.length === 0 && + callee.property.quasis[0]?.value.cooked === 'defineStore' + ) { + return true + } + } // Chain expression (optional chaining): pinia?.defineStore(...) if (callee.type === 'ChainExpression') { return isDefineStoreCallee(callee.expression) } + // TS wrappers: unwrap and re-check + if ( + callee.type === 'TSNonNullExpression' || + callee.type === 'TSAsExpression' || + callee.type === 'TSSatisfiesExpression' + ) { + // @ts-ignore - these nodes expose `.expression` + return isDefineStoreCallee((callee as any).expression) + } + return false }
74-99: Allow computed 'id' keys in getStoreId object-argument form.Support defineStore({ ['id']: 'user' }, ...) and defineStore({ [
id]: 'user' }, ...).// Handle object expression with id property if (firstArg?.type === 'ObjectExpression') { for (const prop of firstArg.properties) { if ( prop.type === 'Property' && - !prop.computed && - prop.key.type === 'Identifier' && - prop.key.name === 'id' + ( + (!prop.computed && + prop.key.type === 'Identifier' && + prop.key.name === 'id') || + (prop.computed && + prop.key.type === 'Literal' && + prop.key.value === 'id') || + (prop.computed && + prop.key.type === 'TemplateLiteral' && + prop.key.expressions.length === 0 && + prop.key.quasis[0]?.value.cooked === 'id') + ) ) { // Handle string literal value if ( prop.value.type === 'Literal' && typeof prop.value.value === 'string' ) { return prop.value.value } // Handle template literal value without interpolations if ( prop.value.type === 'TemplateLiteral' && prop.value.expressions.length === 0 ) { return prop.value.quasis[0]?.value.cooked || null } } } }
340-365: Unwrap TS/JS wrappers when extracting return identifiers.Cases like return { total: count as number }, return { total: (count) } and return { total: count! } are currently missed.
export function extractReturnIdentifiers( returnStatement: TSESTree.ReturnStatement ): string[] { @@ - for (const prop of returnStatement.argument.properties) { + const unwrap = (expr: TSESTree.Expression): TSESTree.Expression => { + // Unwrap TS/JS wrappers to reach the underlying expression + // eslint-disable-next-line no-constant-condition + while (true) { + if (expr.type === 'TSAsExpression' || expr.type === 'TSSatisfiesExpression') { + expr = expr.expression as TSESTree.Expression + continue + } + if (expr.type === 'TSNonNullExpression') { + expr = expr.expression + continue + } + if (expr.type === 'ParenthesizedExpression' || expr.type === 'ChainExpression') { + expr = (expr as any).expression as TSESTree.Expression + continue + } + break + } + return expr + } + + for (const prop of returnStatement.argument.properties) { if (prop.type === 'Property') { if (prop.shorthand && prop.key.type === 'Identifier') { // Shorthand property: { count } -> count identifiers.push(prop.key.name) - } else if (prop.value.type === 'Identifier') { - // Aliased property: { total: count } -> count - identifiers.push(prop.value.name) + } else { + const v = unwrap(prop.value as TSESTree.Expression) + if (v.type === 'Identifier') { + // Aliased property: { total: count } -> count + identifiers.push(v.name) + } } }
389-461: Optional: cover a couple more statement forms in findAllReturnStatements.Consider traversing LabeledStatement and (rare) StaticBlock for completeness. Low impact.
packages/eslint-plugin/src/utils/__tests__/ast-utils.test.ts (2)
198-210: Add coverage for computed string-literal keys.Now that computed string-literal keys are supported, add a test:
it('should extract template literal property keys without interpolations', () => { @@ }) + + it('should extract computed string-literal property keys', () => { + const code = ` + function setup() { + return { ["name"]: value, ['count']: value2 } + } + ` + const ast = parseCode(code) + const func = ast.body[0] as TSESTree.FunctionDeclaration + const returnStmt = func.body!.body[0] as TSESTree.ReturnStatement + const result = extractReturnProperties(returnStmt) + expect(result).toEqual(['name', 'count']) + })
5-13: Consider adding tests for extractReturnIdentifiers, hasSpreadInReturn, findAllReturnStatements/findReturnStatement.These utilities are used by rules but currently untested here. I can add minimal cases covering aliasing with TS assertions, spread presence, and multiple returns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/eslint-plugin/package.json(1 hunks)packages/eslint-plugin/src/utils/__tests__/ast-utils.test.ts(1 hunks)packages/eslint-plugin/src/utils/ast-utils.ts(1 hunks)packages/eslint-plugin/tsup.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/eslint-plugin/tsup.config.ts
- packages/eslint-plugin/package.json
🧰 Additional context used
🧬 Code graph analysis (2)
packages/eslint-plugin/src/utils/__tests__/ast-utils.test.ts (2)
packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts (1)
node(113-117)packages/eslint-plugin/src/utils/ast-utils.ts (4)
isDefineStoreCall(11-19)getStoreId(55-102)extractDeclarations(140-230)extractReturnProperties(272-334)
packages/eslint-plugin/src/utils/ast-utils.ts (3)
packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts (2)
node(113-117)CallExpression(51-111)packages/eslint-plugin/src/rules/require-setup-store-properties-export.ts (1)
CallExpression(47-135)packages/eslint-plugin/src/rules/no-store-in-computed.ts (1)
CallExpression(40-51)
🪛 Biome (2.1.2)
packages/eslint-plugin/src/utils/ast-utils.ts
[error] 308-310: This branch can never execute. Its condition is a duplicate or covered by previous conditions in the if-else-if chain.
(lint/suspicious/noDuplicateElseIf)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Rename extractDeclarations to extractTopLevelDeclarations for clarity and scope consistency - Add extractReturnIdentifiers for improved return property identifier detection - Extend unit tests for AST utilities to cover edge cases and TS/JS wrappers - Update rules to ensure consistent usage of extractTopLevelDeclarations and identifiers handling - Improve test coverage and robustness of rule logic
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
packages/eslint-plugin/src/rules/__tests__/no-circular-store-dependencies.test.ts (1)
55-69: Clarify cross-rule interplay with no-store-in-computed.This "valid for this rule" sample would be flagged by @pinia/no-store-in-computed. Consider a brief inline comment so readers don’t assume it’s generally allowed.
packages/eslint-plugin/src/utils/ast-utils.ts (2)
359-373: Fix unreachable branch: gate non-computed Literal keys with !prop.computed.Without
!prop.computedhere, the subsequent computed‑Literal branch is shadowed (Biome noDuplicateElseIf). Apply:- else if ( - prop.key.type === 'Literal' && - typeof prop.key.value === 'string' - ) { + else if ( + !prop.computed && + prop.key.type === 'Literal' && + typeof prop.key.value === 'string' + ) { properties.push(prop.key.value) }
59-68: Avoid ts-ignore by using a type guard for wrappers.Minor polish: replace the ts-ignore with a structural check.
- if ( - callee.type === 'TSNonNullExpression' || - callee.type === 'TSAsExpression' || - callee.type === 'TSSatisfiesExpression' - ) { - // @ts-ignore - these nodes expose `.expression` - return isDefineStoreCallee((callee as any).expression) - } + if ( + callee.type === 'TSNonNullExpression' || + callee.type === 'TSAsExpression' || + callee.type === 'TSSatisfiesExpression' + ) { + const expr = (callee as unknown as { expression?: TSESTree.Node }).expression + return expr ? isDefineStoreCallee(expr as any) : false + }packages/eslint-plugin/src/rules/require-setup-store-properties-export.ts (1)
104-133: Make the fixer brace-safe and formatting-friendly.Insert before the closing brace instead of after the last property. This avoids awkward comma placement and better preserves formatting/comments.
- } else { - // Add to existing properties - const lastProperty = - existingProperties[existingProperties.length - 1] - return fixer.insertTextAfter( - lastProperty, - `, ${newProperties.join(', ')}` - ) - } + } else { + // Add before the closing "}" to respect trailing commas and comments + const objEnd = objectExpression.range![1] + const insertPos = objEnd - 1 // position of "}" + const text = `${existingProperties.length > 0 ? ', ' : ''}${newProperties.join(', ')}` + return fixer.insertTextBeforeRange([insertPos, insertPos], text) + }Please run the rule’s tests with objects containing trailing commas and comments in the return to confirm the fixer preserves formatting.
packages/eslint-plugin/src/utils/__tests__/ast-utils.test.ts (3)
47-75: Add bracket/computed callee forms to isDefineStoreCall tests.Cover
pinia["defineStore"](...)andpinia[\defineStore`](...)` to match the helper logic.it('should detect member expression defineStore calls', () => { const code = 'pinia.defineStore("test", () => {})' const ast = parseCode(code) const callExpr = findCallExpression(ast) expect(isDefineStoreCall(callExpr)).toBe(true) }) + + it('should detect bracket member expression defineStore calls', () => { + const code = 'pinia["defineStore"]("test", () => {})' + const ast = parseCode(code) + const callExpr = findCallExpression(ast) + expect(isDefineStoreCall(callExpr)).toBe(true) + }) + + it('should detect computed template member expression defineStore calls', () => { + const code = 'pinia[`defineStore`]("test", () => {})' + const ast = parseCode(code) + const callExpr = findCallExpression(ast) + expect(isDefineStoreCall(callExpr)).toBe(true) + })
77-112: Extend getStoreId tests for computed keys.Exercise
defineStore({ ["id"]: "user" }, ...)and template computed key.it('should extract IDs from object expressions with template literals', () => { const code = 'defineStore({ id: `user` }, () => {})' const ast = parseCode(code) const callExpr = findCallExpression(ast) expect(getStoreId(callExpr)).toBe('user') }) + + it('should extract IDs from computed string-literal keys', () => { + const code = 'defineStore({ ["id"]: "user" }, () => {})' + const ast = parseCode(code) + const callExpr = findCallExpression(ast) + expect(getStoreId(callExpr)).toBe('user') + }) + + it('should extract IDs from computed template-literal keys without interpolations', () => { + const code = 'defineStore({ [`id`]: `user` }, () => {})' + const ast = parseCode(code) + const callExpr = findCallExpression(ast) + expect(getStoreId(callExpr)).toBe('user') + })
241-255: Consider adding a focused test for findReturnStatement.Add a case with multiple returns to assert it picks the last object return.
it('extractReturnIdentifiers should unwrap TS/JS wrappers to find identifiers', () => { ... }) + + it('findReturnStatement should return the last object-return', () => { + const code = ` + function setup() { + if (cond) return 1 + if (cond2) return { a: 1 } + return { b: 2 } + } + ` + const ast = parseCode(code) + const func = ast.body[0] as TSESTree.FunctionDeclaration + // @ts-expect-error: import from module under test + const { findReturnStatement } = await import('../ast-utils') + const ret = findReturnStatement(func.body!) + expect(ret!.argument!.type).toBe('ObjectExpression') + })If dynamic import isn’t convenient in this test file, export the helper via the existing imports at the top.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
packages/eslint-plugin/package.json(1 hunks)packages/eslint-plugin/src/rules/__tests__/no-circular-store-dependencies.test.ts(1 hunks)packages/eslint-plugin/src/rules/__tests__/no-store-in-computed.test.ts(1 hunks)packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts(1 hunks)packages/eslint-plugin/src/rules/no-store-in-computed.ts(1 hunks)packages/eslint-plugin/src/rules/prefer-use-store-naming.ts(1 hunks)packages/eslint-plugin/src/rules/require-setup-store-properties-export.ts(1 hunks)packages/eslint-plugin/src/utils/__tests__/ast-utils.test.ts(1 hunks)packages/eslint-plugin/src/utils/ast-utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/eslint-plugin/package.json
- packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts
- packages/eslint-plugin/src/rules/prefer-use-store-naming.ts
- packages/eslint-plugin/src/rules/no-store-in-computed.ts
- packages/eslint-plugin/src/rules/tests/no-store-in-computed.test.ts
🧰 Additional context used
🧬 Code graph analysis (4)
packages/eslint-plugin/src/rules/require-setup-store-properties-export.ts (1)
packages/eslint-plugin/src/utils/ast-utils.ts (7)
isDefineStoreCall(11-19)isSetupStore(140-146)getSetupFunction(151-167)extractTopLevelDeclarations(269-300)findReturnStatement(562-581)extractReturnIdentifiers(408-454)hasSpreadInReturn(459-472)
packages/eslint-plugin/src/utils/__tests__/ast-utils.test.ts (1)
packages/eslint-plugin/src/utils/ast-utils.ts (5)
isDefineStoreCall(11-19)getStoreId(81-135)extractTopLevelDeclarations(269-300)extractReturnProperties(342-402)extractReturnIdentifiers(408-454)
packages/eslint-plugin/src/rules/__tests__/no-circular-store-dependencies.test.ts (1)
packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts (1)
noCircularStoreDependencies(28-124)
packages/eslint-plugin/src/utils/ast-utils.ts (3)
packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts (2)
node(112-116)CallExpression(50-110)packages/eslint-plugin/src/rules/no-store-in-computed.ts (1)
CallExpression(39-50)packages/eslint-plugin/src/rules/require-setup-store-properties-export.ts (1)
CallExpression(46-136)
🪛 Biome (2.1.2)
packages/eslint-plugin/src/utils/ast-utils.ts
[error] 368-370: This branch can never execute. Its condition is a duplicate or covered by previous conditions in the if-else-if chain.
(lint/suspicious/noDuplicateElseIf)
🔇 Additional comments (3)
packages/eslint-plugin/src/utils/ast-utils.ts (1)
169-300: Good addition: top‑level declaration extractor reduces false positives.Thanks for adding
extractTopLevelDeclarationsand keeping the recursive variant for other use cases.packages/eslint-plugin/src/rules/require-setup-store-properties-export.ts (1)
27-42: Rule intent and docs URL look good.Meta/messages/readability are solid.
packages/eslint-plugin/src/rules/__tests__/no-circular-store-dependencies.test.ts (1)
19-125: Add coverage for direct and indirect cycles (messageId: "circularDependency")Verified checkIndirectCircularDependencies is implemented and invoked in packages/eslint-plugin/src/rules/no-circular-store-dependencies.ts; current tests only exercise setupCircularDependency — add these invalid cases to packages/eslint-plugin/src/rules/tests/no-circular-store-dependencies.test.ts:
invalid: [ + // Direct cycle between two setup stores + { + code: ` + export const useAStore = defineStore('a', () => { + const b = useBStore() + return {} + }) + export const useBStore = defineStore('b', () => { + const a = useAStore() + return {} + }) + `, + errors: [{ messageId: 'circularDependency' }], + }, + // Indirect cycle A -> B -> C -> A + { + code: ` + export const useAStore = defineStore('a', () => { + const b = useBStore() + return {} + }) + export const useBStore = defineStore('b', () => { + const c = useCStore() + return {} + }) + export const useCStore = defineStore('c', () => { + const a = useAStore() + return {} + }) + `, + errors: [{ messageId: 'circularDependency' }], + }, ],
|
Hi~ This looks really interesting! Is there anything I can help with? Maybe we could use |
|
The current ESLint plugin(eslint-plugin-pinia) isn’t actively maintained, and I also think an official plugin would be better. But, we’d of course need the official team’s support to make it happen. |
|
@9romise Perhaps we should cherry-pick the most essential rules from the non-maintained plugin. We can then create a plan for what should be added before we release the official version. |
Resolves #2612
Summary by CodeRabbit