Skip to content

Commit 2b9b122

Browse files
braden-wTkDodo
andauthored
test: add callback return type tests for mutation callbacks (#9252)
* 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]>
1 parent 4335993 commit 2b9b122

File tree

1 file changed

+219
-2
lines changed

1 file changed

+219
-2
lines changed

packages/query-core/src/__tests__/mutations.test.tsx

Lines changed: 219 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
1-
import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest'
21
import { queryKey, sleep } from '@tanstack/query-test-utils'
3-
import { MutationObserver } from '../mutationObserver'
2+
import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest'
43
import { QueryClient } from '..'
4+
import { MutationObserver } from '../mutationObserver'
55
import { executeMutation } from './utils'
66
import type { MutationState } from '../mutation'
77

@@ -613,4 +613,221 @@ describe('mutations', () => {
613613
'finish-B2',
614614
])
615615
})
616+
617+
describe('callback return types', () => {
618+
test('should handle all sync callback patterns', async () => {
619+
const key = queryKey()
620+
const results: Array<string> = []
621+
622+
await executeMutation(
623+
queryClient,
624+
{
625+
mutationKey: key,
626+
mutationFn: () => Promise.resolve('success'),
627+
onMutate: () => {
628+
results.push('onMutate-sync')
629+
return { backup: 'data' } // onMutate can return context
630+
},
631+
onSuccess: () => {
632+
results.push('onSuccess-implicit-void')
633+
// Implicit void return
634+
},
635+
onError: () => {
636+
results.push('onError-explicit-void')
637+
return // Explicit void return
638+
},
639+
onSettled: () => {
640+
results.push('onSettled-return-value')
641+
return 'ignored-value' // Non-void return (should be ignored)
642+
},
643+
},
644+
'vars',
645+
)
646+
647+
expect(results).toEqual([
648+
'onMutate-sync',
649+
'onSuccess-implicit-void',
650+
'onSettled-return-value',
651+
])
652+
})
653+
654+
test('should handle all async callback patterns', async () => {
655+
const key = queryKey()
656+
const results: Array<string> = []
657+
658+
executeMutation(
659+
queryClient,
660+
{
661+
mutationKey: key,
662+
mutationFn: () => Promise.resolve('success'),
663+
onMutate: async () => {
664+
results.push('onMutate-async')
665+
await sleep(1)
666+
return { backup: 'async-data' }
667+
},
668+
onSuccess: async () => {
669+
results.push('onSuccess-async-start')
670+
await sleep(2)
671+
results.push('onSuccess-async-end')
672+
// Implicit void return from async
673+
},
674+
onSettled: () => {
675+
results.push('onSettled-promise')
676+
return Promise.resolve('also-ignored') // Promise<string> (should be ignored)
677+
},
678+
},
679+
'vars',
680+
)
681+
682+
await vi.runAllTimersAsync()
683+
684+
expect(results).toEqual([
685+
'onMutate-async',
686+
'onSuccess-async-start',
687+
'onSuccess-async-end',
688+
'onSettled-promise',
689+
])
690+
})
691+
692+
test('should handle Promise.all() and Promise.allSettled() patterns', async () => {
693+
const key = queryKey()
694+
const results: Array<string> = []
695+
696+
executeMutation(
697+
queryClient,
698+
{
699+
mutationKey: key,
700+
mutationFn: () => Promise.resolve('success'),
701+
onSuccess: () => {
702+
results.push('onSuccess-start')
703+
return Promise.all([
704+
sleep(2).then(() => results.push('invalidate-queries')),
705+
sleep(1).then(() => results.push('track-analytics')),
706+
])
707+
},
708+
onSettled: () => {
709+
results.push('onSettled-start')
710+
return Promise.allSettled([
711+
sleep(1).then(() => results.push('cleanup-1')),
712+
Promise.reject('error').catch(() =>
713+
results.push('cleanup-2-failed'),
714+
),
715+
])
716+
},
717+
},
718+
'vars',
719+
)
720+
721+
await vi.runAllTimersAsync()
722+
723+
expect(results).toEqual([
724+
'onSuccess-start',
725+
'track-analytics',
726+
'invalidate-queries',
727+
'onSettled-start',
728+
'cleanup-2-failed',
729+
'cleanup-1',
730+
])
731+
})
732+
733+
test('should handle mixed sync/async patterns and return value isolation', async () => {
734+
const key = queryKey()
735+
const results: Array<string> = []
736+
737+
const mutationPromise = executeMutation(
738+
queryClient,
739+
{
740+
mutationKey: key,
741+
mutationFn: () => Promise.resolve('actual-result'),
742+
onMutate: () => {
743+
results.push('sync-onMutate')
744+
return { rollback: 'data' }
745+
},
746+
onSuccess: async () => {
747+
results.push('async-onSuccess')
748+
await sleep(1)
749+
return 'success-return-ignored'
750+
},
751+
onError: () => {
752+
results.push('sync-onError')
753+
return Promise.resolve('error-return-ignored')
754+
},
755+
onSettled: (_data, _error, _variables, context) => {
756+
results.push(`settled-context-${context?.rollback}`)
757+
return Promise.all([
758+
Promise.resolve('cleanup-1'),
759+
Promise.resolve('cleanup-2'),
760+
])
761+
},
762+
},
763+
'vars',
764+
)
765+
766+
await vi.runAllTimersAsync()
767+
768+
const mutationResult = await mutationPromise
769+
770+
// Verify mutation returns its own result, not callback returns
771+
expect(mutationResult).toBe('actual-result')
772+
console.log(results)
773+
expect(results).toEqual([
774+
'sync-onMutate',
775+
'async-onSuccess',
776+
'settled-context-data',
777+
])
778+
})
779+
780+
test('should handle error cases with all callback patterns', async () => {
781+
const key = queryKey()
782+
const results: Array<string> = []
783+
784+
const newMutationError = new Error('mutation-error')
785+
let mutationError: Error | undefined
786+
executeMutation(
787+
queryClient,
788+
{
789+
mutationKey: key,
790+
mutationFn: () => Promise.reject(newMutationError),
791+
onMutate: () => {
792+
results.push('onMutate')
793+
return { backup: 'error-data' }
794+
},
795+
onSuccess: () => {
796+
results.push('onSuccess-should-not-run')
797+
},
798+
onError: async () => {
799+
results.push('onError-async')
800+
await sleep(1)
801+
// Test Promise.all() in error callback
802+
return Promise.all([
803+
sleep(1).then(() => results.push('error-cleanup-1')),
804+
sleep(2).then(() => results.push('error-cleanup-2')),
805+
])
806+
},
807+
onSettled: (_data, _error, _variables, context) => {
808+
results.push(`settled-error-${context?.backup}`)
809+
return Promise.allSettled([
810+
Promise.resolve('settled-cleanup'),
811+
Promise.reject('settled-error'),
812+
])
813+
},
814+
},
815+
'vars',
816+
).catch((error) => {
817+
mutationError = error
818+
})
819+
820+
await vi.runAllTimersAsync()
821+
822+
expect(results).toEqual([
823+
'onMutate',
824+
'onError-async',
825+
'error-cleanup-1',
826+
'error-cleanup-2',
827+
'settled-error-error-data',
828+
])
829+
830+
expect(mutationError).toEqual(newMutationError)
831+
})
832+
})
616833
})

0 commit comments

Comments
 (0)