diff --git a/.changeset/soft-pianos-sip.md b/.changeset/soft-pianos-sip.md new file mode 100644 index 00000000000..9bf92af6af6 --- /dev/null +++ b/.changeset/soft-pianos-sip.md @@ -0,0 +1,5 @@ +--- +'@tanstack/eslint-plugin-query': patch +--- + +Relax `exhaustive-deps` so function call targets are not required in query keys while values referenced in nested callbacks are still checked. diff --git a/docs/eslint/exhaustive-deps.md b/docs/eslint/exhaustive-deps.md index 3fa56f1a54f..6f6e8725bea 100644 --- a/docs/eslint/exhaustive-deps.md +++ b/docs/eslint/exhaustive-deps.md @@ -3,8 +3,10 @@ id: exhaustive-deps title: Exhaustive dependencies for query keys --- -Query keys should be seen like a dependency array to your query function: Every variable that is used inside the queryFn should be added to the query key. -This makes sure that queries are cached independently and that queries are refetched automatically when the variables changes. +Query keys should contain the serializable values that identify the data returned by your queryFn. +This makes sure that queries are cached independently and that queries are refetched automatically when those values change. + +Function call targets are not query key dependencies. For example, `fetchTodoById(todoId)` needs `todoId` in the query key, but not `fetchTodoById`. Values referenced inside nested callbacks are still dependencies, so `promise.then(() => todoId)` also needs `todoId` in the query key. ## Rule Details @@ -29,7 +31,7 @@ Examples of **correct** code for this rule: const Component = ({ todoId }) => { const todos = useTodos() useQuery({ - queryKey: ['todo', todos, todoId], + queryKey: ['todo', todoId], queryFn: () => todos.getTodo(todoId), }) } @@ -46,24 +48,12 @@ const todoQueries = { ``` ```tsx -// with { allowlist: { variables: ["todos"] }} -const Component = ({ todoId }) => { - const todos = useTodos() - useQuery({ - queryKey: ['todo', todoId], - queryFn: () => todos.getTodo(todoId), - }) -} -``` - -```tsx -// with { allowlist: { types: ["TodosClient"] }} -class TodosClient { ... } -const Component = ({ todoId }) => { - const todos: TodosClient = new TodosClient() +// with { allowlist: { types: ["Config"] }} +class Config { ... } +const Component = ({ todoId, config }: { todoId: string, config: Config }) => { useQuery({ queryKey: ['todo', todoId], - queryFn: () => todos.getTodo(todoId), + queryFn: () => fetchTodo(todoId, config.baseUrl), }) } ``` diff --git a/packages/eslint-plugin-query/src/__tests__/ast-utils.test.ts b/packages/eslint-plugin-query/src/__tests__/ast-utils.test.ts new file mode 100644 index 00000000000..63f4fdbd47e --- /dev/null +++ b/packages/eslint-plugin-query/src/__tests__/ast-utils.test.ts @@ -0,0 +1,53 @@ +import { AST_NODE_TYPES } from '@typescript-eslint/utils' +import { describe, expect, it } from 'vitest' +import { ExhaustiveDepsUtils } from '../rules/exhaustive-deps/exhaustive-deps.utils' +import { ASTUtils } from '../utils/ast-utils' +import type { TSESLint, TSESTree } from '@typescript-eslint/utils' + +function createIdentifier(name: string): TSESTree.Identifier { + return { type: AST_NODE_TYPES.Identifier, name } as TSESTree.Identifier +} + +describe('ASTUtils', () => { + it('stops member traversal when a node has no parent', () => { + const identifier = createIdentifier('value') + + expect(ASTUtils.traverseUpMemberExpression(identifier)).toBe(identifier) + }) + + it('handles an external reference without a parent', () => { + const operation = createIdentifier('operation') + const reference = { + identifier: operation, + isRead: () => true, + resolved: null, + } as TSESLint.Scope.Reference + const scope = { + childScopes: [], + references: [reference], + set: new Map(), + } as unknown as TSESLint.Scope.Scope + const scopeManager = { + acquire: () => scope, + } as unknown as TSESLint.Scope.ScopeManager + const sourceCode = { + getText: () => 'operation', + } as unknown as Readonly + + expect( + ASTUtils.getExternalRefs({ + scopeManager, + sourceCode, + node: operation, + }), + ).toEqual([reference]) + }) +}) + +describe('ExhaustiveDepsUtils', () => { + it('does not treat a detached identifier as a function call target', () => { + expect( + ExhaustiveDepsUtils.isFunctionCallTarget(createIdentifier('fetchTodos')), + ).toBe(false) + }) +}) diff --git a/packages/eslint-plugin-query/src/__tests__/exhaustive-deps.test.ts b/packages/eslint-plugin-query/src/__tests__/exhaustive-deps.test.ts index 7e67ae85404..23f3492395c 100644 --- a/packages/eslint-plugin-query/src/__tests__/exhaustive-deps.test.ts +++ b/packages/eslint-plugin-query/src/__tests__/exhaustive-deps.test.ts @@ -48,6 +48,43 @@ ruleTester.run('exhaustive-deps', rule, { } `, }, + { + name: 'should not require a component scoped function call target in queryKey', + code: normalizeIndent` + function Component({ todoId }) { + const fetchTodoById = (id) => Promise.resolve(id) + + return useQuery({ + queryKey: ['todos', todoId], + queryFn: () => fetchTodoById(todoId), + }) + } + `, + }, + { + name: 'should not require a method call receiver in queryKey', + code: normalizeIndent` + function Component({ todoId }) { + const todos = useTodos() + + return useQuery({ + queryKey: ['todo', todoId], + queryFn: () => todos.getTodo(todoId), + }) + } + `, + }, + { + name: 'should not require a data method receiver in queryKey', + code: normalizeIndent` + function Component({ items }) { + useQuery({ + queryKey: ['items'], + queryFn: () => items?.map((item) => item.id), + }) + } + `, + }, { name: 'should pass props.src', code: ` @@ -776,6 +813,28 @@ ruleTester.run('exhaustive-deps', rule, { } `, }, + { + name: 'should pass when optional chaining method call receiver is omitted', + code: normalizeIndent` + function useThing(a) { + return useQuery({ + queryKey: ['thing'], + queryFn: () => a?.foo() + }) + } + `, + }, + { + name: 'should pass when non-null assertion method call receiver is omitted', + code: normalizeIndent` + function useThing(a) { + return useQuery({ + queryKey: ['thing'], + queryFn: () => a!.foo() + }) + } + `, + }, { name: 'should pass when queryKey uses TSAsExpression with array', code: normalizeIndent` @@ -881,80 +940,47 @@ ruleTester.run('exhaustive-deps', rule, { `, }, { - name: 'should pass when queryFn is ternary with both branches having deps in queryKey', + name: 'should pass when sibling member method call receivers are omitted', code: normalizeIndent` - function useThing(condition, a, b) { + function useThing(a) { return useQuery({ - queryKey: ['thing', a, b], - queryFn: condition ? () => fetchA(a) : () => fetchB(b) + queryKey: ['thing'], + queryFn: () => { + a.b.foo() + a.c.bar() + return 1 + } }) } `, }, - ], - invalid: [ { - name: 'should fail when optional chaining method call is missing root', + name: 'should pass when nested member method call receiver is omitted', code: normalizeIndent` function useThing(a) { return useQuery({ queryKey: ['thing'], - queryFn: () => a?.foo() + queryFn: () => { + a.b.foo() + return 1 + } }) } `, - errors: [ - { - messageId: 'missingDeps', - data: { deps: 'a' }, - suggestions: [ - { - messageId: 'fixTo', - output: normalizeIndent` - function useThing(a) { - return useQuery({ - queryKey: ['thing', a], - queryFn: () => a?.foo() - }) - } - `, - }, - ], - }, - ], }, { - name: 'should fail when non-null assertion method call is missing root', + name: 'should pass when queryFn is ternary with both branches having deps in queryKey', code: normalizeIndent` - function useThing(a) { + function useThing(condition, a, b) { return useQuery({ - queryKey: ['thing'], - queryFn: () => a!.foo() + queryKey: ['thing', a, b], + queryFn: condition ? () => fetchA(a) : () => fetchB(b) }) } `, - errors: [ - { - messageId: 'missingDeps', - data: { deps: 'a' }, - suggestions: [ - { - messageId: 'fixTo', - output: normalizeIndent` - function useThing(a) { - return useQuery({ - queryKey: ['thing', a], - queryFn: () => a!.foo() - }) - } - `, - }, - ], - }, - ], }, { - name: 'should fail when alias of props used in queryFn is missing in queryKey', + name: 'should not require a nested method call receiver in queryKey', code: normalizeIndent` function Component(props) { const entities = props.entities; @@ -969,26 +995,32 @@ ruleTester.run('exhaustive-deps', rule, { }); } `, + }, + ], + invalid: [ + { + name: 'should fail when a computed method name is missing in queryKey', + code: normalizeIndent` + function Component({ client, operation }) { + useQuery({ + queryKey: ['data'], + queryFn: () => client[operation](), + }) + } + `, errors: [ { messageId: 'missingDeps', - data: { deps: 'entities' }, + data: { deps: 'operation' }, suggestions: [ { messageId: 'fixTo', - data: { result: "['get-stuff', entities]" }, output: normalizeIndent` - function Component(props) { - const entities = props.entities; - - const q = useQuery({ - queryKey: ['get-stuff', entities], - queryFn: () => { - return api.fetchStuff({ - ids: entities.map((o) => o.id) - }); - } - }); + function Component({ client, operation }) { + useQuery({ + queryKey: ['data', operation], + queryFn: () => client[operation](), + }) } `, }, @@ -1631,80 +1663,6 @@ ruleTester.run('exhaustive-deps', rule, { }, ], }, - { - name: 'should fail when sibling member method calls missing one path', - code: normalizeIndent` - function useThing(a) { - return useQuery({ - queryKey: ['thing', a.b], - queryFn: () => { - a.b.foo() - a.c.bar() - return 1 - } - }) - } - `, - errors: [ - { - messageId: 'missingDeps', - data: { deps: 'a.c' }, - suggestions: [ - { - messageId: 'fixTo', - output: normalizeIndent` - function useThing(a) { - return useQuery({ - queryKey: ['thing', a.b, a.c], - queryFn: () => { - a.b.foo() - a.c.bar() - return 1 - } - }) - } - `, - }, - ], - }, - ], - }, - { - name: 'should fail when single member method call missing path and root', - code: normalizeIndent` - function useThing(a) { - return useQuery({ - queryKey: ['thing'], - queryFn: () => { - a.b.foo() - return 1 - } - }) - } - `, - errors: [ - { - messageId: 'missingDeps', - data: { deps: 'a.b' }, - suggestions: [ - { - messageId: 'fixTo', - output: normalizeIndent` - function useThing(a) { - return useQuery({ - queryKey: ['thing', a.b], - queryFn: () => { - a.b.foo() - return 1 - } - }) - } - `, - }, - ], - }, - ], - }, { name: 'should fail when queryKey has TSAsExpression with missing dep', code: normalizeIndent` @@ -1771,27 +1729,27 @@ ruleTester.run('exhaustive-deps', rule, { name: 'should fail when type allowlist is empty', options: [{ allowlist: { types: [] } }], code: normalizeIndent` - interface Api { fetch: () => void } + interface Api { baseUrl: string } function useThing(api: Api) { return useQuery({ queryKey: ['thing'], - queryFn: () => api.fetch() + queryFn: () => api.baseUrl }) } `, errors: [ { messageId: 'missingDeps', - data: { deps: 'api' }, + data: { deps: 'api.baseUrl' }, suggestions: [ { messageId: 'fixTo', output: normalizeIndent` - interface Api { fetch: () => void } + interface Api { baseUrl: string } function useThing(api: Api) { return useQuery({ - queryKey: ['thing', api], - queryFn: () => api.fetch() + queryKey: ['thing', api.baseUrl], + queryFn: () => api.baseUrl }) } `, @@ -1929,13 +1887,12 @@ ruleTester.run('exhaustive-deps allowlist.types', rule, { name: 'should ignore missing member path when root type is in allowlist.types', options: [{ allowlist: { types: ['Svc'] } }], code: normalizeIndent` - interface Svc { part: { load: (id: string) => void } } + interface Svc { part: { baseUrl: string } } function useThing(svc: Svc, id: string) { return useQuery({ queryKey: ['thing', id], queryFn: () => { - svc.part.load(id) - return id + return { baseUrl: svc.part.baseUrl, id } } }) } @@ -2007,13 +1964,12 @@ ruleTester.run('exhaustive-deps allowlist.types', rule, { name: 'should report missing member path when root type not in allowlist.types', options: [{ allowlist: { types: ['Other'] } }], code: normalizeIndent` - interface Svc { part: { load: (id: string) => void } } + interface Svc { part: { baseUrl: string } } function useThing(svc: Svc, id: string) { return useQuery({ queryKey: ['thing', id], queryFn: () => { - svc.part.load(id) - return id + return { baseUrl: svc.part.baseUrl, id } } }) } @@ -2021,18 +1977,17 @@ ruleTester.run('exhaustive-deps allowlist.types', rule, { errors: [ { messageId: 'missingDeps', - data: { deps: 'svc.part' }, + data: { deps: 'svc.part.baseUrl' }, suggestions: [ { messageId: 'fixTo', output: normalizeIndent` - interface Svc { part: { load: (id: string) => void } } + interface Svc { part: { baseUrl: string } } function useThing(svc: Svc, id: string) { return useQuery({ - queryKey: ['thing', id, svc.part], + queryKey: ['thing', id, svc.part.baseUrl], queryFn: () => { - svc.part.load(id) - return id + return { baseUrl: svc.part.baseUrl, id } } }) } @@ -2046,13 +2001,12 @@ ruleTester.run('exhaustive-deps allowlist.types', rule, { name: 'should report missing member path when variable has type annotation but type not allowlisted', options: [{ allowlist: { types: ['AllowedService'] } }], code: normalizeIndent` - interface MyService { method: () => void } + interface MyService { baseUrl: string } function useData(service: MyService) { return useQuery({ queryKey: ['data'], queryFn: () => { - service.method() - return 'data' + return service.baseUrl } }) } @@ -2060,18 +2014,17 @@ ruleTester.run('exhaustive-deps allowlist.types', rule, { errors: [ { messageId: 'missingDeps', - data: { deps: 'service' }, + data: { deps: 'service.baseUrl' }, suggestions: [ { messageId: 'fixTo', output: normalizeIndent` - interface MyService { method: () => void } + interface MyService { baseUrl: string } function useData(service: MyService) { return useQuery({ - queryKey: ['data', service], + queryKey: ['data', service.baseUrl], queryFn: () => { - service.method() - return 'data' + return service.baseUrl } }) } @@ -2085,20 +2038,19 @@ ruleTester.run('exhaustive-deps allowlist.types', rule, { name: 'should not inherit allowlisted type from outer shadowed binding', options: [{ allowlist: { types: ['AllowedService'] } }], code: normalizeIndent` - interface AllowedService { load: () => void } - interface OtherService { load: () => void } + interface AllowedService { baseUrl: string } + interface OtherService { baseUrl: string } function useThing() { - const svc: AllowedService = { load: () => undefined } + const svc: AllowedService = { baseUrl: 'allowed' } if (true) { - const svc: OtherService = { load: () => undefined } + const svc: OtherService = { baseUrl: 'other' } return useQuery({ queryKey: ['thing'], queryFn: () => { - svc.load() - return 'data' + return svc.baseUrl } }) } @@ -2109,25 +2061,24 @@ ruleTester.run('exhaustive-deps allowlist.types', rule, { errors: [ { messageId: 'missingDeps', - data: { deps: 'svc' }, + data: { deps: 'svc.baseUrl' }, suggestions: [ { messageId: 'fixTo', output: normalizeIndent` - interface AllowedService { load: () => void } - interface OtherService { load: () => void } + interface AllowedService { baseUrl: string } + interface OtherService { baseUrl: string } function useThing() { - const svc: AllowedService = { load: () => undefined } + const svc: AllowedService = { baseUrl: 'allowed' } if (true) { - const svc: OtherService = { load: () => undefined } + const svc: OtherService = { baseUrl: 'other' } return useQuery({ - queryKey: ['thing', svc], + queryKey: ['thing', svc.baseUrl], queryFn: () => { - svc.load() - return 'data' + return svc.baseUrl } }) } @@ -2153,8 +2104,7 @@ ruleTester.run('exhaustive-deps allowlist.variables', rule, { return useQuery({ queryKey: ['thing', id], queryFn: () => { - svc.part.load(id) - return id + return { part: svc.part, id } } }) } @@ -2184,9 +2134,7 @@ ruleTester.run('exhaustive-deps allowlist.variables', rule, { return useQuery({ queryKey: ['thing'], queryFn: () => { - svc.part.load() - other.x.run() - return 1 + return { svcPart: svc.part, otherX: other.x } } }) } @@ -2203,9 +2151,7 @@ ruleTester.run('exhaustive-deps allowlist.variables', rule, { return useQuery({ queryKey: ['thing', other.x], queryFn: () => { - svc.part.load() - other.x.run() - return 1 + return { svcPart: svc.part, otherX: other.x } } }) } @@ -2222,8 +2168,7 @@ ruleTester.run('exhaustive-deps allowlist.variables', rule, { return useQuery({ queryKey: ['thing', id], queryFn: () => { - svc.part.load(id) - return id + return { part: svc.part, id } } }) } @@ -2240,8 +2185,7 @@ ruleTester.run('exhaustive-deps allowlist.variables', rule, { return useQuery({ queryKey: ['thing', id, svc.part], queryFn: () => { - svc.part.load(id) - return id + return { part: svc.part, id } } }) } diff --git a/packages/eslint-plugin-query/src/rules/exhaustive-deps/exhaustive-deps.utils.ts b/packages/eslint-plugin-query/src/rules/exhaustive-deps/exhaustive-deps.utils.ts index 135a5b620d1..4f467aa4548 100644 --- a/packages/eslint-plugin-query/src/rules/exhaustive-deps/exhaustive-deps.utils.ts +++ b/packages/eslint-plugin-query/src/rules/exhaustive-deps/exhaustive-deps.utils.ts @@ -55,11 +55,24 @@ export const ExhaustiveDepsUtils = { return ( reference.identifier.name !== 'undefined' && + !ExhaustiveDepsUtils.isFunctionCallTarget(reference.identifier) && reference.identifier.parent.type !== AST_NODE_TYPES.NewExpression && !ExhaustiveDepsUtils.isInstanceOfKind(reference.identifier.parent) ) }, + isFunctionCallTarget( + identifier: TSESTree.Identifier | TSESTree.JSXIdentifier, + ): boolean { + const callee = ASTUtils.traverseUpMemberExpression(identifier) + + return ( + callee.parent !== undefined && + callee.parent.type === AST_NODE_TYPES.CallExpression && + callee.parent.callee === callee + ) + }, + /** * Given required refs and existing queryKey entries, compute missing dependency paths * respecting allowlisted variables and types. @@ -296,11 +309,7 @@ export const ExhaustiveDepsUtils = { }): { path: string; root: string; coversRootMembers: boolean } | null { const { identifier, sourceCode } = params - const fullChainNode = ASTUtils.traverseUpOnly(identifier, [ - AST_NODE_TYPES.MemberExpression, - AST_NODE_TYPES.TSNonNullExpression, - AST_NODE_TYPES.Identifier, - ]) + const fullChainNode = ASTUtils.traverseUpMemberExpression(identifier) const fullText = ExhaustiveDepsUtils.normalizeChain( sourceCode.getText(fullChainNode), diff --git a/packages/eslint-plugin-query/src/utils/ast-utils.ts b/packages/eslint-plugin-query/src/utils/ast-utils.ts index a44ae864549..b73825432e6 100644 --- a/packages/eslint-plugin-query/src/utils/ast-utils.ts +++ b/packages/eslint-plugin-query/src/utils/ast-utils.ts @@ -143,6 +143,21 @@ export const ASTUtils = { return identifier }, + traverseUpMemberExpression(node: TSESTree.Node): TSESTree.Node { + const parent = node.parent + + if ( + parent !== undefined && + ((parent.type === AST_NODE_TYPES.MemberExpression && + parent.object === node) || + parent.type === AST_NODE_TYPES.ChainExpression || + parent.type === AST_NODE_TYPES.TSNonNullExpression) + ) { + return ASTUtils.traverseUpMemberExpression(parent) + } + + return node + }, isDeclaredInNode(params: { functionNode: TSESTree.Node reference: TSESLint.Scope.Reference @@ -184,10 +199,22 @@ export const ASTUtils = { const references = collectReferences(scope) .filter((x) => x.isRead() && !scope.set.has(x.identifier.name)) .map((x) => { - const referenceNode = ASTUtils.traverseUpOnly(x.identifier, [ - AST_NODE_TYPES.MemberExpression, - AST_NODE_TYPES.Identifier, - ]) + const memberPath = ASTUtils.traverseUpMemberExpression(x.identifier) + const memberExpression = memberPath.parent + const isComputedCallProperty = + memberExpression !== undefined && + memberExpression.type === AST_NODE_TYPES.MemberExpression && + memberExpression.computed && + memberExpression.property === memberPath && + memberExpression.parent.type === AST_NODE_TYPES.CallExpression && + memberExpression.parent.callee === memberExpression + + const referenceNode = isComputedCallProperty + ? memberPath + : ASTUtils.traverseUpOnly(x.identifier, [ + AST_NODE_TYPES.MemberExpression, + AST_NODE_TYPES.Identifier, + ]) return { variable: x,