fix(query): unable to infer mutation context when onMutate has args#200
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request updates the handling of mutation options and test context in both the React and Vue query packages. In the test suites, the Changes
Possibly related PRs
Poem
✨ Finishing Touches
🪧 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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
More templates
@orpc/client
@orpc/contract
@orpc/openapi-client
@orpc/react-query
@orpc/openapi
@orpc/server
@orpc/shared
@orpc/standard-server
@orpc/standard-server-fetch
@orpc/standard-server-node
@orpc/vue-colada
@orpc/vue-query
@orpc/zod
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/vue-query/src/procedure-utils.ts (1)
1-67: 💡 Verification agent🧩 Analysis chain
Consider adding tests for the fixed mutation context inference.
The PR title indicates this is fixing an issue with mutation context inference when
onMutatehas arguments. To ensure this fix is robust and doesn't regress in the future, consider adding specific tests that verify the type inference works correctly in this scenario.
🏁 Script executed:
#!/bin/bash # Check if there are any tests or types for MutationOptionsRest echo "Checking for MutationOptionsRest definition:" rg -A 5 "MutationOptionsRest" --glob "**/*.ts" | grep -v "procedure-utils.ts" echo "Checking for any related type tests:" rg -A 3 -B 3 "type test" --glob "**/*.ts" | grep -E "Mutation|onMutate"Length of output: 588
Add Comprehensive Type Tests for Mutation Context Inference
The fix for mutation context inference when
onMutatehas arguments looks solid, but our search indicates that there are no type tests verifying this behavior. To prevent regressions, please add tests that explicitly confirm the type inference works as expected for mutation options, especially whenonMutatereceives arguments. Consider testing both overload paths ofmutationOptionsto ensure that the inferred types are correct.
- Test Scenarios:
- Validate that providing arguments in
onMutatecorrectly infers the mutation context.- Confirm that the overload handling in
mutationOptionsproduces the expected types in various use cases.
🧹 Nitpick comments (1)
packages/vue-query/src/procedure-utils.ts (1)
58-64: Ensure consistent implementation across packages.The implementation of
mutationOptionsis correctly kept identical between the React and Vue packages. It handles both signature overloads properly through the use of rest parameters with a default value.I recommend adding a comment to document the purpose of this implementation pattern, given the changes made to support better type inference.
mutationOptions(...[{ context, ...rest } = {}]) { + // Implementation handles both overload signatures: + // 1. With rest parameters (for backward compatibility) + // 2. With direct options object (for better type inference when onMutate has args) return { mutationKey: buildKey(path, { type: 'mutation' }), mutationFn: input => client(input, { context: unrefDeep(context) as any }), ...(rest as any), } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/react-query/src/procedure-utils.test-d.ts(1 hunks)packages/react-query/src/procedure-utils.ts(2 hunks)packages/react-query/src/types.ts(1 hunks)packages/vue-query/src/procedure-utils.test-d.ts(1 hunks)packages/vue-query/src/procedure-utils.ts(2 hunks)packages/vue-query/src/types.ts(1 hunks)
🔇 Additional comments (11)
packages/vue-query/src/types.ts (1)
63-63: Good addition of type utility for handling conditional rest parametersThis new
MutationOptionsRest<T>type utility elegantly handles the conditional parameter passing pattern, resolving to an empty tuple when the options object is empty, and a single-element tuple otherwise.This is a good pattern for handling optional parameters in TypeScript function signatures, particularly for mutation options which may or may not require additional configuration.
packages/react-query/src/types.ts (1)
37-37: Good addition of type utility for handling conditional rest parametersThis new
MutationOptionsRest<T>type utility elegantly handles the conditional parameter passing pattern, resolving to an empty tuple when the options object is empty, and a single-element tuple otherwise.The utility ensures consistent typing across both React and Vue implementations, providing a solid foundation for handling mutation options in a type-safe manner.
packages/vue-query/src/procedure-utils.test-d.ts (1)
280-280: UpdatedonMutateto accept mutation variables parameterThe function signature now accepts a parameter that can be used to access mutation variables, aligning with the PR objective to fix inference of mutation context when
onMutatehas args.This change enables proper type inference for mutation context when the
onMutatecallback has parameters, addressing the issue mentioned in the PR title.packages/react-query/src/procedure-utils.test-d.ts (1)
293-293: UpdatedonMutateto accept mutation variables parameterThe function signature now accepts a parameter that can be used to access mutation variables, aligning with the PR objective to fix inference of mutation context when
onMutatehas args.This change enables proper type inference for mutation context when the
onMutatecallback has parameters, addressing the issue mentioned in the PR title. The consistency between React and Vue implementations is maintained.packages/react-query/src/procedure-utils.ts (4)
4-4: Updated import properly includes the new MutationOptionsRest type.The import statement has been correctly updated to include the new
MutationOptionsResttype that will be used in the method signatures.
20-22: Improved type inference for mutation options.Replacing
MaybeOptionalOptionswithMutationOptionsRestshould help fix the issue where mutation context wasn't being properly inferred whenonMutatehas arguments. This change maintains backward compatibility while enhancing type safety.
24-26: Added explicit signature overload for better type inference.Adding this overload signature is a good approach that allows the API to be more flexible while providing proper type inference. This explicit signature will make it clearer to consumers how to properly type their mutation options.
54-60:Details
✅ Verification successful
Verify implementation compatibility with the new signatures.
The implementation of
mutationOptionshasn't changed, but it should work with both the updated rest parameter signature and the new direct options signature. Since TypeScript function overloads share a single implementation, the current implementation using rest parameters with default value should correctly handle both cases.
🏁 Script executed:
#!/bin/bash # Check if there are any tests that validate the new mutation options behavior echo "Checking for tests related to mutation options:" rg -A 3 -B 3 "mutationOptions" --glob "**/*.test.ts" --glob "**/*.spec.ts" echo "Checking for tests that might be testing onMutate with arguments:" rg -A 3 -B 3 "onMutate" --glob "**/*.test.ts" --glob "**/*.spec.ts"Length of output: 3298
Action Required: Confirming Mutation Options Signature Compatibility
The tests in multiple packages (react-query, vue-query, and vue-colada) confirm that the current implementation of
mutationOptions, which uses a rest parameter with a default object, correctly supports both the legacy rest parameter signature and the new direct options signature. There were no discrepancies found in the tests verifying that passing an options object (with or without thecontextproperty) results in the expected behavior.
- The tests validate that calling
mutationOptions()without parameters and with an options object (e.g.,{ context: { batch: '__batch__' } }) behaves as intended.- The implementation using destructuring defaults in the rest parameter slot is fully compatible with TypeScript function overloads, ensuring both usage patterns are handled.
No modifications are necessary at this time.
packages/vue-query/src/procedure-utils.ts (3)
4-4: Updated import properly includes the new MutationOptionsRest type.The import statement has been correctly updated to include the new
MutationOptionsResttype that will be used in the method signatures.
22-24: Improved type inference for mutation options.Replacing
MaybeOptionalOptionswithMutationOptionsRestshould help fix the issue where mutation context wasn't being properly inferred whenonMutatehas arguments. This change maintains backward compatibility while enhancing type safety.
26-28: Added explicit signature overload for better type inference.Adding this overload signature is a good approach that allows the API to be more flexible while providing proper type inference. This explicit signature will make it clearer to consumers how to properly type their mutation options.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/server/src/procedure-decorated.ts (1)
127-127: Consider a more type-safe alternative toas any.While the type assertion works as a fix, using
as anybypasses TypeScript's type checking which can hide potential type errors. Consider if there's a more type-safe way to handle this case, potentially by creating a more specific type assertion or refining the type definitions.If a more type-safe alternative isn't practical at this time, please add a code comment explaining why the
as anycast is necessary here to help future maintainers.packages/vue-query/src/procedure-utils.test.ts (1)
64-64: Type assertion may be avoidable in test case.The change from
options.mutationFn!('__input__')to(options as any).mutationFn('__input__')works around type constraints, but may not be necessary in a test context. Consider if the test could be updated to work with the new type structure directly, rather than using a type assertion.Since this is a test file, the type assertion has less impact on production code safety, but maintaining type safety in tests is still a good practice when possible.
packages/vue-query/src/procedure-utils.ts (4)
34-34: Consider simplifying the parameter destructuring pattern.The current implementation uses an unusual destructuring pattern with rest parameters that could be simplified.
-queryOptions(...[options = {} as any]) { +queryOptions(options = {} as any) {
45-45: Consider reducing the number of type assertions in this line.The line uses multiple
as anytype assertions which bypass TypeScript's type checking. This could potentially hide type errors at compile time.-return buildKey(path, { type: 'infinite', input: unrefDeep(options.input(unrefDeep(options.initialPageParam) as any) as any) }) +return buildKey(path, { type: 'infinite', input: unrefDeep(options.input(unrefDeep(options.initialPageParam))) })If type issues persist, consider adding more specific type parameters or interfaces rather than using
any.
48-48: Multiple type assertions could be improved with better typing.Similar to the previous comment, this line also uses multiple
as anytype assertions. Consider refining the types to avoid these assertions.-return client(unrefDeep(options.input(pageParam as any)) as any, { signal, context: unrefDeep(options.context) as any }) +return client(unrefDeep(options.input(pageParam)), { signal, context: unrefDeep(options.context) })
54-54: Consider simplifying the parameter destructuring pattern in mutationOptions.Similar to the queryOptions method, the destructuring pattern with rest parameters could be simplified for clarity.
-mutationOptions(...[options = {} as any]) { +mutationOptions(options = {} as any) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/react-query/src/procedure-utils.ts(3 hunks)packages/react-query/src/types.ts(1 hunks)packages/server/src/procedure-decorated.ts(1 hunks)packages/shared/src/types.ts(1 hunks)packages/vue-colada/tests/e2e.test-d.ts(1 hunks)packages/vue-query/src/procedure-utils.test.ts(1 hunks)packages/vue-query/src/procedure-utils.ts(3 hunks)packages/vue-query/src/types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/vue-colada/tests/e2e.test-d.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react-query/src/procedure-utils.ts
- packages/vue-query/src/types.ts
🔇 Additional comments (8)
packages/react-query/src/types.ts (2)
27-29: Good use of type structure enhancement.The change effectively uses the new
MutationOptionstype, providing a cleaner separation of concerns. This abstraction makes the code more maintainable and improves readability.
31-31: Well-designed type alias introduction.Creating this type alias is a good abstraction that addresses the issue with mutation context inference. This change allows for better type inference when
onMutatehas arguments, as mentioned in the PR description.packages/shared/src/types.ts (1)
1-3: Excellent type definition improvement.This change to
MaybeOptionalOptionsis a significant improvement over the previous definition. The conditional type approach is more readable and directly expresses the intent: make options optional only when the options type is an empty record.This change directly helps address the issue with mutation context inference mentioned in the PR title.
packages/vue-query/src/procedure-utils.ts (5)
4-4: Added new type imports to support mutation context updates.The import statement has been updated to include
MutationOptionsandMutationOptionsIntypes, which are essential for the mutation context inference changes in this PR.
22-24: Improved mutation options method signature to support context inference.The
mutationOptionsmethod signature has been updated to properly support mutation context by:
- Accepting a generic
UMutationContextparameter- Using the rest parameter pattern with
MaybeOptionalOptions- Returning a properly typed
MutationOptionsthat includes the mutation contextThis change directly addresses the PR objective of fixing the context inference when
onMutatehas arguments.
36-38: Updated queryOptions implementation to use a simplified options object pattern.The implementation now consistently accesses input and context from a single options object, which aligns with the interface changes and makes the code more maintainable.
42-42: Simplified infiniteOptions signature to use a single options parameter.This change aligns with the pattern used in the other methods, creating consistency across the interface.
57-57: Updated mutationFn to use the context from options.This change correctly takes the context from the options object, which is necessary for the mutation context inference fix.
1 participant
Related: https://discord.com/channels/1308966753044398161/1316404696830836816/1346472892547727414
Summary by CodeRabbit
New Features
Refactor
mutationOptions,queryOptions, andinfiniteOptions, streamlining option handling across the interface.