ronantakizawa/codereview-bench huggingface dataset:
Repository: tannerlinsley/react-query
File: packages/react-query/src/__tests__/mutationOptions.test-d.tsx (TypeScript)

Code before:
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>()
      },
    })
  })
})


Review comment (suggestion):
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.

```tsx
const Example = () => {
  const mutation = useMutation({
    ...mutationOptions({
      mutationKey: ['key'],
      mutationFn: () => Promise.resolve({ field: 'test' })
    }),
    onSuccess: (data) => expectTypeOf(data).toEqualTypeOf<{ field: string }>()
  })
  expectTypeOf(mutation).toEqualTypeOf ...
}
```


Code after:
import { assertType, describe, expectTypeOf, it } from 'vitest'
import { QueryClient } from '@tanstack/query-core'
import { useIsMutating, useMutation, useMutationState } from '..'
import { mutationOptions } from '../mutationOptions'
import type {
  DefaultError,
  MutationState,
  WithRequired,
} from '@tanstack/query-core'
import type { UseMutationOptions, UseMutationResult } from '../types'

describe('mutationOptions', () => {
  it('should not allow excess properties', () => {
    // @ts-expect-error this is a good error, because onMutates does not exist!
    mutationOptions({
      mutationFn: () => Promise.resolve(5),
      mutationKey: ['key'],
      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')
      },
      mutationKey: ['key'],
      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)
Repository: tannerlinsley/react-query
File: packages/react-query/src/__tests__/mutationOptions.test-d.tsx (TypeScript)

Code before:
    const mutation = mutationOptions({
      mutationFn: (id: string) => Promise.resolve(id.length),
      mutationKey: ['key'],
      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: string }>({ field: 'test' }),
      }),
      onSuccess: (data) =>
        expectTypeOf(data).toEqualTypeOf<{ field: string }>(),
    })
    expectTypeOf(mutation).toMatchTypeOf<
      UseMutationResult<{ field: string }, DefaultError, void, unknown>
    >()
  })
})


Review comment (suggestion):
@Ubinquitous It seems better to add type test cases for `mutationOptions` not only with `useMutation`, but also with `useIsMutating`, `queryClient.isMutating`, etc. Being compatible with these interfaces is also an important feature of `mutationOptions` and should be tested IMO

Code after:
        },
      }),
    ).toEqualTypeOf<
      WithRequired<
        UseMutationOptions<number, DefaultError, string>,
        'mutationKey'
      >
    >()
    expectTypeOf(
      mutationOptions({
        mutationFn: (id: string) => Promise.resolve(id.length),
        onSuccess: (data) => {
          expectTypeOf(data).toEqualTypeOf<number>()
        },
      }),
    ).toEqualTypeOf<
      Omit<UseMutationOptions<number, DefaultError, string>, 'mutationKey'>
    >()
  })

  it('should infer types when used with useMutation', () => {
    const mutation = useMutation(
      mutationOptions({
        mutationKey: ['key'],
        mutationFn: () => Promise.resolve('data'),
        onSuccess: (data) => {
          expectTypeOf(data).toEqualTypeOf<string>()
        },
      }),
    )
    expectTypeOf(mutation).toEqualTypeOf<
      UseMutationResult<string, DefaultError, void, unknown>
    >()

    useMutation(
      // should allow when used with useMutation without mutationKey
      mutationOptions({
        mutationFn: () => Promise.resolve('data'),
        onSuccess: (data) => {
          expectTypeOf(data).toEqualTypeOf<string>()
        },
      }),
    )
  })

  it('should infer types when used with useIsMutating', () => {
    const isMutating = useIsMutating(
      mutationOptions({
        mutationKey: ['key'],
        mutationFn: () => Promise.resolve(5),
      }),
Repository: tannerlinsley/react-query
File: packages/react-query/src/mutationOptions.ts (TypeScript)

Code before:
import type { DefaultError, WithRequired } 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
}


Review comment (suggestion):
Just had a quick thread with @TkDodo [on bsky](https://bsky.app/profile/tkdodo.eu/post/3lr3jww3t422t) and thought I'd leave this as a possible tweak. How about these overloads to infer whether mutationKey is actually there at compile time? (might need tweaking a bit)

```suggestion
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'
> {
export function mutationOptions<
  TData = unknown,
  TError = DefaultError,
  TVariables = void,
  TContext = unknown,
>(
  options: Omit<
    UseMutationOptions<TData, TError, TVariables, TContext>,
    'mutationKey'
  >,
): Omit<
  UseMutationOptions<TData, TError, TVariables, TContext>,
  'mutationKey'
> & { mutationKey?: undefined }
export function mutationOptions<
  TData = unknown,
  TError = DefaultError,
  TVariables = void,
  TContext = unknown,
>(
  options: UseMutationOptions<TData, TError, TVariables, TContext>,
): UseMutationOptions<TData, TError, TVariables, TContext> {
```

Code after:
import type { DefaultError, WithRequired } 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'
>
export function mutationOptions<
  TData = unknown,
  TError = DefaultError,
  TVariables = void,
  TContext = unknown,
>(
  options: Omit<
    UseMutationOptions<TData, TError, TVariables, TContext>,
    'mutationKey'
  >,
): Omit<UseMutationOptions<TData, TError, TVariables, TContext>, 'mutationKey'>
export function mutationOptions<
  TData = unknown,
  TError = DefaultError,
  TVariables = void,
  TContext = unknown,
>(
  options: UseMutationOptions<TData, TError, TVariables, TContext>,
): UseMutationOptions<TData, TError, TVariables, TContext> {
  return options
}
Repository: tannerlinsley/react-query
File: packages/react-query/src/__tests__/mutationOptions.test-d.tsx (TypeScript)

Code before:
    expectTypeOf(
      mutationOptions({
        mutationFn: (id: string) => Promise.resolve(id.length),
        onSuccess: (data) => {
          expectTypeOf(data).toEqualTypeOf<number>()
        },
      }),
    ).toEqualTypeOf<
      Omit<UseMutationOptions<number, DefaultError, string>, 'mutationKey'>
    >()
  })

  it('should infer types when used with useMutation', () => {
    const mutation = useMutation(
      mutationOptions({
        mutationKey: ['key'],
        mutationFn: () => Promise.resolve('data'),
        onSuccess: (data) => {
          expectTypeOf(data).toEqualTypeOf<string>()
        },
      }),
    )
    expectTypeOf(mutation).toEqualTypeOf<
      UseMutationResult<string, DefaultError, void, unknown>
    >()
  })

  it('should infer types when used with useIsMutating', () => {
    const isMutating = useIsMutating(
      mutationOptions({
        mutationKey: ['key'],
        mutationFn: () => Promise.resolve(5),
      }),
    )
    expectTypeOf(isMutating).toEqualTypeOf<number>()

    useIsMutating(
      // @ts-expect-error filters should have mutationKey
      mutationOptions({
        mutationFn: () => Promise.resolve(5),
      }),
    )
  })

  it('should infer types when used with queryClient.isMutating', () => {
    const queryClient = new QueryClient()

    const isMutating = queryClient.isMutating(
      mutationOptions({
        mutationKey: ['key'],
        mutationFn: () => Promise.resolve(5),

Review comment (suggestion):
we should have a test _without_ `mutationKey` here too, please (like we have below with `useIsMutating` and `useMutationState`)

Code after:
      mutationOptions({
        mutationFn: (id: string) => Promise.resolve(id.length),
        onSuccess: (data) => {
          expectTypeOf(data).toEqualTypeOf<number>()
        },
      }),
    ).toEqualTypeOf<
      Omit<UseMutationOptions<number, DefaultError, string>, 'mutationKey'>
    >()
  })

  it('should infer types when used with useMutation', () => {
    const mutation = useMutation(
      mutationOptions({
        mutationKey: ['key'],
        mutationFn: () => Promise.resolve('data'),
        onSuccess: (data) => {
          expectTypeOf(data).toEqualTypeOf<string>()
        },
      }),
    )
    expectTypeOf(mutation).toEqualTypeOf<
      UseMutationResult<string, DefaultError, void, unknown>
    >()

    useMutation(
      // should allow when used with useMutation without mutationKey
      mutationOptions({
        mutationFn: () => Promise.resolve('data'),
        onSuccess: (data) => {
          expectTypeOf(data).toEqualTypeOf<string>()
        },
      }),
    )
  })

  it('should infer types when used with useIsMutating', () => {
    const isMutating = useIsMutating(
      mutationOptions({
        mutationKey: ['key'],
        mutationFn: () => Promise.resolve(5),
      }),
    )
    expectTypeOf(isMutating).toEqualTypeOf<number>()

    useIsMutating(
      // @ts-expect-error filters should have mutationKey
      mutationOptions({
        mutationFn: () => Promise.resolve(5),
      }),
    )
Repository: tannerlinsley/react-query
File: packages/react-query/src/__tests__/mutationOptions.test-d.tsx (TypeScript)

Code before:
        mutationFn: (id: string) => Promise.resolve(id.length),
        onSuccess: (data) => {
          expectTypeOf(data).toEqualTypeOf<number>()
        },
      }),
    ).toEqualTypeOf<
      Omit<UseMutationOptions<number, DefaultError, string>, 'mutationKey'>
    >()
  })

  it('should infer types when used with useMutation', () => {
    const mutation = useMutation(
      mutationOptions({
        mutationKey: ['key'],
        mutationFn: () => Promise.resolve('data'),
        onSuccess: (data) => {
          expectTypeOf(data).toEqualTypeOf<string>()
        },
      }),
    )
    expectTypeOf(mutation).toEqualTypeOf<
      UseMutationResult<string, DefaultError, void, unknown>
    >()

    useMutation(
      // @ts-check should allow when used with useMutation without mutationKey
      mutationOptions({
        mutationFn: () => Promise.resolve('data'),
        onSuccess: (data) => {
          expectTypeOf(data).toEqualTypeOf<string>()
        },
      }),
    )
  })

  it('should infer types when used with useIsMutating', () => {
    const isMutating = useIsMutating(
      mutationOptions({
        mutationKey: ['key'],
        mutationFn: () => Promise.resolve(5),
      }),
    )
    expectTypeOf(isMutating).toEqualTypeOf<number>()

    useIsMutating(
      // @ts-expect-error filters should have mutationKey
      mutationOptions({
        mutationFn: () => Promise.resolve(5),
      }),
    )
  })

Review comment (suggestion):
```suggestion
      // should allow when used with useMutation without mutationKey
```

Code after:
        mutationFn: (id: string) => Promise.resolve(id.length),
        onSuccess: (data) => {
          expectTypeOf(data).toEqualTypeOf<number>()
        },
      }),
    ).toEqualTypeOf<
      Omit<UseMutationOptions<number, DefaultError, string>, 'mutationKey'>
    >()
  })

  it('should infer types when used with useMutation', () => {
    const mutation = useMutation(
      mutationOptions({
        mutationKey: ['key'],
        mutationFn: () => Promise.resolve('data'),
        onSuccess: (data) => {
          expectTypeOf(data).toEqualTypeOf<string>()
        },
      }),
    )
    expectTypeOf(mutation).toEqualTypeOf<
      UseMutationResult<string, DefaultError, void, unknown>
    >()

    useMutation(
      // should allow when used with useMutation without mutationKey
      mutationOptions({
        mutationFn: () => Promise.resolve('data'),
        onSuccess: (data) => {
          expectTypeOf(data).toEqualTypeOf<string>()
        },
      }),
    )
  })

  it('should infer types when used with useIsMutating', () => {
    const isMutating = useIsMutating(
      mutationOptions({
        mutationKey: ['key'],
        mutationFn: () => Promise.resolve(5),
      }),
    )
    expectTypeOf(isMutating).toEqualTypeOf<number>()

    useIsMutating(
      // @ts-expect-error filters should have mutationKey
      mutationOptions({
        mutationFn: () => Promise.resolve(5),
      }),
    )
  })
Repository: tannerlinsley/react-query
File: packages/query-persist-client-core/src/persist.ts (TypeScript)

Code before:
        if (expired || busted) {
          return persister.removeClient()
        } else {
          hydrate(queryClient, persistedClient.clientState, hydrateOptions)
        }
      } else {
        return persister.removeClient()
      }
    }
  } catch (restoreClientError) {
    if (process.env.NODE_ENV !== 'production') {
      console.error(restoreClientError)
      console.warn(
        'Encountered an error attempting to restore client cache from persisted location. As a precaution, the persisted cache will be discarded.',
      )
    }

    try {
      await persister.removeClient()
    } catch (removeClientError) {
      if (process.env.NODE_ENV !== 'production') {
        console.error(removeClientError)
      }

      throw removeClientError
    }

    throw restoreClientError
  }
}

/**
 * Persists data from the QueryCache
 *  - data dehydrated using dehydrateOptions
 *  - data is persisted using persister.persistClient
 */
export async function persistQueryClientSave({
  queryClient,
  persister,
  buster = '',
  dehydrateOptions,
}: PersistedQueryClientSaveOptions) {
  const persistClient: PersistedClient = {
    buster,
    timestamp: Date.now(),
    clientState: dehydrate(queryClient, dehydrateOptions),
  }

  await persister.persistClient(persistClient)
}


Review comment (suggestion):
I would not catch and rethrow here just for logging. We don’t do this in other places where we call `persister.removeClient()

Code after:
        if (expired || busted) {
          return persister.removeClient()
        } else {
          hydrate(queryClient, persistedClient.clientState, hydrateOptions)
        }
      } else {
        return persister.removeClient()
      }
    }
  } catch (err) {
    if (process.env.NODE_ENV !== 'production') {
      console.error(err)
      console.warn(
        'Encountered an error attempting to restore client cache from persisted location. As a precaution, the persisted cache will be discarded.',
      )
    }

    await persister.removeClient()

    throw err
  }
}

/**
 * Persists data from the QueryCache
 *  - data dehydrated using dehydrateOptions
 *  - data is persisted using persister.persistClient
 */
export async function persistQueryClientSave({
  queryClient,
  persister,
  buster = '',
  dehydrateOptions,
}: PersistedQueryClientSaveOptions) {
  const persistClient: PersistedClient = {
    buster,
    timestamp: Date.now(),
    clientState: dehydrate(queryClient, dehydrateOptions),
  }

  await persister.persistClient(persistClient)
}

/**
 * Subscribe to QueryCache and MutationCache updates (for persisting)
 * @returns an unsubscribe function (to discontinue monitoring)
 */
export function persistQueryClientSubscribe(
  props: PersistedQueryClientSaveOptions,
) {
  const unsubscribeQueryCache = props.queryClient
Repository: tannerlinsley/react-query
File: packages/query-core/src/__tests__/mutations.test.tsx (TypeScript)

Code before:
        {
          context: 'todo',
          data: undefined,
          error: null,
          failureCount: 1,
          failureReason: 'err',
          isPaused: true,
          status: 'pending',
          variables: 'todo',
          submittedAt: 1,
        },
      )

    expect(mutation.state).toEqual({
      context: 'todo',
      data: undefined,
      error: null,
      failureCount: 1,
      failureReason: 'err',
      isPaused: true,
      status: 'pending',
      variables: 'todo',
      submittedAt: 1,
    })

    queryClient.resumePausedMutations()
    await vi.advanceTimersByTimeAsync(0)

    // check that the mutation is correctly resumed
    expect(mutation.state).toEqual({
      context: 'todo',
      data: undefined,
      error: null,
      failureCount: 1,
      failureReason: 'err',
      isPaused: false,
      status: 'pending',
      variables: 'todo',
      submittedAt: 1,
    })

    await vi.advanceTimersByTimeAsync(20)

    expect(mutation.state).toEqual({
      context: 'todo',
      data: 'todo',
      error: null,
      failureCount: 0,
      failureReason: null,
      isPaused: false,
      status: 'success',

Review comment (suggestion):
```suggestion
    void queryClient.resumePausedMutations()
