From f65ebe70cb86e5ea484fc916cf4a7e71bb435f1b Mon Sep 17 00:00:00 2001 From: Manuel Schiller Date: Tue, 20 Aug 2024 20:51:35 +0200 Subject: [PATCH] feat(eslint-plugin-query): add rule that disallows putting the result of query hooks directly in a React hook dependency array (#7911) * feat(eslint-plugin-query): add rule that disallows putting the result of useMutation directly in a React hook dependency array * add doc * add rule to recommended rules * generalize to all affected query hooks * rename rule to "no-unstable-deps" --- docs/config.json | 4 + docs/eslint/eslint-plugin-query.md | 1 + docs/eslint/no-unstable-deps.md | 56 ++++++++ .../src/__tests__/no-unstable-deps.test.ts | 132 ++++++++++++++++++ packages/eslint-plugin-query/src/index.ts | 2 + packages/eslint-plugin-query/src/rules.ts | 2 + .../no-unstable-deps/no-unstable-deps.rule.ts | 128 +++++++++++++++++ 7 files changed, 325 insertions(+) create mode 100644 docs/eslint/no-unstable-deps.md create mode 100644 packages/eslint-plugin-query/src/__tests__/no-unstable-deps.test.ts create mode 100644 packages/eslint-plugin-query/src/rules/no-unstable-deps/no-unstable-deps.rule.ts diff --git a/docs/config.json b/docs/config.json index 24239e85e6..89eb47fd2a 100644 --- a/docs/config.json +++ b/docs/config.json @@ -792,6 +792,10 @@ { "label": "No Rest Destructuring", "to": "eslint/no-rest-destructuring" + }, + { + "label": "No Unstable Deps", + "to": "eslint/no-unstable-deps" } ] }, diff --git a/docs/eslint/eslint-plugin-query.md b/docs/eslint/eslint-plugin-query.md index 4cad83beb9..d8f6839c94 100644 --- a/docs/eslint/eslint-plugin-query.md +++ b/docs/eslint/eslint-plugin-query.md @@ -96,3 +96,4 @@ Alternatively, add `@tanstack/eslint-plugin-query` to the plugins section, and c - [@tanstack/query/exhaustive-deps](../exhaustive-deps) - [@tanstack/query/no-rest-destructuring](../no-rest-destructuring) - [@tanstack/query/stable-query-client](../stable-query-client) +- [@tanstack/query/no-unstable-deps](../no-unstable-deps.md) diff --git a/docs/eslint/no-unstable-deps.md b/docs/eslint/no-unstable-deps.md new file mode 100644 index 0000000000..529f82def6 --- /dev/null +++ b/docs/eslint/no-unstable-deps.md @@ -0,0 +1,56 @@ +--- +id: no-unstable-deps +title: Disallow putting the result of query hooks directly in a React hook dependency array +--- + +The object returned from the following query hooks is **not** referentially stable: + +- `useQuery` +- `useSuspenseQuery` +- `useQueries` +- `useSuspenseQueries` +- `useInfiniteQuery` +- `useSuspenseInfiniteQuery` +- `useMutation` + +The object returned from those hooks should **not** be put directly into the dependency array of a React hook (e.g. `useEffect`, `useMemo`, `useCallback`). +Instead, destructure the return value of the query hook and pass the destructured values into the dependency array of the React hook. + +## Rule Details + +Examples of **incorrect** code for this rule: + +```tsx +/* eslint "@tanstack/query/no-unstable-deps": "warn" */ +import { useCallback } from 'React' +import { useMutation } from '@tanstack/react-query' + +function Component() { + const mutation = useMutation({ mutationFn: (value: string) => value }) + const callback = useCallback(() => { + mutation.mutate('hello') + }, [mutation]) + return null +} +``` + +Examples of **correct** code for this rule: + +```tsx +/* eslint "@tanstack/query/no-unstable-deps": "warn" */ +import { useCallback } from 'React' +import { useMutation } from '@tanstack/react-query' + +function Component() { + const { mutate } = useMutation({ mutationFn: (value: string) => value }) + const callback = useCallback(() => { + mutate('hello') + }, [mutate]) + return null +} +``` + +## Attributes + +- [x] ✅ Recommended +- [ ] 🔧 Fixable diff --git a/packages/eslint-plugin-query/src/__tests__/no-unstable-deps.test.ts b/packages/eslint-plugin-query/src/__tests__/no-unstable-deps.test.ts new file mode 100644 index 0000000000..31b0fc8ef7 --- /dev/null +++ b/packages/eslint-plugin-query/src/__tests__/no-unstable-deps.test.ts @@ -0,0 +1,132 @@ +import { RuleTester } from '@typescript-eslint/rule-tester' +import { + reactHookNames, + rule, + useQueryHookNames, +} from '../rules/no-unstable-deps/no-unstable-deps.rule' + +const ruleTester = new RuleTester({ + parser: '@typescript-eslint/parser', + settings: {}, +}) + +interface TestCase { + reactHookImport: string + reactHookInvocation: string + reactHookAlias: string +} +const baseTestCases = { + valid: ({ reactHookImport, reactHookInvocation, reactHookAlias }: TestCase) => + [ + { + name: `should pass when destructured mutate is passed to ${reactHookAlias} as dependency`, + code: ` + ${reactHookImport} + import { useMutation } from "@tanstack/react-query"; + + function Component() { + const { mutate } = useMutation({ mutationFn: (value: string) => value }); + const callback = ${reactHookInvocation}(() => { mutate('hello') }, [mutate]); + return; + } + `, + }, + ].concat( + useQueryHookNames.map((queryHook) => ({ + name: `should pass result of ${queryHook} is passed to ${reactHookInvocation} as dependency`, + code: ` + ${reactHookImport} + import { ${queryHook} } from "@tanstack/react-query"; + + function Component() { + const { refetch } = ${queryHook}({ queryFn: (value: string) => value }); + const callback = ${reactHookInvocation}(() => { query.refetch() }, [refetch]); + return; + } + `, + })), + ), + invalid: ({ + reactHookImport, + reactHookInvocation, + reactHookAlias, + }: TestCase) => + [ + { + name: `result of useMutation is passed to ${reactHookInvocation} as dependency `, + code: ` + ${reactHookImport} + import { useMutation } from "@tanstack/react-query"; + + function Component() { + const mutation = useMutation({ mutationFn: (value: string) => value }); + const callback = ${reactHookInvocation}(() => { mutation.mutate('hello') }, [mutation]); + return; + } + `, + errors: [ + { + messageId: 'noUnstableDeps', + data: { reactHook: reactHookAlias, queryHook: 'useMutation' }, + }, + ], + }, + ].concat( + useQueryHookNames.map((queryHook) => ({ + name: `result of ${queryHook} is passed to ${reactHookInvocation} as dependency`, + code: ` + ${reactHookImport} + import { ${queryHook} } from "@tanstack/react-query"; + + function Component() { + const query = ${queryHook}({ queryFn: (value: string) => value }); + const callback = ${reactHookInvocation}(() => { query.refetch() }, [query]); + return; + } + `, + errors: [ + { + messageId: 'noUnstableDeps', + data: { reactHook: reactHookAlias, queryHook }, + }, + ], + })), + ), +} + +const testCases = (reactHookName: string) => [ + { + reactHookImport: 'import * as React from "React";', + reactHookInvocation: `React.${reactHookName}`, + reactHookAlias: reactHookName, + }, + { + reactHookImport: `import { ${reactHookName} } from "React";`, + reactHookInvocation: reactHookName, + reactHookAlias: reactHookName, + }, + { + reactHookImport: `import { ${reactHookName} as useAlias } from "React";`, + reactHookInvocation: 'useAlias', + reactHookAlias: 'useAlias', + }, +] + +reactHookNames.forEach((reactHookName) => { + testCases(reactHookName).forEach( + ({ reactHookInvocation, reactHookAlias, reactHookImport }) => { + ruleTester.run('no-unstable-deps', rule, { + valid: baseTestCases.valid({ + reactHookImport, + reactHookInvocation, + reactHookAlias, + }), + invalid: baseTestCases.invalid({ + reactHookImport, + reactHookInvocation, + reactHookAlias, + }), + }) + }, + ) +}) diff --git a/packages/eslint-plugin-query/src/index.ts b/packages/eslint-plugin-query/src/index.ts index 595e44ea2d..f70ccf7a88 100644 --- a/packages/eslint-plugin-query/src/index.ts +++ b/packages/eslint-plugin-query/src/index.ts @@ -28,6 +28,7 @@ Object.assign(plugin.configs, { '@tanstack/query/exhaustive-deps': 'error', '@tanstack/query/no-rest-destructuring': 'warn', '@tanstack/query/stable-query-client': 'error', + '@tanstack/query/no-unstable-deps': 'error', }, }, 'flat/recommended': [ @@ -39,6 +40,7 @@ Object.assign(plugin.configs, { '@tanstack/query/exhaustive-deps': 'error', '@tanstack/query/no-rest-destructuring': 'warn', '@tanstack/query/stable-query-client': 'error', + '@tanstack/query/no-unstable-deps': 'error', }, }, ], diff --git a/packages/eslint-plugin-query/src/rules.ts b/packages/eslint-plugin-query/src/rules.ts index fe7ac8c215..9bc18ae9ea 100644 --- a/packages/eslint-plugin-query/src/rules.ts +++ b/packages/eslint-plugin-query/src/rules.ts @@ -1,6 +1,7 @@ import * as exhaustiveDeps from './rules/exhaustive-deps/exhaustive-deps.rule' import * as stableQueryClient from './rules/stable-query-client/stable-query-client.rule' import * as noRestDestructuring from './rules/no-rest-destructuring/no-rest-destructuring.rule' +import * as noUnstableDeps from './rules/no-unstable-deps/no-unstable-deps.rule' import type { ESLintUtils } from '@typescript-eslint/utils' import type { ExtraRuleDocs } from './types' @@ -16,4 +17,5 @@ export const rules: Record< [exhaustiveDeps.name]: exhaustiveDeps.rule, [stableQueryClient.name]: stableQueryClient.rule, [noRestDestructuring.name]: noRestDestructuring.rule, + [noUnstableDeps.name]: noUnstableDeps.rule, } diff --git a/packages/eslint-plugin-query/src/rules/no-unstable-deps/no-unstable-deps.rule.ts b/packages/eslint-plugin-query/src/rules/no-unstable-deps/no-unstable-deps.rule.ts new file mode 100644 index 0000000000..d82738d158 --- /dev/null +++ b/packages/eslint-plugin-query/src/rules/no-unstable-deps/no-unstable-deps.rule.ts @@ -0,0 +1,128 @@ +import { AST_NODE_TYPES, ESLintUtils } from '@typescript-eslint/utils' +import { getDocsUrl } from '../../utils/get-docs-url' +import { detectTanstackQueryImports } from '../../utils/detect-react-query-imports' +import type { TSESTree } from '@typescript-eslint/utils' +import type { ExtraRuleDocs } from '../../types' + +export const name = 'no-unstable-deps' + +export const reactHookNames = ['useEffect', 'useCallback', 'useMemo'] +export const useQueryHookNames = [ + 'useQuery', + 'useSuspenseQuery', + 'useQueries', + 'useSuspenseQueries', + 'useInfiniteQuery', + 'useSuspenseInfiniteQuery', +] +const allHookNames = ['useMutation', ...useQueryHookNames] +const createRule = ESLintUtils.RuleCreator(getDocsUrl) + +export const rule = createRule({ + name, + meta: { + type: 'problem', + docs: { + description: + 'Disallow putting the result of useMutation directly in a React hook dependency array', + recommended: 'error', + }, + messages: { + noUnstableDeps: `The result of {{queryHook}} is not referentially stable, so don't pass it directly into the dependencies array of {{reactHook}}. Instead, destructure the return value of {{queryHook}} and pass the destructured values into the dependency array of {{reactHook}}.`, + }, + schema: [], + }, + defaultOptions: [], + + create: detectTanstackQueryImports((context) => { + const trackedVariables: Record = {} + const hookAliasMap: Record = {} + + function getReactHook(node: TSESTree.CallExpression): string | undefined { + if (node.callee.type === 'Identifier') { + const calleeName = node.callee.name + // Check if the identifier is a known React hook or an alias + if (reactHookNames.includes(calleeName) || calleeName in hookAliasMap) { + return calleeName + } + } else if ( + node.callee.type === 'MemberExpression' && + node.callee.object.type === 'Identifier' && + node.callee.object.name === 'React' && + node.callee.property.type === 'Identifier' && + reactHookNames.includes(node.callee.property.name) + ) { + // Member expression case: `React.useCallback` + return node.callee.property.name + } + return undefined + } + + function collectVariableNames( + pattern: TSESTree.BindingName, + queryHook: string, + ) { + if (pattern.type === AST_NODE_TYPES.Identifier) { + trackedVariables[pattern.name] = queryHook + } + } + + return { + ImportDeclaration(node: TSESTree.ImportDeclaration) { + if ( + node.specifiers.length > 0 && + node.importKind === 'value' && + node.source.value === 'React' + ) { + node.specifiers.forEach((specifier) => { + if ( + specifier.type === AST_NODE_TYPES.ImportSpecifier && + reactHookNames.includes(specifier.imported.name) + ) { + // Track alias or direct import + hookAliasMap[specifier.local.name] = specifier.imported.name + } + }) + } + }, + + VariableDeclarator(node) { + if ( + node.init !== null && + node.init.type === AST_NODE_TYPES.CallExpression && + node.init.callee.type === AST_NODE_TYPES.Identifier && + allHookNames.includes(node.init.callee.name) + ) { + collectVariableNames(node.id, node.init.callee.name) + } + }, + CallExpression: (node) => { + const reactHook = getReactHook(node) + if ( + reactHook !== undefined && + node.arguments.length > 1 && + node.arguments[1]?.type === AST_NODE_TYPES.ArrayExpression + ) { + const depsArray = node.arguments[1].elements + depsArray.forEach((dep) => { + if ( + dep !== null && + dep.type === AST_NODE_TYPES.Identifier && + trackedVariables[dep.name] !== undefined + ) { + const queryHook = trackedVariables[dep.name] + context.report({ + node: dep, + messageId: 'noUnstableDeps', + data: { + queryHook, + reactHook, + }, + }) + } + }) + } + }, + } + }), +})