refactor: simplify callback return types to void
#9245
Conversation
…to Promise<void> | void Previously, the `onSuccess`, `onError`, `onMutate`, and `onSettled` callbacks were typed as `() => Promise<unknown> | unknown`. While `unknown` is technically valid, it implies that the return value might be used, assigned, or further processed. However, throughout the codebase, these callbacks are invoked solely for their side effects, and their return values are always ignored. Narrowing the type to `Promise<void> | void` makes this intent explicit, clarifies that any return value will be discarded, and prevents misleading type signatures that suggest otherwise. This commit narrows their types to `() => Promise<void> | void`, which more accurately reflects their intended use.
…n-related documentation This commit refines the documentation for mutation-related callbacks (`onSuccess`, `onError`, `onSettled`, and `onMutate`), changing their return types from `Promise<unknown> | unknown` to `Promise<void> | void`.
… Promise.all() usage
…e<Array<void>> for improved handling in onSuccess and onError
github-actions
bot
added
documentation
|
View your CI Pipeline Execution ↗ for commit 77b0cab.
☁️ Nx Cloud last updated this comment at |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9245 +/- ##
===========================================
+ Coverage 45.33% 59.64% +14.30%
===========================================
Files 207 136 -71
Lines 8271 5509 -2762
Branches 1865 1480 -385
===========================================
- Hits 3750 3286 -464
+ Misses 4080 1925 -2155
+ Partials 441 298 -143 🚀 New features to boost your workflow:
|
|
This results in an ESLint error if I don't know how common this rule is, but it's part of the recommended type-check config. |
|
@rothsandro That's a great point. I was playing around with different return signatures. Unfortunately, even if we wanted to accommodate the eslint error and widened the type to be Consider the query3 example in this code snippet: For some reason, it seems like the Typescript compiler can't accept a function that returns a number, even though it was just fine earlier when return types of callback functions were just Since it seems like your example works under our current eslint config (we don't have any |
|
Isn't just |
|
Good point, using just https://tanstack.com/query/v5/docs/framework/react/guides/mutations#mutation-side-effects The issue with It gets confusing because it seems like we are awaiting In summary, @TkDodo , what's the call here? Seems like we have several options:
|
|
@braden-w I think we should revert back to we don’t await on the observer callbacks because they run after the cache entry has been written, for each observer, so they are fire-and-forget. Returning a promise from the onSuccess callback of the cache keeps the mutation in |
|
I have not been following this closely, but could typing it as |
yeah if it might have to be in any case, some type-level tests for these things would be great so that we don’t regress again 🙏 |
|
What if we use If we consider this case, perhaps the return type should be: void | Promise<void> | Promise<Array<void>> | Promise<Array<PromiseSettledResult<void>>>This type feels a bit more verbose than just Sometimes we want to reuse existing functions that return values, even though we don’t care about the return values in this context. For example: onSuccess: (data) => Promise.all([
updateSomeModel(data), // returns updated model, but we ignore it
logSomeAction(data), // returns true/false, but we ignore it
])In this case, the return type doesn’t match onSuccess: async (data) => {
await Promise.all([
updateSomeModel(data),
logSomeAction(data),
])
}This works fine, but it feels a bit restrictive. I think |
|
How about e.g. this discord thread: https://discord.com/channels/719702312431386674/1379733266604494968 |
|
@TkDodo That would still unfortunately throw an error if we did: const toast = () => {
const id = Math.random()
console.log("Pretend toast")
return id
}
// throws error since onSuccess returns a sync non-void value
onSuccess: () => toast(),But what if we did I also wonder if this might be a genuine use case for
In general, the the semantic difference feels like:
Since we're discarding the return value entirely, So the final options could be:
|
|
Shouldn't the example be: i think I'd be fine with |
|
I think Using While I actually considered writing it like this: onSuccess?:
| ((data: TData, variables: TVariables, context: TContext) => void)
| ((data: TData, variables: TVariables, context: TContext) => Promise<void>)
| ((data: TData, variables: TVariables, context: TContext) => void | Promise<void>)But it might be a bit too verbose. If it’s important to inform that the return value—aside from a On the other hand, if enforcing this at compile time isn’t a high priority, using |
|
@jjmean2 That's a really cool solution! It seems like that would allow us to have the benefits of implying that the return value is discarded and async/sync while still allowing the This would remove previous false positive errors for consumers while still conveying the idea that the value isn’t intended to be used directly.
I don't think this would require too much extra boilerplate or a
@TkDodo I agree that would pass through Also for some reason, At this point, I prefer @jjmean2 's solution, but can understand why we might want to go for |
This reverts commit 4e9a360.
Replace callback return type unions with unions of complete function signatures to improve type safety and clarity. This change addresses the issues raised in TanStack#9241 and TanStack#9245 by: - Preventing mixed sync/async behavior within single callbacks - Maintaining support for Promise.all() patterns
|
Update: Just tried implementing it in this commit, and it works great in these cases without error: // 1. Implicit void return
onSuccess: (data, variables, context) => {
console.log('Success!')
}
// 2. Explicit void return
onSuccess: (data, variables, context) => {
console.log('Success!')
return
}
// 3. Explicit undefined return
onSuccess: (data, variables, context) => {
console.log('Success!')
return undefined
}
// 4. Async function with implicit void
onSuccess: async (data, variables, context) => {
await saveToDatabase(data)
}
// 5. Async function with explicit void return
onSuccess: async (data, variables, context) => {
await saveToDatabase(data)
return
}
// 6. Promise.resolve() pattern
onSuccess: (data, variables, context) => {
return Promise.resolve()
}
// 7. Promise.all() pattern (the original use case!)
onSuccess: (data, variables, context) => {
return Promise.all([
queryClient.invalidateQueries({ queryKey: ['products'] }),
queryClient.invalidateQueries({ queryKey: ['categories'] })
])
}
// 8. Complex async operations
onSuccess: (data, variables, context) => {
return Promise.all([
analytics.track('product_created'),
notifications.show('Success!'),
queryClient.invalidateQueries({ queryKey: ['products'] })
]).then(() => {
console.log('All operations completed')
})
}But from the commit diff, you can see it is quite verbose. Just hovering on a callback function honestly gives worse DX:
And also, technically speaking, the function should be able to return a mix of both onSuccess: (data, variables, context) => {
if (someCondition) {
return // void
} else {
return Promise.resolve() // Promise<void>
}
// Error: Cannot mix sync and async returns in same function
}This example would technically pass since the function matches to So in summary, I think **Update: Just made a PR to revert to |
Adds test coverage for mutation callback return types, inspired by the discussions and [TkDodo's comment](TanStack#9245 (comment) ) in TanStack#9245 and the original Promise.all() edge case identified in TanStack#9202. The original PR TanStack#9202 narrowed callback return types from `Promise<unknown> | unknown` to `Promise<void> | void`, but this broke common patterns like: ```ts onSuccess: (data) => Promise.all([ invalidateQueries(), trackAnalytics(), ]) ``` As noted in [the original discussion](TanStack#9202 (comment)), this `Promise.all()` pattern was a legitimate use case that many users relied on. These tests ensure we support all the callback patterns that users expect: ✅ **Sync patterns**: Implicit void, explicit void, non-void returns ✅ **Async patterns**: Async functions, Promise.resolve(), Promise<T> returns ✅ **Promise.all() patterns**: The original breaking case from TanStack#9202 ✅ **Promise.allSettled() patterns**: Additional parallel operation support ✅ **Mixed patterns**: Different callback types in same mutation ✅ **Error handling**: All patterns work in error scenarios ✅ **Return value isolation**: Callback returns don't affect mutation result
* Revert "refactor(types): narrow onSuccess/onError/onMutate/onSettled callback types to Promise<void> | void (#9202)" This reverts commit 982f6ca. * test: add callback return type tests for mutation callbacks Adds test coverage for mutation callback return types, inspired by the discussions and [TkDodo's comment](#9245 (comment) ) in #9245 and the original Promise.all() edge case identified in #9202. The original PR #9202 narrowed callback return types from `Promise<unknown> | unknown` to `Promise<void> | void`, but this broke common patterns like: ```ts onSuccess: (data) => Promise.all([ invalidateQueries(), trackAnalytics(), ]) ``` As noted in [the original discussion](#9202 (comment)), this `Promise.all()` pattern was a legitimate use case that many users relied on. These tests ensure we support all the callback patterns that users expect: ✅ **Sync patterns**: Implicit void, explicit void, non-void returns ✅ **Async patterns**: Async functions, Promise.resolve(), Promise<T> returns ✅ **Promise.all() patterns**: The original breaking case from #9202 ✅ **Promise.allSettled() patterns**: Additional parallel operation support ✅ **Mixed patterns**: Different callback types in same mutation ✅ **Error handling**: All patterns work in error scenarios ✅ **Return value isolation**: Callback returns don't affect mutation result * refactor(tests): remove unnecessary parameters in mutation tests * test(mutations): fix mutation test ordering with error handling and cleanup verification --------- Co-authored-by: Dominik Dorfmeister <[email protected]>
This PR simplifies the callback return types fromPromise<void> | voidto justvoid. This is a more idiomatic TypeScript approach that maintains the same functionality while being more concise, and addresses user feedback from #9202.PR #9202 narrowed callback types fromPromise<unknown> | unknowntoPromise<void> | voidto better reflect the intent that callback return values are ignored. However, this change broke existing user code that was using a common pattern of returningPromise.all()from callbacks.## ProblemThe change broke code like this:SincePromise.all()returnsPromise<Array<void>>, this pattern was no longer type-compatible.## SolutionIn TypeScript, when a function type is specified asvoid, it can accept both synchronous functions that returnvoidand asynchronous functions that returnPromise<void>. This is because TypeScript's type system is designed to be more permissive withvoidreturn types in function signatures.This PR removesPromise<void>from the union type and turns it to justvoid, allowing the commonPromise.all()pattern while maintaining the intent that individual callback return values are ignored (they're all variations ofvoid).The previous PR (#9241) attempted to handle Promise returns explicitly, which led to more complex code and type definitions. This PR takes a simpler approach by:1. Using TypeScript's built-invoidtype which already handles both sync and async cases2. Removing unnecessary Promise chaining3. Making the code more maintainable and easier to understand## Addresses- Feedback from @Sagie501 in #9202- Feedback from @jgarplind in #9202- CommonPromise.all()usage pattern in mutation callbacksFixes #9202 (user feedback)