fix(eslint-plugin): exhaustive-deps with variables and type assertions (#9643) #9687
Conversation
TanStack#9643) Recursively dereference variables and type assertions in query keys. Also dereference even if the referenced expression is not an array expression.
WalkthroughReplaces inline unwrapping in the exhaustive-deps rule with a centralized dereference helper and adds valid tests for query key factories and const-assertion scenarios so dereferenced/const-asserted variables are treated as stable query keys. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer Code
participant Rule as ESLint: exhaustive-deps.rule
participant Helper as dereferenceVariablesAndTypeAssertions
participant Analyzer as Dependency Analyzer
Dev->>Rule: useQuery({ queryKey, queryFn })
Rule->>Helper: dereferenceVariablesAndTypeAssertions(queryKey.value, context)
Helper-->>Rule: resolvedQueryKeyNode (or current node on cycle)
Rule->>Analyzer: inspect resolvedQueryKeyNode for external refs / missing deps
Analyzer-->>Rule: missing deps / none
alt Missing dependencies
Rule-->>Dev: report missing dependencies
else
Rule-->>Dev: no findings (valid)
end
Estimated code review effortπ― 3 (Moderate) | β±οΈ ~25 minutes Poem
Pre-merge checks and finishing touchesβ Failed checks (1 warning)
β Passed checks (4 passed)
β¨ Finishing touches
π§ͺ Generate unit tests
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: 1
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (2)
packages/eslint-plugin-query/src/__tests__/exhaustive-deps.test.ts(2 hunks)packages/eslint-plugin-query/src/rules/exhaustive-deps/exhaustive-deps.rule.ts(2 hunks)
π§° Additional context used
𧬠Code graph analysis (1)
packages/eslint-plugin-query/src/__tests__/exhaustive-deps.test.ts (1)
packages/eslint-plugin-query/src/__tests__/test-utils.ts (1)
normalizeIndent(3-7)
πͺ Biome (2.1.2)
packages/eslint-plugin-query/src/rules/exhaustive-deps/exhaustive-deps.rule.ts
[error] 175-178: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
π Additional comments (2)
packages/eslint-plugin-query/src/__tests__/exhaustive-deps.test.ts (2)
229-260: Great coverage for the query key factory regressionCodifies the exact reproduction from #9643, and the paired variant with layered const assertions ensures the new dereferencing logic handles call expressions and returned tuples without flagging false positives. Nicely done.
283-305: Const assertion scenarios nicely capturedThese two cases exercise both the βassert at usage siteβ and βassert at declarationβ pathways, giving strong confidence that recursive dereferencing now accounts for TypeScript
as constchains.
packages/eslint-plugin-query/src/rules/exhaustive-deps/exhaustive-deps.rule.ts
Outdated
Show resolved
Hide resolved
β¦sertions (TanStack#9643) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (1)
packages/eslint-plugin-query/src/rules/exhaustive-deps/exhaustive-deps.rule.ts(2 hunks)
π Additional comments (2)
packages/eslint-plugin-query/src/rules/exhaustive-deps/exhaustive-deps.rule.ts (2)
68-68: Good call centralizing dereferencing; confirm fixer intent across variable initializers.Using the dereferenced node enables correct key analysis and now allows fixing array literals defined via variables. Please confirm it's intended that the fixer edits the variableβs initializer (when the array literal is defined elsewhere in the same file) rather than the queryKey property usage.
175-186: noSwitchDeclarations resolved.Wrapping the Identifier case body in braces fixes the Biome lint error. LGTM.
packages/eslint-plugin-query/src/rules/exhaustive-deps/exhaustive-deps.rule.ts
Outdated
Show resolved
Hide resolved
|
Hi! I agree that the current behavior (false positive) is an actual bug and should be fixed, but are we sure we want to support query key factory? it can get quite complicated; I'll give a basic example: const createKey = (id) => ["foo"];
function useHook({ id }) {
return useQuery({ queryKey: createKey({ id }), ... }); // Should it be an error? since `createKey` doesn't pass the `id` to the final array.
}I'm not against supporting it, but it's important to note that if we're going to support it, we should decide how βdeepβ we want to get into this, especially now that Thoughts? @TkDodo |
|
I was under the impression that query key factories are already (partially) supported. Given your example, I understand that the support isn't perfect, and probably never will be. However, I would have thought that this PR touches query key factory support only peripherally. |
|
I think the example is somewhat flawed because why would you a pass a value (id) to a key factory if it has no influence on the key whatsoever? So yes, this should error, because for all we know, you call a function with |
β¦type-assertions
π¦ Changeset detectedLatest commit: e21267d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
View your CI Pipeline Execution β for commit e21267d
βοΈ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 0
π§Ή Nitpick comments (1)
.changeset/cold-falcons-warn.md (1)
5-6: Tighten the changeset message and autoβlink the issue.Make the summary more descriptive and add βFixes #9643β so GitHub autoβcloses the issue.
Apply this diff:
-fix: exhaustive-deps with variables and type assertions +fix(exhaustive-deps): recursively dereference identifiers and TS assertions in query keys (variables, as-const), even when the referenced expression is not an array; avoid crashes on cycles + +Fixes #9643
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (2)
.changeset/cold-falcons-warn.md(1 hunks)packages/eslint-plugin-query/src/rules/exhaustive-deps/exhaustive-deps.rule.ts(2 hunks)
π§° Additional context used
π§ Learnings (1)
π Learning: 2025-09-02T17:57:33.184Z
Learnt from: TkDodo
PR: TanStack/query#9612
File: packages/query-async-storage-persister/src/asyncThrottle.ts:0-0
Timestamp: 2025-09-02T17:57:33.184Z
Learning: When importing from tanstack/query-core in other TanStack Query packages like query-async-storage-persister, a workspace dependency "tanstack/query-core": "workspace:*" needs to be added to the package.json.
Applied to files:
.changeset/cold-falcons-warn.md
𧬠Code graph analysis (1)
packages/eslint-plugin-query/src/rules/exhaustive-deps/exhaustive-deps.rule.ts (1)
packages/eslint-plugin-query/src/utils/ast-utils.ts (1)
ASTUtils(5-391)
π Additional comments (2)
packages/eslint-plugin-query/src/rules/exhaustive-deps/exhaustive-deps.rule.ts (2)
68-71: Good call: centralizing deref logic.Using a helper for dereferencing reduces duplication and keeps the main rule readable.
171-204: Expand unwrapping to cover all type wrappers
AddTSTypeAssertion,TSNonNullExpression,ParenthesizedExpression, andTSSatisfiesExpressionto the switch indereferenceVariablesAndTypeAssertions, bump the iteration limit (MAX_STEPS = 1 << 10), and renamevisitedNodestovisited:function dereferenceVariablesAndTypeAssertions( queryKeyNode: TSESTree.Node, context: Readonly<TSESLint.RuleContext<string, ReadonlyArray<unknown>>>, ) { - const visitedNodes = new Set<TSESTree.Node>() + const MAX_STEPS = 1 << 10 + const visited = new Set<TSESTree.Node>() - for (let i = 0; i < 1 << 8; ++i) { - if (visitedNodes.has(queryKeyNode)) { + for (let i = 0; i < MAX_STEPS; ++i) { + if (visited.has(queryKeyNode)) return queryKeyNode + visited.add(queryKeyNode) switch (queryKeyNode.type) { - case AST_NODE_TYPES.TSAsExpression: + case AST_NODE_TYPES.TSAsExpression: + case AST_NODE_TYPES.TSTypeAssertion: + case AST_NODE_TYPES.TSNonNullExpression: + case AST_NODE_TYPES.ParenthesizedExpression: + case AST_NODE_TYPES.TSSatisfiesExpression: queryKeyNode = queryKeyNode.expression break β¦
|
My goal in the example was to show that, while we could validate that all required args were passed to the query key factory, it doesnβt really "cut it". I tried to keep the example minimal even though itβs not very practical. A better example might look like this: const createKey = (id) => {
const mock = id > 0;
return ["foo", mock]; // user forgot to also pass the `id`
};
function useHook({ id }) {
return useQuery({ queryKey: createKey({ id }), ... });
}Ensuring that But maybe I'm overthinking it and we should support it as a "best-effort" solution. LGTM @fbieler π |
β¦type-assertions
Codecov Reportβ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9687 +/- ##
===========================================
+ Coverage 46.38% 83.03% +36.65%
===========================================
Files 214 19 -195
Lines 8488 560 -7928
Branches 1929 206 -1723
===========================================
- Hits 3937 465 -3472
+ Misses 4108 73 -4035
+ Partials 443 22 -421 π New features to boost your workflow:
|
π― Changes
Recursively dereference variables and type assertions in query keys.
Also dereference even if the referenced expression is not an array expression.
Fixes #9643
β Checklist
pnpm test:pr.π Release Impact
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores