feat(react-query): add mutationOptions #8960
Conversation
github-actions
bot
added
documentation
Ubinquitous
force-pushed
the
feature/react-query-mutation-options
branch
from
5e14207 to
596896d
Compare
|
View your CI Pipeline Execution ↗ for commit 9ded37d.
☁️ 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 #8960 +/- ##
===========================================
+ Coverage 44.40% 84.09% +39.68%
===========================================
Files 206 26 -180
Lines 8174 371 -7803
Branches 1822 109 -1713
===========================================
- Hits 3630 312 -3318
+ Misses 4100 50 -4050
+ Partials 444 9 -435 🚀 New features to boost your workflow:
|
|
Thank you for reviewing my PR. I thought queryOptions and mutationOptions could be structured similarly since it was an options-related function. I re-created useMutation as start. I changed it to only have UseMutationOptions, excluding unnecessary data tags and initialData. |
|
I merged #9094 to resolve below ci failure because of flaky timer tests 
|
docs/framework/react/typescript.md
Outdated
| function useGroupPostMutation() { | ||
| const queryClient = useQueryClient() | ||
|
|
||
| return mutationOptions({ | ||
| mutationKey: ['groups'], | ||
| mutationFn: executeGroups, | ||
| onSuccess: () => { | ||
| queryClient.invalidateQueries({ queryKey: ['posts'] }) | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
It might be worth highlighting in the documentation that mutationOptions can be reused across different interfaces—such as useMutation, useIsMutating, and queryClient.isMutating.
| function useGroupPostMutation() { | |
| const queryClient = useQueryClient() | |
| return mutationOptions({ | |
| mutationKey: ['groups'], | |
| mutationFn: executeGroups, | |
| onSuccess: () => { | |
| queryClient.invalidateQueries({ queryKey: ['posts'] }) | |
| }, | |
| }) | |
| } | |
| function groupMutationOptions() { | |
| return mutationOptions({ | |
| mutationKey: ['groups'], | |
| mutationFn: addGroup, | |
| }) | |
| } | |
| useMutation({ | |
| ...groupMutationOptions() | |
| onSuccess: () => queryClient.invalidateQueries({ queryKey: ['groups'] }) | |
| }) | |
| useIsMutating(groupMutationOptions()) | |
| queryClient.isMutating(groupMutationOptions()) |
There was a problem hiding this comment.
@TkDodo, @Ubinquitous I'd like to hear your thoughts on my review
| 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.
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.
| 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 | |
| } | |
| import type { WithRequired } from 'node_modules/@tanstack/query-core/build/legacy' | |
| import type { DefaultError } from '@tanstack/query-core' | |
| import type { UseMutationOptions } from './types' | |
| export function mutationOptions< | |
| TData = unknown, | |
| TError = DefaultError, | |
| TVariables = void, | |
| TContext = unknown, | |
| >( | |
| options: WithRequired< | |
| UseMutationOptions<TData, TError, TVariables, TContext>, | |
| 'mutationKey' | |
| >, | |
| ): WithRequired< | |
| UseMutationOptions<TData, TError, TVariables, TContext>, | |
| 'mutationKey' | |
| > { | |
| return options | |
| } | |
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 errorSo in my opinion, mutationOptions's mutationKey should be required field.
Additionally, we can make it as optional field later if we need without BREAKING CHANGE.
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.
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.
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.
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.
I can think of developers wondering why useIsMutating isn't working when they forgot the key.
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.
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:
to do this same sharing/separation for mutations
This will make it easier to create custom hooks.
I'd like this for useMutationState
we want to show a state for mutation in progress on a certain item
I think the first 2 usages would be quite surprised that mutationKey is required, while for the last 2, they would be quite surprised if filter didn’t work as expected. It’s also worth noticing that filters can work without a key - the key is not required in filters.
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.
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 mutationOptions, it makes sense to first add mutationOptions with mutationKey as a required field, and then potentially make it optional later based on feedback.
Also, I think it would be good to mark mutationOptions as experimental in the JSDoc
|
@Ubinquitous Resolve eslint error please |
|
Sorry, I fix error that 'should not allow excess properties' test don't have assertions |
|
Really looking forward to this PR! Its really helpful to add reusable optimistic updates, rollbacks etc callbacks for mutations. |
|
Thank you, @andredewaard. I'm not sure if I understood it correctly, but it seems like this can basically be implemented using the spread operator. I’ve used a similar pattern when working with queryOptions as well. Is this what you were referring to? const options = mutationOptions({
...
})
useMutation({
...options,
onSuccess: () => {},
})If you're only changing the funnel in a specific component, I recommend wrapping it in a function for better management. I used to do it that way as well when working with Alternatively, if you're using a function, you could also add logic to branch only when a specific component name is passed in. export const postQueries = {
all: () => ['post-list'],
list: (): UseQueryOptions<Post[], Error, string> =>
queryOptions({
queryKey: [...postListQueries.all()],
queryFn: () => getPostList(),
select: (data) => getPostListString(data),
}),
detail: (type: PostType): UseSuspenseQueryOptions<PostInfo, Error> =>
queryOptions({
queryKey: [...postListQueries.all(), 'detail', type],
queryFn: () => getPostDetail({ type }),
select: (data) => data,
}),
...
}; |
|
@Ubinquitous Thanks for your reply. So more like this. useMutation({
...options,
onSuccess: () => {
options.onSuccess()
// added logic
},
}) |
|
yes, with optional chaining on the options callback: |
|
To override queryOptions without using the spread operator twice, you can use a prop getter. const compose = (...functions) => (...args) =>
functions.forEach((fn) => fn?.(...args))
const options = queryOptions({ ... })
const getOptions = ({ onSuccess }) => {
return {
onSuccess: compose(onSuccess, options.onSuccess),
...options
}
}
getOptions({
onSuccess: () => {} // behaves the same
}) |
| 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.
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.
Thanks, I added more test cases along with the ones that work with useMutation.
|
I find mutationOptions useful. Currently, I am using a style that manages queryKeys and mutationKeys by bundling them into javascript objects, and mutationOptions can reduce developers' type mistakes and boilerplate code when using it this way. In addition, it can be used with useMutation, useIsMutating, and queryClient.isMutating. Just like queryOptions allows call invalidateQueries and use that variable, wouldn't mutationOptions do the same (at useMutation, useIsMutating), keeping the code clean? export const QUERY_KEY = (uniqueKey) => {
return {
get: [...queries.all(), 'get-key', uniqueKey],
list: [...queries.all(), 'list-key', uniqueKey],
};
};
export const queries = {
all: () => ['query-key'],
get: ({ some }) =>
queryOptions({
queryKey: QUERY_KEY(some).get,
queryFn: () => ...,
}),
list: ({ some }) =>
queryOptions({
queryKey: QUERY_KEY(some).list,
queryFn: () => ...,
}),
// this
update: (): UseMutationOptions<
null,
Error,
Request
> => ({
mutationFn: (req) => ...,
}),
}; |
you can achieve the same thing (avoiding type mistakes) by using
it will only work with things like I fear that if we make this helper, it should have But if we do that, a lot of people will ask why all a sudden they have to give every mutation a |
okay, it actually needs to be guess we need that helper after all |
mutationOptions helps extracting mutation options into separate functions.