```

Code after:
        {
          context: 'todo',
          data: undefined,
          error: null,
          failureCount: 1,
          failureReason: 'err',
          isPaused: true,
          status: 'pending',
          variables: 'todo',
          submittedAt: 1,
        },
      )

    expect(mutation.state).toEqual({
      context: 'todo',
      data: undefined,
      error: null,
      failureCount: 1,
      failureReason: 'err',
      isPaused: true,
      status: 'pending',
      variables: 'todo',
      submittedAt: 1,
    })

    void queryClient.resumePausedMutations()
    await vi.advanceTimersByTimeAsync(0)

    // check that the mutation is correctly resumed
    expect(mutation.state).toEqual({
      context: 'todo',
      data: undefined,
      error: null,
      failureCount: 1,
      failureReason: 'err',
      isPaused: false,
      status: 'pending',
      variables: 'todo',
      submittedAt: 1,
    })

    await vi.advanceTimersByTimeAsync(20)

    expect(mutation.state).toEqual({
      context: 'todo',
      data: 'todo',
      error: null,
      failureCount: 0,
      failureReason: null,
      isPaused: false,
      status: 'success',
Repository: tannerlinsley/react-query
File: packages/query-core/src/__tests__/infiniteQueryBehavior.test.tsx (TypeScript)

Code before:
    const observer = new InfiniteQueryObserver(queryClient, {
      queryKey: key,
      queryFn: () => 'data',
      getNextPageParam: () => null,
      initialPageParam: null,
    })

    let observerResult:
      | InfiniteQueryObserverResult<unknown, unknown>
      | undefined

    const unsubscribe = observer.subscribe((result) => {
      observerResult = result
    })

    await vi.waitFor(() =>
      expect(observerResult).toMatchObject({
        isFetching: false,
        data: { pages: ['data'], pageParams: [null] },
      }),
    )

    unsubscribe()
  })

  test('InfiniteQueryBehavior should handle null page parameters correctly', async () => {
    const key = queryKey()

    const queryFnSpy = vi.fn().mockImplementation(({ pageParam }) => {
      return pageParam
    })

    const observer = new InfiniteQueryObserver<number>(queryClient, {
      queryKey: key,
      queryFn: queryFnSpy,
      getNextPageParam: (lastPage) => (lastPage === 1 ? null : lastPage + 1),
      initialPageParam: 1,
    })

    let observerResult:
      | InfiniteQueryObserverResult<unknown, unknown>
      | undefined

    const unsubscribe = observer.subscribe((result) => {
      observerResult = result
    })

    await vi.waitFor(() =>
      expect(observerResult).toMatchObject({
        isFetching: false,
        data: { pages: [1], pageParams: [1] },

Review comment (suggestion):
The test name `InfiniteQueryBehavior should handle null page parameters correctly` is a bit vague. Consider specifying what “correctly” means in this context

Code after:
    const observer = new InfiniteQueryObserver(queryClient, {
      queryKey: key,
      queryFn: () => 'data',
      getNextPageParam: () => null,
      initialPageParam: null,
    })

    let observerResult:
      | InfiniteQueryObserverResult<unknown, unknown>
      | undefined

    const unsubscribe = observer.subscribe((result) => {
      observerResult = result
    })

    await vi.waitFor(() =>
      expect(observerResult).toMatchObject({
        isFetching: false,
        data: { pages: ['data'], pageParams: [null] },
      }),
    )

    unsubscribe()
  })

  test('InfiniteQueryBehavior should not fetch next page when getNextPageParam returns null', async () => {
    const key = queryKey()

    const observer = new InfiniteQueryObserver(queryClient, {
      queryKey: key,
      queryFn: ({ pageParam }) => sleep(0).then(() => pageParam),
      getNextPageParam: (lastPage) => (lastPage === 1 ? null : lastPage + 1),
      initialPageParam: 1,
    })

    let observerResult:
      | InfiniteQueryObserverResult<InfiniteData<number, unknown>, Error>
      | undefined

    const unsubscribe = observer.subscribe((result) => {
      observerResult = result
    })

    await vi.waitFor(() =>
      expect(observerResult).toMatchObject({
        isFetching: false,
        data: { pages: [1], pageParams: [1] },
      }),
    )

    await observer.fetchNextPage()