Improvements to methods in Tanstack React Query package#6529
Conversation
|
This PR was not deployed automatically as @Nick-Lucas does not have access to the Railway project. In order to get automatic PR deploys, please add @Nick-Lucas to the project inside the project settings page. |
WalkthroughThe changes span multiple files across the tanstack-react-query package and its upgrade module. Key updates include renaming the exported type Changes
Sequence Diagram(s)sequenceDiagram
participant Component as React Component
participant TRPCProvider as TRPCProvider
participant ClientContext as TRPCClientContext
Component->>TRPCProvider: Render within Provider
TRPCProvider->>ClientContext: Provide TRPC Client instance
Component->>ClientContext: Invoke useTRPCClient()
ClientContext-->>Component: Return TRPC client instance
sequenceDiagram
participant Component as React Component
participant Mutation as useMutation
participant QueryClient as React Query Client
Component->>Mutation: Trigger mutation (onMutate)
Mutation->>QueryClient: Cancel queries & apply optimistic update
alt Mutation Fails
Mutation->>QueryClient: onError restores previous state
else Mutation Succeeds
Mutation->>QueryClient: onSettled invalidates queries
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (14)
🔇 Additional comments (1)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
@trpc/client
@trpc/next
@trpc/react-query
@trpc/server
@trpc/tanstack-react-query
@trpc/upgrade
commit: |
|
Looking for this PR! My usage of |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/upgrade/test/transforms.test.ts (3)
20-34: Consider explicit handling of empty transformations.While the current implementation implicitly handles empty transformations using the OR operator (
transformed || source), consider making this more explicit for better error handling and debugging.- const formatted = await formatFile(file, transformed || source); + if (!transformed) { + console.warn(`No transformation applied for ${file}`); + } + const formatted = await formatFile(file, transformed ?? source);
44-96: Consider enhancing error handling and logging.The
executeTestsfunction could benefit from:
- Input validation for the parameters
- Structured logging instead of console.log
async function executeTests( fixturesDir: string, file: string, transform: Transformer, ) { + if (!fixturesDir || !file || !transform) { + throw new Error('Missing required parameters'); + } const fixtureFile = join(fixturesDir, file); + const logger = { + info: (msg: string) => console.log(`[Test] ${msg}`), + error: (msg: string) => console.error(`[Test] ${msg}`), + }; // Rest of the function... - console.log(`Running spec on input for ${name}`); + logger.info(`Running spec on input for ${name}`); // And other console.log statements...
98-126: Consider reducing duplication in test suites.The 'hooks' and 'provider' test suites share similar setup and execution logic. Consider extracting the common pattern into a reusable test suite factory.
+const createTestSuite = (name: string, transform: Transformer) => { + describe(name, () => { + const literal = `./__fixtures__/${name}`; + const fixturesDir = new URL(literal, import.meta.url).pathname; + const ONLY_RUN: string[] = []; + const fixtures = ONLY_RUN.length ? ONLY_RUN : readdirSync(fixturesDir).filter(isFixture); + it.each(fixtures)(`${name} %s`, async (file) => { + await executeTests(fixturesDir, file, transform); + }); + }); +}; + +createTestSuite('hooks', hooksTransform); +createTestSuite('provider', providerTransform);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/upgrade/test/transforms.test.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: E2E-tests (fastify-server)
- GitHub Check: e2e-legacy-node (next-prisma-todomvc, 20.x)
- GitHub Check: E2E-tests (express-server)
- GitHub Check: e2e-legacy-node (next-prisma-todomvc, 18.x)
- GitHub Check: E2E-tests (express-minimal)
- GitHub Check: e2e-legacy-node (next-prisma-websockets-starter, 20.x)
- GitHub Check: E2E-tests (cloudflare-workers)
- GitHub Check: e2e-legacy-node (next-prisma-websockets-starter, 18.x)
- GitHub Check: Release using pkg.pr.new
- GitHub Check: E2E-tests (.test/ssg)
- GitHub Check: e2e-legacy-node (next-prisma-starter, 20.x)
- GitHub Check: Test a monorepo using built declaration files
- GitHub Check: E2E-tests (.test/internal-types-export)
- GitHub Check: e2e-legacy-node (next-prisma-starter, 18.x)
- GitHub Check: E2E-tests (.test/diagnostics-big-router)
- GitHub Check: E2E-tests (Deno) (deno-deploy)
- GitHub Check: E2E-tests (.experimental/next-app-dir)
- GitHub Check: E2E-tests (Bun) (bun, ubuntu-latest)
- GitHub Check: test
🔇 Additional comments (3)
packages/upgrade/test/transforms.test.ts (3)
1-1: LGTM! Type definitions enhance code safety.The new type definitions and imports improve type safety and readability. The
Transformertype alias effectively captures the transform function parameters using the TypeScript utility type.Also applies to: 9-9, 19-19
36-42: LGTM! Clear and effective file filtering.The
isFixturefunction provides a clear and maintainable way to filter test files.
102-102: Revert theONLY_RUNarray to empty before merging.The
ONLY_RUNarray is currently empty, which is correct for production. Ensure it stays empty to run all test fixtures in the test suite.Also applies to: 117-117
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/upgrade/test/__fixtures__/hooks/utilities.snap.tsx (1)
54-70: Consider adding type safety for infinite query data structure.The infinite query data structure could benefit from type definitions to ensure consistency.
Consider adding a type definition:
interface InfiniteQueryData { pageParams: number[]; pages: Array<{ items: Array<{ id: number; title: string }>; nextCursor: number; }>; }Then use it in the setQueryData calls:
queryClient.setQueryData<InfiniteQueryData>( trpc.post.paginate.infiniteQueryKey({ cursor: 1 }), // ... rest of the code );packages/upgrade/src/transforms/hooksToOptions.ts (4)
93-122: Consider enhancing error handling.While the function correctly prevents duplicate declarations, it could benefit from additional error handling for edge cases.
Consider adding error handling for:
- Invalid scope detection
- Failed AST traversal
function ensureUseTRPCCall( path: ASTPath<FunctionDeclaration | ArrowFunctionExpression>, ) { + try { // Check if trpc is already declared in scope const existingTRPC = j(path).find(j.VariableDeclarator, { id: { type: 'Identifier', name: trpcImportName, }, }); if (existingTRPC.size() > 0) { return; } const variableDeclaration = j.variableDeclaration('const', [ j.variableDeclarator( j.identifier(trpcImportName!), j.callExpression(j.identifier('useTRPC'), []), ), ]); if (j.FunctionDeclaration.check(path.node)) { path.node.body.body.unshift(variableDeclaration); dirtyFlag = true; } else if (j.BlockStatement.check(path.node.body)) { path.node.body.body.unshift(variableDeclaration); dirtyFlag = true; + } else { + console.warn('Unexpected node type:', path.node.type); } + } catch (error) { + console.error('Failed to ensure TRPC call:', error); + throw new Error(`Failed to ensure TRPC call: ${error.message}`); + } }
277-328: Consider simplifying the filter selection logic.The current implementation uses nested conditionals which could be simplified for better maintainability.
Consider extracting the filter selection logic into a separate function:
+function determineFilterType( + preferedFilter: string, + callExpr: CallExpression, + isNoArgs: boolean, + isUndefindInputArg: boolean +): string { + if (preferedFilter !== 'DYNAMIC_FILTER') { + return preferedFilter; + } + + if (isNoArgs || isUndefindInputArg) { + return 'pathFilter'; + } + + if (!j.ObjectExpression.check(callExpr.arguments[0])) { + return 'queryFilter'; + } + + const hasCursor = callExpr.arguments[0].properties.find( + (p) => + j.ObjectProperty.check(p) && + j.Identifier.check(p.key) && + p.key.name === 'cursor' + ); + + return hasCursor ? 'infiniteQueryFilter' : 'queryFilter'; +} // Replace the existing logic with: const filterType = determineFilterType( preferedFilter, callExpr, isNoArgs, isUndefindInputArg ); memberExpr.property = j.identifier(filterType);This refactor:
- Improves readability by extracting complex logic
- Makes the code more maintainable
- Makes it easier to test filter selection logic
221-261: Enhance error handling in utility migration.The current error handling relies on console.warn calls, which could be improved for better debugging and error recovery.
Consider implementing structured error handling:
+class UtilMigrationError extends Error { + constructor(message: string, public context: any) { + super(message); + this.name = 'UtilMigrationError'; + } +} +function logMigrationError(error: UtilMigrationError) { + console.warn(`[Migration Error] ${error.message}`, error.context); +} root .find(j.Identifier, { name: oldIdentifier.name }) .forEach((path) => { if (j.MemberExpression.check(path.parent?.parent?.node)) { const callExprPath = findParentOfType<CallExpression>( path.parentPath, j.CallExpression, ); if (!callExprPath) { - console.warn( - `Failed to walk up the tree to find utilMethod call expression, on file: ${file.path}`, - callExprPath, - { start: path.node.loc?.start, end: path.node.loc?.end }, - ); + logMigrationError( + new UtilMigrationError( + `Failed to walk up the tree to find utilMethod call expression`, + { + file: file.path, + callExprPath, + location: { start: path.node.loc?.start, end: path.node.loc?.end }, + } + ) + ); return; }
277-283: Add JSDoc comments for complex logic.The filter selection logic would benefit from detailed documentation explaining the decision flow.
Consider adding comprehensive JSDoc comments:
+/** + * Determines the appropriate filter type based on the following rules: + * 1. If a preferred filter is specified (not DYNAMIC_FILTER), use it + * 2. If no arguments or undefined input, use pathFilter + * 3. If input has a cursor property, use infiniteQueryFilter + * 4. Otherwise, use queryFilter + * + * @example + * // Using pathFilter + * utils.post.invalidate() // no args + * utils.post.invalidate(undefined) // undefined input + * + * // Using infiniteQueryFilter + * utils.post.invalidate({ cursor: 'abc' }) + * + * // Using queryFilter + * utils.post.invalidate({ where: { id: 1 } }) + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/upgrade/src/bin/index.ts(1 hunks)packages/upgrade/src/lib/ast/scanners.ts(2 hunks)packages/upgrade/src/lib/git.ts(2 hunks)packages/upgrade/src/lib/pkgmgr.ts(3 hunks)packages/upgrade/src/transforms/hooksToOptions.ts(7 hunks)packages/upgrade/test/__fixtures__/hooks/utilities.snap.tsx(1 hunks)packages/upgrade/test/__fixtures__/hooks/utilities.trpc.tsx(1 hunks)packages/upgrade/test/__fixtures__/hooks/utilities.tsx(1 hunks)packages/upgrade/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/upgrade/src/lib/git.ts
- packages/upgrade/src/bin/index.ts
- packages/upgrade/src/lib/pkgmgr.ts
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: E2E-tests (minimal-react)
- GitHub Check: e2e-legacy-node (next-prisma-todomvc, 20.x)
- GitHub Check: E2E-tests (fastify-server)
- GitHub Check: e2e-legacy-node (next-prisma-todomvc, 18.x)
- GitHub Check: E2E-tests (express-server)
- GitHub Check: e2e-legacy-node (next-prisma-websockets-starter, 20.x)
- GitHub Check: E2E-tests (express-minimal)
- GitHub Check: e2e-legacy-node (next-prisma-websockets-starter, 18.x)
- GitHub Check: E2E-tests (cloudflare-workers)
- GitHub Check: e2e-legacy-node (next-prisma-starter, 20.x)
- GitHub Check: E2E-tests (.test/ssg)
- GitHub Check: Release using pkg.pr.new
- GitHub Check: e2e-legacy-node (next-prisma-starter, 18.x)
- GitHub Check: E2E-tests (.test/internal-types-export)
- GitHub Check: Test a monorepo using built declaration files
- GitHub Check: E2E-tests (.test/diagnostics-big-router)
- GitHub Check: E2E-tests (.experimental/next-app-dir)
- GitHub Check: E2E-tests (Deno) (deno-deploy)
- GitHub Check: E2E-tests (Bun) (bun, ubuntu-latest)
🔇 Additional comments (24)
packages/upgrade/src/lib/ast/scanners.ts (2)
13-13: LGTM! Consistent environment variable access.The change to bracket notation aligns with the standardization of environment variable access across the codebase.
117-117: LGTM! Improved counting logic using nullish coalescing.The change from
||to??operator is a meaningful improvement. The nullish coalescing operator only defaults to 0 whencounts[specifier]isnullorundefined, preserving valid count values including 0, which makes the counting logic more accurate.packages/upgrade/test/__fixtures__/hooks/utilities.trpc.tsx (2)
13-19: Consider edge cases in pagination logic.The pagination logic might not handle all edge cases correctly:
- What happens when
cursoris not found (returns -1)?- What if
nextCursorgoes beyond array bounds?Consider adding input validation and bounds checking:
paginate: t.procedure .input(z.object({ cursor: z.number() })) .query(({ input }) => { const cursor = posts.findIndex((p) => p.id === input.cursor); + if (cursor === -1) { + throw new Error(`Post with id ${input.cursor} not found`); + } const nextCursor = cursor + 1; + const hasNextPage = nextCursor < posts.length; - return { items: posts.slice(cursor, nextCursor), nextCursor }; + return { + items: posts.slice(cursor, nextCursor), + nextCursor: hasNextPage ? nextCursor : undefined + }; }),
20-22: Consider adding error handling for byId procedure.The
findoperation might returnundefinedwhen no post matches the ID.Consider explicit error handling:
byId: t.procedure.input(z.object({ id: z.number() })).query(({ input }) => { - return posts.find((p) => p.id === input.id); + const post = posts.find((p) => p.id === input.id); + if (!post) { + throw new Error(`Post with id ${input.id} not found`); + } + return post; }),packages/upgrade/test/__fixtures__/hooks/utilities.snap.tsx (2)
8-9: Inconsistent use of filter method names.The code alternates between using
pathFilterandqueryFilter. According to the PR objectives, this is being standardized.Standardize to use
pathFilterconsistently:- queryClient.cancelQueries(trpc.post.list.pathFilter()); - queryClient.cancelQueries(trpc.post.byId.queryFilter({ id: 1 })); + queryClient.cancelQueries(trpc.post.list.pathFilter()); + queryClient.cancelQueries(trpc.post.byId.pathFilter({ id: 1 })); // ... similar changes for refetchQueries, resetQueries, and invalidateQueriesAlso applies to: 30-31, 33-34, 36-37
14-16: Inconsistent use of key method names.The code alternates between using
queryKeyandinfiniteQueryKey. According to the PR objectives, this is being standardized.Standardize to use consistent key naming:
- queryClient.getQueryData(trpc.post.list.queryKey()); - queryClient.getQueryData(trpc.post.paginate.infiniteQueryKey()); - queryClient.getQueryData(trpc.post.byId.queryKey({ id: 1 })); + queryClient.getQueryData(trpc.post.list.pathKey()); + queryClient.getQueryData(trpc.post.paginate.infinitePathKey()); + queryClient.getQueryData(trpc.post.byId.pathKey({ id: 1 }));packages/upgrade/tsconfig.json (7)
2-2: Extends Base Configuration
The addition of"extends": "../../tsconfig.json"correctly leverages a shared base configuration for consistency across the repository.
3-3: Expanded Include Files
Updating the"include"array to["**/*.ts", "**/*.tsx"]is a positive change that enables the TypeScript compiler to pick up both regular TS files and TSX files (used in React projects). Please ensure that including all TSX files (possibly including tests) is intentional.
5-5: Verify Module Option Value
The"module": "Preserve"setting is unconventional since module options are typically specified in lowercase (e.g.,"preserve") or use standard values such as"commonjs","esnext", etc.Please verify that this value is supported by your intended TypeScript version and that it does not cause unexpected behavior in your build pipeline.
6-6: Enable JSON Module Imports
Adding"resolveJsonModule": trueis a beneficial change that allows importing JSON files as modules. This is useful for projects that need to integrate JSON data directly.
10-10: Updated Library Target
The"lib": ["ESNext"]configuration ensures that the latest ECMAScript features are available during compilation. If your project eventually depends on specific environments (like DOM APIs), consider if additional libraries should be added—but for now, this looks good.
11-11: Preserve JSX Syntax
The"jsx": "preserve"option is appropriate for libraries or projects that delegate JSX transformation to another tool (like Babel). This change aligns well with React projects where JSX is managed downstream.
12-12: Include Jest DOM Types
Adding"types": ["@testing-library/jest-dom"]improves type support for test assertions when using Jest with the Testing Library, which enhances the testing experience.packages/upgrade/src/transforms/hooksToOptions.ts (1)
39-56: LGTM! Well-structured utility mapping.The updated
utilMapstructure effectively separates method names from filter types and adds support for infinite query operations. The tuple-based approach provides better type safety and flexibility.packages/upgrade/test/__fixtures__/hooks/utilities.tsx (10)
1-4: LGTM! Clean and straightforward setup.The component follows React best practices and correctly imports and initializes the TRPC utilities.
6-7: LGTM! Comprehensive cancel operation testing.The code effectively tests both parameterized and non-parameterized query cancellation.
16-19: LGTM! Well-structured fetch operations.The code demonstrates proper implementation of both regular and cursor-based infinite fetch operations.
20-23: LGTM! Comprehensive prefetch implementation.The code effectively tests prefetching for all query types, including cursor-based infinite queries.
24-29: LGTM! Clear refetch and reset operations.The code properly implements refetch and reset functionality for both list and individual queries.
33-44: LGTM! Robust data update implementation.The code demonstrates proper error handling and follows React's immutable update pattern for both direct values and update functions.
45-61: LGTM! Comprehensive infinite data update implementation.The code effectively demonstrates cursor-based infinite data updates with proper handling of undefined states.
63-64: LGTM! Appropriate return value for test fixture.The simple return value is suitable for testing purposes.
30-32:Details
✅ Verification successful
LGTM! Route invalidation implementation matches PR objectives.
The code demonstrates the new route invalidation functionality mentioned in the PR objectives.
Let's verify the route invalidation implementation:
🏁 Script executed:
#!/bin/bash # Search for route invalidation implementations rg -A 5 'invalidate|routeKey|routeFilter' --type typescriptLength of output: 96
Manual Verification Needed: Route Invalidation Implementation
The current implementation of route invalidation (using
utils.post.list.invalidate()andutils.post.byId.invalidate({ id: 1 })) appears to meet the PR objectives. However, the original verification command produced an error due to an incorrect file type flag. Please re-run the above script or manually verify that the route invalidation changes propagate correctly throughout the codebase.
🏁 Script executed:
[run_scripts] #!/bin/bash # Re-run route invalidation verification across TypeScript and TSX files. rg -A 5 'invalidate|routeKey|routeFilter' -t ts -t tsxLength of output: 206
LGTM! Route Invalidation Functionality Verified
The implementation of route invalidation via
utils.post.list.invalidate();utils.post.byId.invalidate({ id: 1 });in
packages/upgrade/test/__fixtures__/hooks/utilities.tsxappears consistent with the PR objectives. If you haven't already, please verify manually (or via the above script) that these invalidate operations are correctly integrated across the codebase, especially given the previous file type flag issues with ripgrep.
🏁 Script executed:
[run_scripts] #!/bin/bash # Verify invalidate operations in the utilities.tsx fixture using globbing to avoid file type issues. rg -A 3 -B 3 'utils\.post\.list\.invalidate' packages/upgrade/test/__fixtures__/hooks/utilities.tsx rg -A 3 -B 3 'utils\.post\.byId\.invalidate' packages/upgrade/test/__fixtures__/hooks/utilities.tsxLength of output: 706
LGTM! Verified Route Invalidation Functionality Matches PR Objectives
The verification output confirms that both
utils.post.list.invalidate()andutils.post.byId.invalidate({ id: 1 })are present and correctly placed in thepackages/upgrade/test/__fixtures__/hooks/utilities.tsxfile. No further changes are necessary. Please ensure that the invalidate operations are integrated as expected across the codebase.
9-15:Details
✅ Verification successful
LGTM! Comprehensive data retrieval testing.
The code effectively tests all data retrieval scenarios, including the new infinite query functionality mentioned in the PR objectives.
Let's verify the infinite query implementation matches the PR objectives:
🏁 Script executed:
#!/bin/bash # Search for other infinite query implementations to ensure consistency rg -A 5 'getInfiniteData|fetchInfinite|setInfiniteData' --type typescriptLength of output: 110
Approval: Comprehensive Data Retrieval Testing Verified!
The tests fully cover the data retrieval operations by exercising ensureData, getData, and the new infinite query functionality (via getInfiniteData) as described in the PR objectives. A follow-up search in TSX files (which include the file under test) did not reveal inconsistencies. However, please manually verify that the infinite query behavior aligns with the intended updates.
- File:
packages/upgrade/test/__fixtures__/hooks/utilities.tsx(Lines 9-15)- Verification: Re-run search script using TSX file type (see above)
🏁 Script executed:
[run_scripts] #!/bin/bash # Re-run search for infinite query implementations in TSX files to avoid file type issues rg -A 5 'getInfiniteData|fetchInfinite|setInfiniteData' --type tsxLength of output: 218
Verification Needed: Infinite Query Implementations Consistency Check
The infinite query functions (i.e.
getInfiniteData) appear to be tested in the current file under test, but the previous search with file type filtering did not work as expected. The above script usesfdto ensure we’re scanning TSX files for any potential matches. Please run this script and verify that no unexpected infinite query implementations exist elsewhere in the codebase. If the output is empty, it confirms consistency with PR objectives.
🏁 Script executed:
[run_scripts] #!/bin/bash # Use fd to locate TSX files and search for infinite query related implementations fd -e tsx | xargs rg -A 5 'getInfiniteData|fetchInfinite|setInfiniteData'Length of output: 29337
LGTM! Comprehensive Data Retrieval Testing Verified
The infinite query functionality is exercised alongside the other data retrieval operations, and a repository-wide search confirms that implementations like
getInfiniteData, as well as related methods (e.g.,fetchInfinite,prefetchInfinite,setInfiniteData), are used consistently across the codebase and align with the PR objectives. The tests in both the fixtures and the broader suite cover the necessary scenarios for infinite queries.
- File:
packages/upgrade/test/__fixtures__/hooks/utilities.tsx(Lines 9-15)
The snippet correctly invokesutils.post.paginate.getInfiniteData()along with other methods.No further changes are required.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
packages/upgrade/test/transforms.test.ts (2)
20-34: Add error handling for empty transformations.The
snapshotTestTransformfunction should handle cases where the transformation returns an empty result. Consider adding validation before formatting.const transformed = applyTransform( transform, { trpcImportName: 'trpc' }, { source }, {}, ); + if (!transformed) { + throw new Error('Transform returned empty result'); + } - const formatted = await formatFile(file, transformed || source); + const formatted = await formatFile(file, transformed);
44-96: Consider using a proper logger instead of console.log.The test execution logic is well-structured, but consider replacing
console.logwith a proper test logger or reporter for better test output management.- // eslint-disable-next-line no-console - console.log(`Running spec on input for ${name}`); + process.stdout.write(`Running spec on input for ${name}\n`);LGTM! Excellent error handling and validation.
The comprehensive error messages and thorough component validation make debugging test failures much easier.
packages/upgrade/test/__fixtures__/hooks/existingRqMix.tsx (4)
6-7: Add proper type definitions and error handling.The query and mutation hooks lack TypeScript types and error handling. Consider adding:
- Return type definitions
- Error handling for failed queries/mutations
- Loading states management
- useQuery({ queryKey: ['a'], queryFn: () => 1 }); + useQuery<number, Error>({ + queryKey: ['a'], + queryFn: () => 1, + onError: (error) => { + console.error('Query failed:', error); + }, + }); - useMutation({ mutationFn: async () => 1 }); + useMutation<number, Error>({ + mutationFn: async () => 1, + onError: (error) => { + console.error('Mutation failed:', error); + }, + });
5-19: Consider organizing queries for better maintainability.The component mixes different query patterns (standard, infinite, and mutations) without clear organization. Consider grouping related queries and extracting complex logic into custom hooks.
+// Extract into a custom hook +function usePostQueries(postId: number) { + const post = trpc.post.byId.useQuery({ id: postId }); + const num = trpc.num.useQuery(postId); + const posts = trpc.post.list.useInfiniteQuery( + {}, + { + getNextPageParam: (lastPage) => lastPage.nextCursor, + }, + ); + return { post, num, posts }; +} export function Component1() { useQuery<number, Error>({ queryKey: ['a'], queryFn: () => 1, onError: (error) => { console.error('Query failed:', error); }, }); useMutation<number, Error>({ mutationFn: async () => 1, onError: (error) => { console.error('Mutation failed:', error); }, }); - trpc.post.byId.useQuery({ id: 1 }); - trpc.num.useQuery(1); - trpc.post.list.useInfiniteQuery( - {}, - { - getNextPageParam: (lastPage) => lastPage.nextCursor, - }, - ); + const { post, num, posts } = usePostQueries(1); return useQueryClient().isFetching() ? 'loading' : 'loaded'; }
21-28: Enhance type safety and performance.Consider these improvements:
- Add type definitions to the query hook
- Memoize the queryClient instance check
export function Component2() { const queryClient = useQueryClient(); + const isFetching = useMemo(() => queryClient.isFetching(), [queryClient]); - useQuery({ queryKey: ['a'], queryFn: () => 1 }); + useQuery<number, Error>({ + queryKey: ['a'], + queryFn: () => 1, + onError: (error) => { + console.error('Query failed:', error); + }, + }); trpc.post.byId.useQuery({ id: 1 }); - return queryClient.isFetching() ? 'loading' : 'loaded'; + return isFetching ? 'loading' : 'loaded'; }
30-42: Improve effect handling and type safety.The component needs several improvements:
- Add cleanup to the cancellation effect
- Include utils in the effect dependencies
- Add type definitions to the query hook
export function Component3() { const queryClient = useQueryClient(); const utils = trpc.useUtils(); + const isFetching = useMemo(() => queryClient.isFetching(), [queryClient]); - useQuery({ queryKey: ['a'], queryFn: () => 1 }); + useQuery<number, Error>({ + queryKey: ['a'], + queryFn: () => 1, + onError: (error) => { + console.error('Query failed:', error); + }, + }); trpc.post.byId.useQuery({ id: 1 }); useEffect(() => { utils.post.byId.cancel({ id: 1 }); - }, []); + return () => { + // Cleanup any ongoing operations if needed + }; + }, [utils]); - return queryClient.isFetching() ? 'loading' : 'loaded'; + return isFetching ? 'loading' : 'loaded'; }packages/upgrade/src/transforms/hooksToOptions.ts (2)
286-291: Enhance comment block with more details.While the comment block outlines the basic logic, it would be more helpful to include:
- Examples for each case
- Edge cases that are handled
- Why each condition leads to a specific filter type
-/** - * If no input, use pathFilter - * If input is undefined, use pathFilter - * If input has cursor, use infiniteQueryFilter - * Otherwise, use queryFilter - */ +/** + * Filter type determination logic: + * 1. No input or undefined input -> pathFilter + * Example: utils.invalidate() or utils.invalidate(undefined) + * 2. Input with cursor property -> infiniteQueryFilter + * Example: utils.invalidate({ cursor: 'abc' }) + * 3. Other inputs -> queryFilter + * Example: utils.invalidate({ id: 123 }) + * + * Special cases: + * - setData/setInfiniteData: First arg goes to queryKey, second is forwarded + * - DYNAMIC_FILTER methods follow the above logic + */
292-337: Consider extracting complex conditions into helper functions.The filter type determination logic is complex and could be more readable if broken down into helper functions.
+function shouldUsePathFilter(callExpr: CallExpression) { + const isNoArgs = !callExpr.arguments.length; + const isUndefindInputArg = + callExpr.arguments.length > 0 && + j.Identifier.check(callExpr.arguments[0]) && + callExpr.arguments[0].name === 'undefined'; + return isNoArgs || isUndefindInputArg; +} + +function shouldUseInfiniteQueryFilter(arg: any) { + return j.ObjectExpression.check(arg) && + arg.properties.find( + (p) => + j.ObjectProperty.check(p) && + j.Identifier.check(p.key) && + p.key.name === 'cursor' + ); +} + const preferedFilter = utilMap[proxyMethod][1]; -const isNoArgs = !callExpr.arguments.length; -const isUndefindInputArg = - callExpr.arguments.length > 0 && - j.Identifier.check(callExpr.arguments[0]) && - callExpr.arguments[0].name === 'undefined'; let argToForward: any = undefined; if (preferedFilter !== 'DYNAMIC_FILTER') { // ... existing code ... -} else if (isNoArgs || isUndefindInputArg) { +} else if (shouldUsePathFilter(callExpr)) { memberExpr.property = j.identifier('pathFilter'); - if (isUndefindInputArg) { + if (callExpr.arguments.length > 0) { callExpr.arguments.shift(); } } else if (j.ObjectExpression.check(callExpr.arguments[0])) { - if ( - callExpr.arguments[0].properties.find( - (p) => - j.ObjectProperty.check(p) && - j.Identifier.check(p.key) && - p.key.name === 'cursor', - ) - ) { + if (shouldUseInfiniteQueryFilter(callExpr.arguments[0])) { memberExpr.property = j.identifier('infiniteQueryFilter'); } else { memberExpr.property = j.identifier('queryFilter'); } }packages/upgrade/test/__fixtures__/hooks/existingRqMix.snap.tsx (1)
13-13: Remove unused mutation.The mutation is defined but never used in the component.
- useMutation({ mutationFn: async () => 1 });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/upgrade/src/transforms/hooksToOptions.ts(7 hunks)packages/upgrade/test/__fixtures__/hooks/existingRqMix.snap.tsx(1 hunks)packages/upgrade/test/__fixtures__/hooks/existingRqMix.tsx(1 hunks)packages/upgrade/test/transforms.test.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: E2E-tests (vercel-edge-runtime)
- GitHub Check: E2E-tests (tanstack-start)
- GitHub Check: E2E-tests (standalone-server)
- GitHub Check: E2E-tests (soa)
- GitHub Check: E2E-tests (next-prisma-websockets-starter)
- GitHub Check: E2E-tests (next-prisma-todomvc)
- GitHub Check: E2E-tests (next-prisma-starter)
- GitHub Check: E2E-tests (next-minimal-starter)
- GitHub Check: E2E-tests (next-formdata)
- GitHub Check: E2E-tests (minimal-react)
- GitHub Check: E2E-tests (fastify-server)
- GitHub Check: E2E-tests (express-server)
- GitHub Check: E2E-tests (express-minimal)
- GitHub Check: E2E-tests (cloudflare-workers)
- GitHub Check: E2E-tests (.test/ssg)
- GitHub Check: E2E-tests (.test/internal-types-export)
- GitHub Check: E2E-tests (.test/diagnostics-big-router)
- GitHub Check: E2E-tests (.experimental/next-app-dir)
- GitHub Check: test
🔇 Additional comments (11)
packages/upgrade/test/transforms.test.ts (3)
1-1: LGTM! Good use of type safety.The added imports and type definitions improve code maintainability and type safety. The
Transformertype alias effectively leverages TypeScript's type inference capabilities.Also applies to: 9-9, 19-19
36-42: LGTM! Clear and focused filtering logic.The
isFixturefunction effectively filters out non-fixture files with clear, maintainable conditions.
98-118: LGTM! Well-structured test suites.The test suites effectively use parameterized tests and maintain DRY principles by reusing the
executeTestsfunction.packages/upgrade/test/__fixtures__/hooks/existingRqMix.tsx (2)
1-3: LGTM!The imports are well-organized and include all necessary dependencies for React Query integration.
11-16: Infinite query implementation needs pagination parameters.The infinite query is called with minimal configuration. Previous review already highlighted this issue.
packages/upgrade/src/transforms/hooksToOptions.ts (3)
93-107: LGTM! Well-structured helper function.The
isDeclarationInScopefunction is a clean and efficient implementation for checking variable declarations in the current scope.
39-56: LGTM! Well-structured utility method mappings.The updated
utilMapprovides a clear and consistent pattern for method mappings, with good separation between query types and proper handling of dynamic filters.
109-131: LGTM! Clean implementation of utility functions.The changes to
ensureUseTRPCCalland queryClient declaration handling are well-implemented with proper error handling and efficient use of the newisDeclarationInScopehelper.Also applies to: 353-365
packages/upgrade/test/__fixtures__/hooks/existingRqMix.snap.tsx (3)
1-8: LGTM!The imports are well-organized and all are being used within the components.
29-37: LGTM!The component is well-structured and correctly uses the query client.
47-47: Update to use pathFilter.Based on the AI summary,
queryFiltershould be renamed topathFilteras part of the API changes.- queryClient.cancelQueries(trpc.post.byId.queryFilter({ id: 1 })); + queryClient.cancelQueries(trpc.post.byId.pathFilter({ id: 1 }));Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/upgrade/src/transforms/hooksToOptions.ts (2)
290-340: Consider adding early returns to improve readability.The filter selection logic is complex with nested conditions. Consider restructuring with early returns for better readability.
- /** - * If no input, use pathFilter - * If input is undefined, use pathFilter - * If input has cursor, use infiniteQueryFilter - * Otherwise, use queryFilter - */ - const preferedFilter = utilMap[proxyMethod][1]; - const isNoArgs = !callExpr.arguments.length; - const isUndefindInputArg = - callExpr.arguments.length > 0 && - j.Identifier.check(callExpr.arguments[0]) && - callExpr.arguments[0].name === 'undefined'; - - let argToForward: any = undefined; - - if (preferedFilter !== 'DYNAMIC_FILTER') { - if ( - proxyMethod === 'setData' || - proxyMethod === 'setInfiniteData' - ) { - // First arg goes into queryKey(), second is forwarded to the wrapped method - // We can omit the first arg if it's undefined - argToForward = callExpr.arguments[1]; - if (callExpr.arguments[0] && !isUndefindInputArg) { - callExpr.arguments = [callExpr.arguments[0]]; - } else { - callExpr.arguments = []; - } - } - - memberExpr.property = j.identifier(preferedFilter); - } else if (isNoArgs || isUndefindInputArg) { - memberExpr.property = j.identifier('pathFilter'); - - if (isUndefindInputArg) { - callExpr.arguments.shift(); - } - } else if (j.ObjectExpression.check(callExpr.arguments[0])) { - if ( - callExpr.arguments[0].properties.find( - (p) => - j.ObjectProperty.check(p) && - j.Identifier.check(p.key) && - p.key.name === 'cursor', - ) - ) { - memberExpr.property = j.identifier('infiniteQueryFilter'); - } else { - memberExpr.property = j.identifier('queryFilter'); - } - } + /** + * If no input, use pathFilter + * If input is undefined, use pathFilter + * If input has cursor, use infiniteQueryFilter + * Otherwise, use queryFilter + */ + const preferedFilter = utilMap[proxyMethod][1]; + let argToForward: any = undefined; + + // Handle non-dynamic filters + if (preferedFilter !== 'DYNAMIC_FILTER') { + if (proxyMethod === 'setData' || proxyMethod === 'setInfiniteData') { + argToForward = callExpr.arguments[1]; + callExpr.arguments = callExpr.arguments[0] && + !(j.Identifier.check(callExpr.arguments[0]) && callExpr.arguments[0].name === 'undefined') + ? [callExpr.arguments[0]] + : []; + } + memberExpr.property = j.identifier(preferedFilter); + return; + } + + // Handle no arguments or undefined argument + const isNoArgs = !callExpr.arguments.length; + const isUndefindInputArg = + callExpr.arguments.length > 0 && + j.Identifier.check(callExpr.arguments[0]) && + callExpr.arguments[0].name === 'undefined'; + + if (isNoArgs || isUndefindInputArg) { + memberExpr.property = j.identifier('pathFilter'); + if (isUndefindInputArg) { + callExpr.arguments.shift(); + } + return; + } + + // Handle object expressions + if (!j.ObjectExpression.check(callExpr.arguments[0])) { + memberExpr.property = j.identifier('queryFilter'); + return; + } + + const hasCursor = callExpr.arguments[0].properties.find( + (p) => + j.ObjectProperty.check(p) && + j.Identifier.check(p.key) && + p.key.name === 'cursor', + ); + memberExpr.property = j.identifier( + hasCursor ? 'infiniteQueryFilter' : 'queryFilter' + );
303-303: Consider using a more specific type for argToForward.Using
anytype could mask potential type errors. Consider using a more specific type or adding a comment explaining whyanyis necessary.- let argToForward: any = undefined; + // Type needs to be flexible as it can be various query parameters + let argToForward: unknown = undefined;
📜 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 (2)
packages/upgrade/package.json(1 hunks)packages/upgrade/src/transforms/hooksToOptions.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/upgrade/package.json
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: E2E-tests (next-formdata)
- GitHub Check: E2E-tests (fastify-server)
- GitHub Check: E2E-tests (express-server)
- GitHub Check: E2E-tests (express-minimal)
- GitHub Check: e2e-legacy-node (next-prisma-todomvc, 20.x)
- GitHub Check: E2E-tests (cloudflare-workers)
- GitHub Check: e2e-legacy-node (next-prisma-todomvc, 18.x)
- GitHub Check: E2E-tests (.test/ssg)
- GitHub Check: e2e-legacy-node (next-prisma-websockets-starter, 20.x)
- GitHub Check: E2E-tests (.test/internal-types-export)
- GitHub Check: Release using pkg.pr.new
- GitHub Check: e2e-legacy-node (next-prisma-websockets-starter, 18.x)
- GitHub Check: E2E-tests (.test/diagnostics-big-router)
- GitHub Check: Test a monorepo using built declaration files
- GitHub Check: e2e-legacy-node (next-prisma-starter, 20.x)
- GitHub Check: E2E-tests (.experimental/next-app-dir)
- GitHub Check: e2e-legacy-node (next-prisma-starter, 18.x)
- GitHub Check: E2E-tests (Deno) (deno-deploy)
- GitHub Check: E2E-tests (Bun) (bun, ubuntu-latest)
- GitHub Check: test
🔇 Additional comments (3)
packages/upgrade/src/transforms/hooksToOptions.ts (3)
39-60: LGTM! Clear and well-documented mapping structure.The updated
utilMapstructure with method pairs is well-organized and the comment clearly explains the mapping format.
97-111: LGTM! Well-implemented scope checking utility.The
isDeclarationInScopehelper function is a clean implementation that correctly checks for variable declarations in the current scope.
358-368: LGTM! Efficient handling of queryClient declaration.The code correctly handles the case where
queryClientis already declared in scope, avoiding duplicate declarations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/tanstack-react-query/src/internals/createOptionsProxy.ts (1)
201-213: Consider improving type readability.The conditional type expression could be more readable with better formatting.
-export type DecorateProcedure< - TType extends TRPCProcedureType, - TDef extends ResolverDef, -> = TType extends 'query' - ? DecorateQueryProcedure<TDef> & - (TDef['input'] extends OptionalCursorInput - ? DecorateInfiniteQueryProcedure<TDef> - : Record<string, never>) - : TType extends 'mutation' - ? DecorateMutationProcedure<TDef> - : TType extends 'subscription' - ? DecorateSubscriptionProcedure<TDef> - : never; +export type DecorateProcedure< + TType extends TRPCProcedureType, + TDef extends ResolverDef +> = TType extends 'query' + ? DecorateQueryProcedure<TDef> & + (TDef['input'] extends OptionalCursorInput + ? DecorateInfiniteQueryProcedure<TDef> + : Record<string, never>) + : TType extends 'mutation' + ? DecorateMutationProcedure<TDef> + : TType extends 'subscription' + ? DecorateSubscriptionProcedure<TDef> + : never;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/tanstack-react-query/src/internals/createOptionsProxy.ts(12 hunks)www/docs/client/tanstack-react-query/migrating.mdx(1 hunks)
🔇 Additional comments (5)
www/docs/client/tanstack-react-query/migrating.mdx (1)
90-90: LGTM! Documentation update aligns with API changes.The example correctly demonstrates the migration from
queryKeytoqueryFilter, providing clear guidance for users upgrading their codebase.packages/tanstack-react-query/src/internals/createOptionsProxy.ts (4)
47-63: LGTM! Clear and well-documented interface renaming.The renaming from
DecorateQueryKeyabletoDecorateRouterKeyableand its methods (pathKey,pathFilter) better reflects their routing-related functionality. The documentation is thorough and includes relevant links.
90-136: LGTM! Well-structured infinite query interface.The new
DecorateInfiniteQueryProcedureinterface is well-designed with:
- Clear type safety through
TypeHelperextension- Comprehensive documentation for each method
- Proper type definitions for infinite query operations
323-331: LGTM! Clean implementation of path-based methods.The implementation of
pathKeyandpathFilteris clean and follows a consistent pattern:
- Uses
getQueryKeyInternalfor key generation- Properly spreads additional filters
- Maintains type safety
361-369: LGTM! Well-implemented infinite query methods.The infinite query methods (
infiniteQueryKey,infiniteQueryFilter) are implemented consistently with their regular query counterparts, maintaining a clear pattern throughout the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
www/blog/2025-02-17-new-tanstack-react-query-integration.mdx (1)
35-35: Clarify the "React" bullet to improve precision.
The new bullet point effectively emphasizes that the classic integration violates the rules of hooks. However, the phrase "passing around hooks as props" could be phrased more clearly—perhaps noting that the classic integration risks misuse of hooks outside of proper function component contexts, leading to linting and runtime issues. Consider rewording for clarity and precision. For example:- - **React**: Our classic React Query integration actually breaks the rules of hooks; it [cannot be correctly linted](https://github.com/trpc/trpc/issues/2330), and it encourages some patterns which will break under the React Compiler such as passing around hooks as props. In this respect the new client is more idiomatic React + - **React**: Our classic React Query integration violates the rules of hooks, resulting in linting issues and problematic patterns—such as misusing hooks outside of their intended context—which can lead to runtime errors. In contrast, the new client strictly adheres to React’s hook rules for more idiomatic and reliable usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
www/blog/2025-02-17-new-tanstack-react-query-integration.mdx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
…key to begin withwq Closes # ## 🎯 Changes What changes are made in this PR? Is it a feature or a bug fix? <!-- Note: once you create a Pull request, we will automatically fix auto-fixable lint issues in your branch --> ## ✅ Checklist - [ ] I have followed the steps listed in the [Contributing guide](https://github.com/trpc/trpc/blob/main/CONTRIBUTING.md). - [ ] If necessary, I have added documentation related to the changes made. - [ ] I have added or updated the tests related to the changes made.
juliusmarminge
left a comment
There was a problem hiding this comment.
Feel free to revert my last commit
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/tanstack-react-query/src/internals/types.ts (1)
21-25: Consider usingundefinedinstead ofvoidin union types.The static analysis tool correctly flags that using
voidin a union type can be confusing. While functionally similar in this context, usingundefinedis more conventional and semantically clearer.Apply this diff to address the lint warning:
/** * @remark `void` is here due to https://github.com/trpc/trpc/pull/4374 */ type CursorInput = { cursor?: any }; -export type OptionalCursorInput = CursorInput | void; +export type OptionalCursorInput = CursorInput | undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 25-25: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
📜 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 (3)
packages/tanstack-react-query/src/internals/createOptionsProxy.ts(10 hunks)packages/tanstack-react-query/src/internals/types.ts(2 hunks)packages/tanstack-react-query/test/queryKeyable.test.tsx(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/tanstack-react-query/test/queryKeyable.test.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/tanstack-react-query/src/internals/types.ts
[error] 25-25: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: E2E-tests (vercel-edge-runtime)
- GitHub Check: E2E-tests (tanstack-start)
- GitHub Check: E2E-tests (standalone-server)
- GitHub Check: E2E-tests (soa)
- GitHub Check: E2E-tests (next-prisma-websockets-starter)
- GitHub Check: E2E-tests (next-prisma-todomvc)
- GitHub Check: E2E-tests (next-prisma-starter)
- GitHub Check: E2E-tests (next-minimal-starter)
- GitHub Check: E2E-tests (next-formdata)
- GitHub Check: E2E-tests (minimal-react)
- GitHub Check: E2E-tests (fastify-server)
- GitHub Check: E2E-tests (express-server)
- GitHub Check: E2E-tests (express-minimal)
- GitHub Check: E2E-tests (cloudflare-workers)
- GitHub Check: E2E-tests (.test/ssg)
- GitHub Check: E2E-tests (.test/internal-types-export)
- GitHub Check: E2E-tests (.test/diagnostics-big-router)
- GitHub Check: E2E-tests (.experimental/next-app-dir)
- GitHub Check: test
- GitHub Check: Lint and auto-fix
🔇 Additional comments (8)
packages/tanstack-react-query/src/internals/types.ts (2)
7-9: Great addition of theWithRequiredutility type.This utility type is a valuable addition that enforces required properties in object types, enhancing type safety throughout the codebase. It's correctly implemented using mapped types.
30-32: Clean refactoring of theExtractCursorTypetype.This change improves code organization by reusing the
CursorInputtype rather than repeating the same structure, making the code more maintainable.packages/tanstack-react-query/src/internals/createOptionsProxy.ts (6)
48-69: Good renaming fromDecorateQueryKeyabletoDecorateRouterKeyable.The interface rename and method changes (
queryKey→pathKey,queryFilter→pathFilter) align better with their purpose of working with router paths. The documentation updates clearly explain the functionality and provide relevant links to official documentation.
96-151: Well-structuredDecorateInfiniteQueryProcedureinterface.This new interface adds proper support for infinite queries with well-documented methods that align with the PR objectives. The implementation is type-safe and comprehensive, covering query keys, options, and filters specifically for infinite queries.
152-196: Good extension ofDecorateQueryProcedurefromDecorateRouterKeyable.This change correctly establishes the inheritance relationship between query procedures and router keyable functionality, improving the overall type hierarchy and preventing code duplication.
228-231: Smart conditional type for infinite query procedures.The conditional inclusion of
DecorateInfiniteQueryProcedurebased on whether the input extendsOptionalCursorInputelegantly handles the requirement mentioned in the PR objectives of making infinite methods only appear when a cursor is present.
346-354: Simplified implementation of path-related methods.The
pathKeyimplementation now correctly callsgetQueryKeyInternalwith just the path parameter, addressing the PR objective of supportinggetQueryKeyInternalwithout needing second and third parameters for route methods.
386-394: Good implementation of infinite query methods.The new methods for infinite queries are well implemented, properly using the internal utilities and maintaining consistency with the existing query methods. These changes directly address the PR objectives of adding
infiniteQueryKeyandinfiniteQueryFilter.
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/tanstack-react-query/src/internals/createOptionsProxy.ts(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: E2E-tests (vercel-edge-runtime)
- GitHub Check: E2E-tests (tanstack-start)
- GitHub Check: E2E-tests (standalone-server)
- GitHub Check: E2E-tests (soa)
- GitHub Check: E2E-tests (next-prisma-websockets-starter)
- GitHub Check: E2E-tests (next-prisma-todomvc)
- GitHub Check: E2E-tests (next-prisma-starter)
- GitHub Check: E2E-tests (next-minimal-starter)
- GitHub Check: E2E-tests (next-formdata)
- GitHub Check: E2E-tests (minimal-react)
- GitHub Check: E2E-tests (fastify-server)
- GitHub Check: E2E-tests (express-server)
- GitHub Check: E2E-tests (express-minimal)
- GitHub Check: E2E-tests (cloudflare-workers)
- GitHub Check: E2E-tests (.test/ssg)
- GitHub Check: E2E-tests (.test/internal-types-export)
- GitHub Check: E2E-tests (.test/diagnostics-big-router)
- GitHub Check: E2E-tests (.experimental/next-app-dir)
- GitHub Check: test
🔇 Additional comments (6)
packages/tanstack-react-query/src/internals/createOptionsProxy.ts (6)
96-151: Good addition: New interface for infinite query proceduresAdding the
DecorateInfiniteQueryProcedureinterface with dedicated methods for infinite queries improves type safety and makes it clearer when and how to use infinite queries.The JSDoc comments are thorough and include links to relevant documentation, which is excellent for developer experience.
229-231: Fixed lint issue: Using Record<string, never> instead of {}You've correctly replaced
{}withRecord<string, never>as recommended in previous reviews to address the lint warning about using empty object types.
346-354: Functionality improvement: Simplified path key methodsThe implementation of
pathKeynow usesgetQueryKeyInternalwith fewer parameters, making it easier to invalidate all queries for a route without needing input parameters.This aligns with the PR objective to "allow for the complete invalidation of routes and single procedures without requiring input."
386-394: New feature: Added infinite query key and filter methodsThe addition of
infiniteQueryKeyandinfiniteQueryFiltermethods provides dedicated support for infinite queries, making them first-class citizens in the API.This implementation matches the PR objective to "introduce infiniteQueryKey and infiniteQueryFilter."
263-267:Details
❓ Verification inconclusive
Ensure backward compatibility
With the transition from query-based to router/path-based terminology in the API, ensure that sufficient deprecation notices and migration paths are provided for users upgrading from previous versions.
Consider adding temporary backwards compatibility shims if this is a breaking change.
🏁 Script executed:
#!/bin/bash # Check for any migrations or upgrade scripts fd "upgrade|migration" --type f packages/ # Check if there are any re-export patterns that might provide backwards compatibility rg -A 5 -B 5 "export.*DecorateQueryKeyable|export.*type.*=.*DecorateRouterKeyable" packages/Length of output: 138
Backward Compatibility Verification Needed
The transition from query-based to router/path-based terminology is clear, but the automated checks for upgrade or migration scripts and re-export patterns did not reveal the existence of deprecation notices or backward compatibility shims. Please manually verify that:
- Sufficient deprecation warnings and migration guides are provided to support users upgrading from previous versions.
- Temporary backward compatibility shims or re-export patterns are implemented where necessary to bridge the terminology change.
This verification is essential to ensure that users are well-informed and that the migration path is smooth.
241-260:Details
❓ Verification inconclusive
API renaming: DecoratedRouterRecord
Renaming
DecoratedProcedureUtilsRecordtoDecoratedRouterRecordmaintains consistency with the other interface renamings in this PR.Ensure this rename is documented in any migration guides.
🏁 Script executed:
#!/bin/bash # Check if this type is exported and used in other files that might need updating rg "DecoratedProcedureUtilsRecord|DecoratedRouterRecord" packages/ --type tsLength of output: 78
Manual Verification Required: Update Migration Documentation for DecoratedRouterRecord
- The renaming from
DecoratedProcedureUtilsRecordtoDecoratedRouterRecordhas been implemented for API consistency.- The automated grep search in the
packages/directory did not return any additional references. However, please manually verify that:
- Any usage of the old name in migration guides and external documentation is updated.
- The exported type in
packages/tanstack-react-query/src/internals/createOptionsProxy.tsis consistent with upstream dependencies.
| export interface DecorateRouterKeyable { | ||
| /** | ||
| * Calculate the TanStack Query Key for a Route | ||
| * Calculate the TanStack Query Key for any path, could be used to invalidate every procedure beneath this path | ||
| * | ||
| * @see https://tanstack.com/query/latest/docs/framework/react/guides/query-keys | ||
| * @see https://trpc.io/docs/client/tanstack-react-query/usage#queryKey | ||
| */ | ||
| queryKey: () => TRPCQueryKey; | ||
| pathKey: () => TRPCQueryKey; | ||
|
|
||
| /** | ||
| * Calculate a TanStack Query Filter for a Route | ||
| * Calculate a TanStack Query Filter for any path, could be used to manipulate every procedure beneath this path | ||
| * | ||
| * @see https://tanstack.com/query/latest/docs/framework/react/guides/filters | ||
| * @see https://trpc.io/docs/client/tanstack-react-query/usage#queryFilter | ||
| */ | ||
| queryFilter: (input?: undefined, filters?: QueryFilters) => QueryFilters; | ||
| pathFilter: ( | ||
| filters?: QueryFilters<unknown, unknown, unknown, TRPCQueryKey>, | ||
| ) => WithRequired< | ||
| QueryFilters<unknown, unknown, unknown, TRPCQueryKey>, | ||
| 'queryKey' | ||
| >; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
API change: Renamed interface and methods
The interface has been renamed from DecorateQueryKeyable to DecorateRouterKeyable, with methods renamed from queryKey/queryFilter to pathKey/pathFilter. This better represents their purpose for invalidating entire routes rather than specific queries.
Make sure this API change is well-documented in migration guides to help users update their code accordingly.
🏁 Script executed:
#!/bin/bash
# Check if there are upgrade guides or migration docs for these API changes
fd "migration|upgrade" --type f -e md -e mdx packages/Length of output: 56
API Change Notification: Migration Guide Update Required
The updated code in packages/tanstack-react-query/src/internals/createOptionsProxy.ts now renames the interface from DecorateQueryKeyable to DecorateRouterKeyable and changes the method names from queryKey/queryFilter to pathKey/pathFilter for better clarity regarding route invalidation. However, our search for migration or upgrade guides (using a file search for "migration" or "upgrade" within the packages/ directory) returned no results.
Please add or update the migration documentation to detail these API changes so that users understand how to update their code accordingly. Consider including:
- A clear explanation of the API renaming.
- A code diff snippet highlighting the changes.
- Guidance for users on how to adapt their existing code.
|
This pull request has been locked because we are very unlikely to see comments on closed issues. If you think, this PR is still necessary, create a new one with the same branch. Thank you. |
Closes #
🎯 Changes
What changes are made in this PR? Is it a feature or a bug fix?
✅ Checklist
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests