Skip to content

feat(query-core): make MutateFunction optional undefinable-variables #8737

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.

Sign up for GitHub

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
wants to merge 4 commits into
base: main
Choose a base branch
from
+151 −2
Open

feat(query-core): make MutateFunction optional undefinable-variables #8737

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2ddfb1c
feat(query-core): make `MutateFunction` optional undefinable-variables
unnoq Mar 3, 2025
3085870
test(query-core): MutateFunction
unnoq Mar 4, 2025
02db0f2
test(query-core): MutateFunction
unnoq Mar 4, 2025
01b77b4
Merge branch 'main' into feat/query-core/optional-undefindable-variables
TkDodo Mar 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions packages/query-core/src/__tests__/types.test-d.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import { describe, expectTypeOf, it } from 'vitest'
import type { MutateFunction } from 'src/types'

describe('MutateFunction', () => {
it('optional undefinable variables', () => {
const mutate = {} as MutateFunction<
unknown,
unknown,
number | undefined,
unknown
>

expectTypeOf<Parameters<typeof mutate>[0]>().toEqualTypeOf<
number | undefined
>()

mutate() // can be called with no arguments
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that’s a good improvement 👍 . I’m missing a test for the use-case where we have a mutation function that doesn’t take anything in, so the variables are void.

Also, I’d prefer if we create the mutation functions not via type assertions, but by whatever a MutationObserver returns. Something like:

const { mutate } = new MutationObserver(new QueryClient(), {
  mutationFn: async (_vars: number | undefined) => {
    return null
  },
})

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the void case as requested. However, I couldn’t create a test using MutationObserver because the mutate function is implemented as a standalone function rather than using MutateFunction. As a result, the changes don’t impact its behavior. Let me know if you have any suggestions on how to approach testing this scenario.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh interesting. question is if it would also work with the mutate function returned from useMutation, as that is what most users will use. The type is defined here:

query/packages/react-query/src/types.ts

Lines 197 to 204 in 3e3fba9

export type UseMutateFunction<
TData = unknown,
TError = DefaultError,
TVariables = void,
TContext = unknown,
> = (
...args: Parameters<MutateFunction<TData, TError, TVariables, TContext>>
) => void

would be great to have a test in useMutation.test-d.tsx in the react-query adapter for this then 🙏

Copy link
Author

@unnoq unnoq Mar 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've already covered this case. Please check my tests.

https://github.com/TanStack/query/pull/8737/files#diff-fd352cb3d35bea507f0ff667365ff9ab3c51a78c493656207dc3a5291a2491b2R67-R133

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've already covered this case

yeah that’s okay for the query-core, but it doesn’t actually test what useMutation in react-query does. It’s great that we cover the functionality that we have in useMutation now, but if we refactor that, it won’t be covered by the test in the core.

That’s why each adapter should have its own tests for the types they are doing. But we can do this in a follow-up.

})

it('required non-undefinable variables', () => {
const mutate = {} as MutateFunction<unknown, unknown, number, unknown>

expectTypeOf<Parameters<typeof mutate>[0]>().toEqualTypeOf<number>()

// @ts-expect-error --- required variables
mutate()
})

describe('compatible with spread arguments pattern', () => {
// this is common pattern used internal so we need make sure it still works

it('optional undefinable variables', () => {
const mutate = {} as (
...options: Parameters<
MutateFunction<unknown, unknown, number | undefined, unknown>
>
) => void

expectTypeOf<Parameters<typeof mutate>[0]>().toEqualTypeOf<
number | undefined
>()

mutate() // can be called with no arguments
})

it('required non-undefinable variables', () => {
const mutate = {} as (
...options: Parameters<
MutateFunction<unknown, unknown, number, unknown>
>
) => void

expectTypeOf<Parameters<typeof mutate>[0]>().toEqualTypeOf<number>()

// @ts-expect-error --- required variables
mutate()
})
})
})
18 changes: 16 additions & 2 deletions packages/query-core/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1149,14 +1149,28 @@ export interface MutateOptions<
) => void
}

export type MutateFunctionRest<
TData = unknown,
TError = DefaultError,
TVariables = void,
TContext = unknown,
> = undefined extends TVariables
? [
variables?: TVariables,
options?: MutateOptions<TData, TError, TVariables, TContext>,
]
: [
variables: TVariables,
options?: MutateOptions<TData, TError, TVariables, TContext>,
]

export type MutateFunction<
TData = unknown,
TError = DefaultError,
TVariables = void,
TContext = unknown,
> = (
variables: TVariables,
options?: MutateOptions<TData, TError, TVariables, TContext>,
...rest: MutateFunctionRest<TData, TError, TVariables, TContext>
) => Promise<TData>

export interface MutationObserverBaseResult<
Expand Down