-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
feat(react-query): add mutationOptions #8960
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 15 commits
596896d
ff15e5d
ea54b58
2972edd
08a5026
f3b74c0
a4560d3
6889638
b844dee
e61227d
33d3e9f
fd7b9f9
6ee8c76
299a19f
2622902
9ded37d
48d867b
05f4fc0
b202d6e
167fb8c
2b85c72
df3545a
08769ac
36a8af1
22f5ed2
1661565
d7587ba
7ee1f5a
d682a88
f32a7e0
0729167
8730872
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
--- | ||
id: mutationOptions | ||
title: mutationOptions | ||
--- | ||
|
||
```tsx | ||
mutationOptions({ | ||
mutationFn, | ||
...options, | ||
}) | ||
``` | ||
|
||
**Options** | ||
|
||
You can generally pass everything to `mutationOptions` that you can also pass to [`useMutation`](./useMutation.md). | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
import { describe, expectTypeOf, it } from 'vitest' | ||
import { mutationOptions } from '../mutationOptions' | ||
|
||
describe('mutationOptions', () => { | ||
it('should not allow excess properties', () => { | ||
return mutationOptions({ | ||
mutationFn: () => Promise.resolve(5), | ||
mutationKey: ['key'], | ||
// @ts-expect-error this is a good error, because onMutates does not exist! | ||
onMutates: 1000, | ||
onSuccess: (data) => { | ||
expectTypeOf(data).toEqualTypeOf<number>() | ||
}, | ||
}) | ||
}) | ||
|
||
it('should infer types for callbacks', () => { | ||
return mutationOptions({ | ||
mutationFn: () => Promise.resolve(5), | ||
mutationKey: ['key'], | ||
onSuccess: (data) => { | ||
expectTypeOf(data).toEqualTypeOf<number>() | ||
}, | ||
}) | ||
}) | ||
}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my opinion, we should add more test cases before merging this Pull Request—assuming TkDodo agrees with the proposed interface. I'm not sure whether TkDodo will agree with this interface. const Example = () => {
const mutation = useMutation({
...mutationOptions({
mutationKey: ['key'],
mutationFn: () => Promise.resolve({ field: 'test' })
}),
onSuccess: (data) => expectTypeOf(data).toEqualTypeOf<{ field: string }>()
})
expectTypeOf(mutation).toEqualTypeOf ...
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I added more test cases along with the ones that work with useMutation. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
import { describe, expect, it } from 'vitest' | ||
import { mutationOptions } from '../mutationOptions' | ||
import type { UseMutationOptions } from '../types' | ||
|
||
describe('mutationOptions', () => { | ||
it('should return the object received as a parameter without any modification.', () => { | ||
const object: UseMutationOptions = { | ||
mutationKey: ['key'], | ||
mutationFn: () => Promise.resolve(5), | ||
} as const | ||
|
||
expect(mutationOptions(object)).toStrictEqual(object) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,13 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import type { DefaultError } from '@tanstack/query-core' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import type { UseMutationOptions } from './types' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
export function mutationOptions< | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
TData = unknown, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
TError = DefaultError, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
TVariables = void, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
TContext = unknown, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
>( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
options: UseMutationOptions<TData, TError, TVariables, TContext>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
): UseMutationOptions<TData, TError, TVariables, TContext> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return options | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If mutationOptions is intended to be reused across various TanStack React Query interfaces—such as useMutation, useIsMutating, and queryClient.isMutating—then it might make sense to make mutationKey a required field, similar to how queryOptions.queryKey is typed.
Suggested change
Making mutationKey required could help avoid situations like the following: // Without mutationKey, it’s unavailable for useIsMutating or queryClient.isMutating
// cannot reliably identify the mutation, which may lead to unintended behavior.
function groupMutationOptions() {
return mutationOptions({
mutationFn: addGroup,
});
}
useMutation({
...groupMutationOptions(),
onSuccess: () => queryClient.invalidateQueries({ queryKey: ['groups'] }),
});
useIsMutating(groupMutationOptions())
// This cannot detect the isMutating state from the above hook
// because groupMutationOptions doesn't include a mutationKey.
// but TypeScript compiler doesn't detect this as error So in my opinion, mutationOptions's mutationKey should be required field. So when we first add mutationOptions, it might be beneficial to make mutationKey a required field at first There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If mutationOptions is used not only in useMutation but also in other interfaces such as useIsMutating, I think it would be better to make it a required value. One thing I'm concerned about is that developers who only use mutationOptions for useMutation's options might end up writing unnecessary code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah we can’t make it required; yes filters won’t work then; it’s one of the reasons why I’m against this helper in the first place 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't we make mutationKey required? I know its not really used in useMutation often, but it forces us to write code that would work with any api. Or if we can't do this add a default key the api can fall back to if no key is provided? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tbh I don’t know what the use-case for this helper will be. It’s why I’m so hesitant to add it. I tried to look through issues / discussion to find what people want this for, and the most things I found was:
I think the first 2 usages would be quite surprised that Since it’s easier to go from required -> optional, I think it’s better to make it required, then see the feedback and loosen it up to optional if there’s lots of negative feedback. @manudeli FYI There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also feel that this is a somewhat controversial interface, so I'm unsure whether it's the right decision to add it immediately. I agree that if we do decide to add Also, I think it would be good to mark There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@TkDodo If we need to provide mutationKey as optional at that time, what if we allowed developers to opt into making it optional by registering it through the import '@tanstack/react-query'
declare module '@tanstack/react-query' {
interface Register {
// This is a tentative name
mutationOptionsMutationKey: 'optional' // default is 'required'
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that’s an interesting approach. I think we should try to make it But maybe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed mutationKey to required along with updated test code and documentation. |
Uh oh!
There was an error while loading. Please reload this page.