-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Open
Ubinquitous
wants to merge
16
commits into
TanStack:main
Choose a base branch
from
Ubinquitous:feature/react-query-mutation-options
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
596896d
feat(react-query): add mutationOptions
Ubinquitous ff15e5d
test(react-query): add DataTag test case
Ubinquitous ea54b58
Merge branch 'main' into feature/react-query-mutation-options
TkDodo 2972edd
fix(react-query): remove unnecessary types from mutation
Ubinquitous 08a5026
fix(react-query): remove unncessary type overload
Ubinquitous f3b74c0
Merge branch 'main' into feature/react-query-mutation-options
manudeli a4560d3
Merge branch 'main' into feature/react-query-mutation-options
manudeli 6889638
chore(react-query): add mutationOptions to barrel file
Ubinquitous b844dee
Merge branch 'main' into feature/react-query-mutation-options
manudeli e61227d
Merge branch 'main' into feature/react-query-mutation-options
manudeli 33d3e9f
fix(react-query): fix test eslint issue
Ubinquitous fd7b9f9
docs(react-query): add more examples
Ubinquitous 6ee8c76
Merge branch 'main' into feature/react-query-mutation-options
manudeli 299a19f
Merge branch 'main' into feature/react-query-mutation-options
manudeli 2622902
Merge branch 'main' into feature/react-query-mutation-options
TkDodo 9ded37d
test(react-query): add more test cases
Ubinquitous File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
107 changes: 107 additions & 0 deletions
107
packages/react-query/src/__tests__/mutationOptions.test-d.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,107 @@ | ||
import { assertType, describe, expectTypeOf, it } from 'vitest' | ||
import { useMutation } from 'src/useMutation' | ||
import { mutationOptions } from '../mutationOptions' | ||
import type { UseMutationOptions, UseMutationResult } from 'src/types' | ||
import type { DefaultError } from '@tanstack/query-core' | ||
|
||
describe('mutationOptions', () => { | ||
it('should not allow excess properties', () => { | ||
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', () => { | ||
mutationOptions({ | ||
mutationFn: () => Promise.resolve(5), | ||
mutationKey: ['key'], | ||
onSuccess: (data) => { | ||
expectTypeOf(data).toEqualTypeOf<number>() | ||
}, | ||
}) | ||
}) | ||
|
||
it('should infer types for onError callback', () => { | ||
mutationOptions({ | ||
mutationFn: () => { | ||
throw new Error('fail') | ||
}, | ||
onError: (error) => { | ||
expectTypeOf(error).toEqualTypeOf<DefaultError>() | ||
}, | ||
}) | ||
}) | ||
|
||
it('should infer types for variables', () => { | ||
mutationOptions<number, DefaultError, { id: string }>({ | ||
mutationFn: (vars) => { | ||
expectTypeOf(vars).toEqualTypeOf<{ id: string }>() | ||
return Promise.resolve(5) | ||
}, | ||
mutationKey: ['with-vars'], | ||
}) | ||
}) | ||
|
||
it('should infer context type correctly', () => { | ||
mutationOptions<number, DefaultError, void, { name: string }>({ | ||
mutationFn: () => Promise.resolve(5), | ||
onMutate: () => { | ||
return { name: 'context' } | ||
}, | ||
onSuccess: (_data, _variables, context) => { | ||
expectTypeOf(context).toEqualTypeOf<{ name: string }>() | ||
}, | ||
}) | ||
}) | ||
|
||
it('should error if mutationFn return type mismatches TData', () => { | ||
assertType( | ||
mutationOptions<number>({ | ||
// @ts-expect-error this is a good error, because return type is string, not number | ||
mutationFn: async () => Promise.resolve('wrong return'), | ||
}), | ||
) | ||
}) | ||
|
||
it('should allow mutationKey to be omitted', () => { | ||
return mutationOptions({ | ||
mutationFn: () => Promise.resolve(123), | ||
onSuccess: (data) => { | ||
expectTypeOf(data).toEqualTypeOf<number>() | ||
}, | ||
}) | ||
}) | ||
|
||
it('should infer all types when not explicitly provided', () => { | ||
const mutation = mutationOptions({ | ||
mutationFn: (id: string) => Promise.resolve(id.length), | ||
onSuccess: (data) => { | ||
expectTypeOf(data).toEqualTypeOf<number>() | ||
}, | ||
}) | ||
|
||
expectTypeOf(mutation).toMatchTypeOf< | ||
UseMutationOptions<number, DefaultError, string> | ||
>() | ||
}) | ||
|
||
it('should infer types when used with useMutation', () => { | ||
const mutation = useMutation({ | ||
...mutationOptions({ | ||
mutationKey: ['key'], | ||
mutationFn: () => Promise.resolve({ field: 'test' }), | ||
}), | ||
onSuccess: (data) => | ||
expectTypeOf(data).toEqualTypeOf<{ field: string }>(), | ||
}) | ||
expectTypeOf(mutation).toMatchTypeOf< | ||
UseMutationResult<{ field: string }, DefaultError, void, unknown> | ||
>() | ||
}) | ||
}) |
14 changes: 14 additions & 0 deletions
14
packages/react-query/src/__tests__/mutationOptions.test.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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) | ||
}) | ||
}) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 | ||
} | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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.
Making mutationKey required could help avoid situations like the following:
So 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
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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 comment
The 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 comment
The 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.
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.
Choose a reason for hiding this comment
The 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
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.
Choose a reason for hiding this comment
The 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
mutationOptions
, it makes sense to first addmutationOptions
withmutationKey
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 JSDocThere was a problem hiding this comment.
Choose a reason for hiding this comment
The 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
Register
interface like defaultError? If so, it seems we could offermutationOptions
tailored to library users.