ronantakizawa/codereview-bench huggingface dataset:
Repository: tanstack/query
File: packages/angular-query-experimental/src/__tests__/query-options.test-d.ts (TypeScript)

Code before:
})

test('should tag the queryKey even if no promise is returned', () => {
  const { queryKey } = queryOptions({
    queryKey: ['key'],
    queryFn: () => 5,
  })
  assertType<number>(queryKey[dataTagSymbol])
})

test('should tag the queryKey with unknown if there is no queryFn', () => {
  const { queryKey } = queryOptions({
    queryKey: ['key'],
  })

  assertType<unknown>(queryKey[dataTagSymbol])
})

test('should tag the queryKey with the result type of the QueryFn if select is used', () => {
  const { queryKey } = queryOptions({
    queryKey: ['key'],
    queryFn: () => Promise.resolve(5),
    select: (data) => data.toString(),
  })

  assertType<unknown>(queryKey[dataTagSymbol])
})

test('should return the proper type when passed to getQueryData', () => {
  const { queryKey } = queryOptions({
    queryKey: ['key'],
    queryFn: () => Promise.resolve(5),
  })

  const queryClient = new QueryClient()
  const data = queryClient.getQueryData(queryKey)

  expectTypeOf(data).toEqualTypeOf<number | undefined>()
})

test('should properly type updaterFn when passed to setQueryData', () => {
  const { queryKey } = queryOptions({
    queryKey: ['key'],
    queryFn: () => Promise.resolve(5),
  })

  const queryClient = new QueryClient()
  const data = queryClient.setQueryData(queryKey, (prev) => {
    expectTypeOf(prev).toEqualTypeOf<number | undefined>()
    return prev
  })

Review comment (suggestion):
wondering why this would be `unknown`. Shouldn't it be `string` ?

```suggestion
  assertType<string>(queryKey[dataTagSymbol])
```

Code after:
})

test('should tag the queryKey even if no promise is returned', () => {
  const { queryKey } = queryOptions({
    queryKey: ['key'],
    queryFn: () => 5,
  })
  assertType<number>(queryKey[dataTagSymbol])
})

test('should tag the queryKey with unknown if there is no queryFn', () => {
  const { queryKey } = queryOptions({
    queryKey: ['key'],
  })

  assertType<unknown>(queryKey[dataTagSymbol])
})

test('should tag the queryKey with the result type of the QueryFn if select is used', () => {
  const { queryKey } = queryOptions({
    queryKey: ['key'],
    queryFn: () => Promise.resolve(5),
    select: (data) => data.toString(),
  })

  assertType<number>(queryKey[dataTagSymbol])
})

test('should return the proper type when passed to getQueryData', () => {
  const { queryKey } = queryOptions({
    queryKey: ['key'],
    queryFn: () => Promise.resolve(5),
  })

  const queryClient = new QueryClient()
  const data = queryClient.getQueryData(queryKey)

  expectTypeOf(data).toEqualTypeOf<number | undefined>()
})

test('should properly type updaterFn when passed to setQueryData', () => {
  const { queryKey } = queryOptions({
    queryKey: ['key'],
    queryFn: () => Promise.resolve(5),
  })

  const queryClient = new QueryClient()
  const data = queryClient.setQueryData(queryKey, (prev) => {
    expectTypeOf(prev).toEqualTypeOf<number | undefined>()
    return prev
  })
Repository: tanstack/query
File: packages/angular-query-experimental/src/__tests__/query-options.test-d.ts (TypeScript)

Code before:
})

test('should tag the queryKey even if no promise is returned', () => {
  const { queryKey } = queryOptions({
    queryKey: ['key'],
    queryFn: () => 5,
  })
  assertType<number>(queryKey[dataTagSymbol])
})

test('should tag the queryKey with unknown if there is no queryFn', () => {
  const { queryKey } = queryOptions({
    queryKey: ['key'],
  })

  assertType<unknown>(queryKey[dataTagSymbol])
})

test('should tag the queryKey with the result type of the QueryFn if select is used', () => {
  const { queryKey } = queryOptions({
    queryKey: ['key'],
    queryFn: () => Promise.resolve(5),
    select: (data) => data.toString(),
  })

  assertType<string>(queryKey[dataTagSymbol])
})

test('should return the proper type when passed to getQueryData', () => {
  const { queryKey } = queryOptions({
    queryKey: ['key'],
    queryFn: () => Promise.resolve(5),
  })

  const queryClient = new QueryClient()
  const data = queryClient.getQueryData(queryKey)

  expectTypeOf(data).toEqualTypeOf<number | undefined>()
})

test('should properly type updaterFn when passed to setQueryData', () => {
  const { queryKey } = queryOptions({
    queryKey: ['key'],
    queryFn: () => Promise.resolve(5),
  })

  const queryClient = new QueryClient()
  const data = queryClient.setQueryData(queryKey, (prev) => {
    expectTypeOf(prev).toEqualTypeOf<number | undefined>()
    return prev
  })

Review comment (suggestion):
aah i messed this up, this should be number of course:

```suggestion
  assertType<number>(queryKey[dataTagSymbol])
```

Code after:
})

test('should tag the queryKey even if no promise is returned', () => {
  const { queryKey } = queryOptions({
    queryKey: ['key'],
    queryFn: () => 5,
  })
  assertType<number>(queryKey[dataTagSymbol])
})

test('should tag the queryKey with unknown if there is no queryFn', () => {
  const { queryKey } = queryOptions({
    queryKey: ['key'],
  })

  assertType<unknown>(queryKey[dataTagSymbol])
})

test('should tag the queryKey with the result type of the QueryFn if select is used', () => {
  const { queryKey } = queryOptions({
    queryKey: ['key'],
    queryFn: () => Promise.resolve(5),
    select: (data) => data.toString(),
  })

  assertType<number>(queryKey[dataTagSymbol])
})

test('should return the proper type when passed to getQueryData', () => {
  const { queryKey } = queryOptions({
    queryKey: ['key'],
    queryFn: () => Promise.resolve(5),
  })

  const queryClient = new QueryClient()
  const data = queryClient.getQueryData(queryKey)

  expectTypeOf(data).toEqualTypeOf<number | undefined>()
})

test('should properly type updaterFn when passed to setQueryData', () => {
  const { queryKey } = queryOptions({
    queryKey: ['key'],
    queryFn: () => Promise.resolve(5),
  })

  const queryClient = new QueryClient()
  const data = queryClient.setQueryData(queryKey, (prev) => {
    expectTypeOf(prev).toEqualTypeOf<number | undefined>()
    return prev
  })
Repository: tanstack/query
File: packages/query-async-storage-persister/src/asyncThrottle.ts (TypeScript)

Code before:
import { noop } from '../../query-core/src/utils'

export interface AsyncThrottleOptions {
  interval?: number
  onError?: (error: unknown) => void
}

export function asyncThrottle<TArgs extends ReadonlyArray<unknown>>(
  func: (...args: TArgs) => Promise<void>,
  { interval = 1000, onError = noop }: AsyncThrottleOptions = {},
) {
  if (typeof func !== 'function') throw new Error('argument is not function.')

  let running = false
  let lastTime = 0
  let timeout: ReturnType<typeof setTimeout>
  let currentArgs: TArgs | null = null

  const execFunc = async () => {
    if (currentArgs) {
      const args = currentArgs
      currentArgs = null
      try {
        running = true
        await func(...args)
      } catch (error) {

Review comment (suggestion):
I don't think we should cross-import between packages, except from their public interface if there is a dependency on that package. One `noop` util function per package should be the way to go, as we wouldn't want to export a public `noop` function from the query core.

Code after:
import { noop } from './utils'

export interface AsyncThrottleOptions {
  interval?: number
  onError?: (error: unknown) => void
}

export function asyncThrottle<TArgs extends ReadonlyArray<unknown>>(
  func: (...args: TArgs) => Promise<void>,
  { interval = 1000, onError = noop }: AsyncThrottleOptions = {},
) {
  if (typeof func !== 'function') throw new Error('argument is not function.')

  let running = false
  let lastTime = 0
  let timeout: ReturnType<typeof setTimeout>
  let currentArgs: TArgs | null = null

  const execFunc = async () => {
    if (currentArgs) {
      const args = currentArgs
      currentArgs = null
      try {
        running = true
        await func(...args)
      } catch (error) {
Repository: tanstack/query
File: examples/react/basic/src/index.jsx (JavaScript)

Code before:
                    }
                  >
                    {post.title}
                  </a>
                </p>
              ))}
            </div>
            <div>{isFetching ? 'Background Updating...' : ' '}</div>
          </>
        )}
      </div>
    </div>
  )
}

const getPostById = async (id) => {
  const { data } = await axios.get(
    `https://jsonplaceholder.typicode.com/posts/${id}`,
  )
  return data
}

function usePost(postId) {
  return useQuery({
    queryKey: ['post', postId],
    queryFn: !!postId ? () => getPostById(postId) : skipToken,
  })
}

function Post({ postId, setPostId }) {
  const { status, data, error, isFetching } = usePost(postId)

  return (
    <div>
      <div>
        <a onClick={() => setPostId(-1)} href="#">
          Back
        </a>
      </div>
      {!postId || status === 'pending' ? (
        'Loading...'
      ) : status === 'error' ? (
        <span>Error: {error.message}</span>
      ) : (
        <>
          <h1>{data.title}</h1>
          <div>
            <p>{data.body}</p>
          </div>
          <div>{isFetching ? 'Background Updating...' : ' '}</div>
        </>

Review comment (suggestion):
not sure if we should change the example like that unless we deprecate `enabled`, which I don't think we will. I see the `skipToken` as a type-safe alternative that we should document in the TypeScript docs, and maybe have the TypeScript example use it, but not the default examples.

Code after:
                  >
                    {post.title}
                  </a>
                </p>
              ))}
            </div>
            <div>{isFetching ? 'Background Updating...' : ' '}</div>
          </>
        )}
      </div>
    </div>
  )
}

const getPostById = async (id) => {
  const { data } = await axios.get(
    `https://jsonplaceholder.typicode.com/posts/${id}`,
  )
  return data
}

function usePost(postId) {
  return useQuery({
    queryKey: ['post', postId],
    queryFn: () => getPostById(postId),
    enabled: !!postId,
  })
}

function Post({ postId, setPostId }) {
  const { status, data, error, isFetching } = usePost(postId)

  return (
    <div>
      <div>
        <a onClick={() => setPostId(-1)} href="#">
          Back
        </a>
      </div>
      {!postId || status === 'pending' ? (
        'Loading...'
      ) : status === 'error' ? (
        <span>Error: {error.message}</span>
      ) : (
        <>
          <h1>{data.title}</h1>
          <div>
            <p>{data.body}</p>
          </div>
          <div>{isFetching ? 'Background Updating...' : ' '}</div>
        </>
Repository: tanstack/query
File: packages/query-core/src/queryClient.ts (TypeScript)

Code before:
    }
    if (typeof defaultedOptions.throwOnError === 'undefined') {
      defaultedOptions.throwOnError = !!defaultedOptions.suspense
    }

    if (
      typeof defaultedOptions.networkMode === 'undefined' &&
      defaultedOptions.persister
    ) {
      defaultedOptions.networkMode = 'offlineFirst'
    }

    if (defaultedOptions.queryFn === skipToken) {
      defaultedOptions.enabled = false
    }

    return defaultedOptions as DefaultedQueryObserverOptions<
      TQueryFnData,
      TError,
      TData,
      TQueryData,
      TQueryKey
    >
  }

  defaultMutationOptions<T extends MutationOptions<any, any, any, any>>(
    options?: T,
  ): T {
    if (options?._defaulted) {
      return options
    }
    return {
      ...this.#defaultOptions.mutations,
      ...(options?.mutationKey &&
        this.getMutationDefaults(options.mutationKey)),
      ...options,
      _defaulted: true,
    } as T
  }

  clear(): void {
    this.#queryCache.clear()
    this.#mutationCache.clear()
  }
}


Review comment (suggestion):
I think we should still check for enabled here as well before overwriting it. With `useSuspenseQuery`, we explicitly set `enabled: true`, and even though TS typings don't allow it, at runtime, you could still pass a `skipToken` to `queryFn`, which would disable a suspense query (thus removing the guarantee that `data` is always defined after calling `useSuspenseQuery`):

```suggestion
    if (defaultedOptions.enabled !== true && defaultedOptions.queryFn === skipToken) {
      defaultedOptions.enabled = false
    }
```

But the question is really: what should take precedence if both are passed? I think on type level, we should make it impossible to pass `skipToken` and `enabled` together with a union type.

Code after:
      _defaulted: true,
    }

    if (!defaultedOptions.queryHash) {
      defaultedOptions.queryHash = hashQueryKeyByOptions(
        defaultedOptions.queryKey,
        defaultedOptions,
      )
    }

    // dependent default values
    if (defaultedOptions.refetchOnReconnect === undefined) {
      defaultedOptions.refetchOnReconnect =
        defaultedOptions.networkMode !== 'always'
    }
    if (defaultedOptions.throwOnError === undefined) {
      defaultedOptions.throwOnError = !!defaultedOptions.suspense
    }

    if (!defaultedOptions.networkMode && defaultedOptions.persister) {
      defaultedOptions.networkMode = 'offlineFirst'
    }

    if (
      defaultedOptions.enabled !== true &&
      defaultedOptions.queryFn === skipToken
    ) {
      defaultedOptions.enabled = false
    }

    return defaultedOptions as DefaultedQueryObserverOptions<
      TQueryFnData,
      TError,
      TData,
      TQueryData,
      TQueryKey
    >
  }

  defaultMutationOptions<T extends MutationOptions<any, any, any, any>>(
    options?: T,
  ): T {
    if (options?._defaulted) {
      return options
    }
    return {
      ...this.#defaultOptions.mutations,
      ...(options?.mutationKey &&
        this.getMutationDefaults(options.mutationKey)),
      ...options,
      _defaulted: true,
Repository: tanstack/query
File: packages/react-query/src/useSuspenseQuery.ts (TypeScript)

Code before:
'use client'
import { QueryObserver, skipToken } from '@tanstack/query-core'
import { useBaseQuery } from './useBaseQuery'
import { defaultThrowOnError } from './suspense'
import type { UseSuspenseQueryOptions, UseSuspenseQueryResult } from './types'
import type { DefaultError, QueryClient, QueryKey } from '@tanstack/query-core'

export function useSuspenseQuery<
  TQueryFnData = unknown,
  TError = DefaultError,
  TData = TQueryFnData,
  TQueryKey extends QueryKey = QueryKey,
>(
  options: UseSuspenseQueryOptions<TQueryFnData, TError, TData, TQueryKey>,
  queryClient?: QueryClient,
): UseSuspenseQueryResult<TData, TError> {
  // @ts-expect-error
  if (options.queryFn === skipToken) {
    throw new Error(
      `You have provided a \`skipToken\` to \`queryFn\` which is not allowed in useSuspenseQuery.`,
    )
  }

  return useBaseQuery(
    {
      ...options,
      enabled: true,
      suspense: true,
      throwOnError: defaultThrowOnError,
      placeholderData: undefined,
    },
    QueryObserver,
    queryClient,
  ) as UseSuspenseQueryResult<TData, TError>
}


Review comment (suggestion):
I think my proposed check in `defaultQueryOptions` would take care of this. We could log this in dev mode though to hint towards the configuration issue.

Code after:
'use client'
import { QueryObserver, skipToken } from '@tanstack/query-core'
import { useBaseQuery } from './useBaseQuery'
import { defaultThrowOnError } from './suspense'
import type { UseSuspenseQueryOptions, UseSuspenseQueryResult } from './types'
import type { DefaultError, QueryClient, QueryKey } from '@tanstack/query-core'

export function useSuspenseQuery<
  TQueryFnData = unknown,
  TError = DefaultError,
  TData = TQueryFnData,
  TQueryKey extends QueryKey = QueryKey,
>(
  options: UseSuspenseQueryOptions<TQueryFnData, TError, TData, TQueryKey>,
  queryClient?: QueryClient,
): UseSuspenseQueryResult<TData, TError> {
  if (process.env.NODE_ENV !== 'production') {
    if (options.queryFn === skipToken) {
      console.error('skipToken is not allowed for useSuspenseQuery')
    }
  }

  return useBaseQuery(
    {
      ...options,
      enabled: true,
      suspense: true,
      throwOnError: defaultThrowOnError,
      placeholderData: undefined,
    },
    QueryObserver,
    queryClient,
  ) as UseSuspenseQueryResult<TData, TError>
}
Repository: tanstack/query
File: packages/query-core/src/tests/mutationObserver.test.tsx (TypeScript)

Code before:
    ).toMatchObject({
      options: { mutationKey: [...key, '1'] },
      state: {
        status: 'success',
        data: 'input',
      },
    })

    mutationObserver.setOptions({
      mutationKey: [...key, '2'],
    })

    expect(
      queryClient.getMutationCache().find({ mutationKey: [...key, '1'] }),
    ).toMatchObject({
      options: { mutationKey: [...key, '1'] },
      state: {
        status: 'success',
        data: 'input',
      },
    })

    unsubscribe()
  })

  test('changing mutation meta should not affect successful mutations', async () => {
    const mutationObserver = new MutationObserver(queryClient, {
      meta: { count: 1 },
      mutationFn: async (text: string) => {
        await sleep(5)
        return text
      },
    })

    const subscriptionHandler = vi.fn()

    const unsubscribe = mutationObserver.subscribe(subscriptionHandler)

    await mutationObserver.mutate('input')

    expect(
      queryClient.getMutationCache().find({}),
    ).toMatchObject({
      options: { meta: { count: 1 } },
      state: {
        status: 'success',
        data: 'input',
      },
    })

    mutationObserver.setOptions({

Review comment (performance):
it would be good to also test the following scenario:

- set meta to `{ a: 1 }`
- call `mutate`, wait until it's done
- trigger a re-render, update meta to `{ a: 2 }`
- call `mutate` again, wait until it's done

after that, we should have two mutations in the cache, where the first one has `{ a: 1 }` and the second one has `{ a: 2 }`

Code after:
    ).toMatchObject({
      options: { mutationKey: [...key, '1'] },
      state: {
        status: 'success',
        data: 'input',
      },
    })

    mutationObserver.setOptions({
      mutationKey: [...key, '2'],
    })

    expect(
      queryClient.getMutationCache().find({ mutationKey: [...key, '1'] }),
    ).toMatchObject({
      options: { mutationKey: [...key, '1'] },
      state: {
        status: 'success',
        data: 'input',
      },
    })

    unsubscribe()
  })

  test('changing mutation meta should not affect successful mutations', async () => {
    const mutationObserver = new MutationObserver(queryClient, {
      meta: { a: 1 },
      mutationFn: async (text: string) => {
        await sleep(5)
        return text
      },
    })

    const subscriptionHandler = vi.fn()

    const unsubscribe = mutationObserver.subscribe(subscriptionHandler)

    await mutationObserver.mutate('input')

    expect(queryClient.getMutationCache().find({})).toMatchObject({
      options: { meta: { a: 1 } },
      state: {
        status: 'success',
        data: 'input',
      },
    })

    mutationObserver.setOptions({
      meta: { a: 2 },
    })
Repository: tanstack/query
File: packages/angular-query-experimental/src/__tests__/inject-mutation.test.ts (TypeScript)

Code before:
    }

    @Component({
      selector: 'app-fake',
      template: ``,
      standalone: true,
      providers: [FakeService],
    })
    class FakeComponent {
      mutation = injectMutation(() => {
        const service = inject(FakeService)
        return {
          mutationFn: (name: string) => service.updateData(name),
        }
      })
    }

    const fixture = TestBed.createComponent(FakeComponent)
    fixture.detectChanges()

    // check if injection contexts persist in a different task
    await new Promise<void>((resolve) => queueMicrotask(() => resolve()))

    expect(
      await fixture.componentInstance.mutation.mutateAsync('test'),
    ).toEqual('test')
  })
})


Review comment (suggestion):
Without this, the test will pass with the previous implementation. That is because the callback of `effect` which causes the exception will run in a different task. 

Code after:

    @Component({
      selector: 'app-fake',
      template: ``,
      standalone: true,
      providers: [FakeService],
    })
    class FakeComponent {
      mutation = injectMutation(() => {
        try {
          const service = inject(FakeService)
          return {
            mutationFn: (name: string) => service.updateData(name),
          }
        } catch (e) {
          errorSpy(e)
          throw e
        }
      })
    }

    const fixture = TestBed.createComponent(FakeComponent)
    fixture.detectChanges()

    // check if injection contexts persist in a different task
    await new Promise<void>((resolve) => queueMicrotask(() => resolve()))

    expect(
      await fixture.componentInstance.mutation.mutateAsync('test'),
    ).toEqual('test')
    expect(errorSpy).not.toHaveBeenCalled()
  })
})
Repository: tanstack/query
File: packages/query-core/src/hydration.ts (TypeScript)

Code before:
        client,
        {
          ...client.getDefaultOptions().hydrate?.queries,
          ...options?.defaultOptions?.queries,
          queryKey,
          queryHash,
          meta,
        },
        // Reset fetch status to idle to avoid
        // query being stuck in fetching state upon hydration
        {
          ...state,
          fetchStatus: 'idle',
        },
      )
    }

    if (promise) {
      const deserialize =
        client.getDefaultOptions().hydrate?.deserialize ?? defaultDeserialize

      const initialPromise = (async () => {
        const result = await promise
        const deserialized = deserialize(result)
        return deserialized
      })()

      // this doesn't actually fetch - it just creates a retryer
      // which will re-use the passed `initialPromise`
      void query.fetch(undefined, { initialPromise })
    }
  })
}


Review comment (suggestion):
for some reason i don't understand 

```ts
const initialPromise = promise.then((result) => deserialize(result))
```

results in a `Missing queryFn` error but this IIFE approach does work

Code after:
          queryKey,
          queryHash,
          meta,
        },
        // Reset fetch status to idle to avoid
        // query being stuck in fetching state upon hydration
        {
          ...state,
          fetchStatus: 'idle',
        },
      )
    }

    if (promise) {
      const transformPromise =
        client.getDefaultOptions().hydrate?.transformPromise

      // Note: `Promise.resolve` required cause
      // RSC transformed promises are not thenable
      const initialPromise =
        transformPromise?.(Promise.resolve(promise)) ?? promise

      // this doesn't actually fetch - it just creates a retryer
      // which will re-use the passed `initialPromise`
      void query.fetch(undefined, { initialPromise })
    }
  })
}