query icon indicating copy to clipboard operation
query copied to clipboard

`retry` from `defaultOptions.queries.retry` is ignored when `retry: undefined` is used in useQuery's options

Open vdvukhzhilov opened this issue 1 year ago • 2 comments

Describe the bug

You can provide retry in defaultOptions.queries when instantiating QueryClient. This is used when no retry option is provided to useQuery.

But when we provide retry: undefined to useQuery options defaultOptions.queries.retry is ignored, instead lib default retry kicks in.

Your minimal, reproducible example

https://codesandbox.io/p/sandbox/react-query-retry-bug-vd8y9h?file=%2Fsrc%2FApp.js%3A5%2C31

Steps to reproduce

  1. Add any logic or retry count to defaultOptions.queries.retry of a query client
  2. Add retry: undefined as option to useQuery

Expected behavior

retry is used from defaultOptions.queries

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

any platfrom

Tanstack Query adapter

None

TanStack Query version

v5.37.1

TypeScript version

No response

Additional context

No response

vdvukhzhilov avatar May 23 '24 14:05 vdvukhzhilov

I believe the problem is in this line

And this one, too

undefined values should be filtered out before assigning as they are essentially the same as providing no value at all and in that case should fallback to defaultOptions and than fallback to lib defaults

vdvukhzhilov avatar May 23 '24 14:05 vdvukhzhilov

More advanced example

function useSomeQuery({ retry }: {retry?: number}) {
  return useQuery({
    retry
  })
}

Here we suppose that you can omit retry when passing to useSomeQuery({}). But due to spread of argument retry is passed as undefined to useQuery.

This would lead to some hours spent on debugging why default option is not applied here? I did not pass retry to my hook so logically it should fallback to default option.

vdvukhzhilov avatar May 23 '24 15:05 vdvukhzhilov

This is on purpose. The default options only kick in when the key is not passed to useQuery, not when the value is set to undefined. A slight difference, but a difference nonetheless. we just object spread what you give us, so the fix is to not assign the key on your end. So instead of:

useQuery({
  queryKey,
  queryFn,
  retry: someCondition ? 5 : undefined
}

do:

useQuery({
  queryKey,
  queryFn,
  ...(someCondition && { retry: 5 })
}

TkDodo avatar May 24 '24 19:05 TkDodo

The default options only kick in when the key is not passed to useQuery, not when the value is set to undefined

@TkDodo Thank you for the clarification!

If that is by design, I would say that this should be pointed out in documentation.

Here is why:

  1. I have not found any information in docs or your blog that would explain this particular behaviour (I mean falling back to lib defaults while setting option to undefined in query). Except that is sortof implied here in line It is important to know that this will only work if your actual useQuery has no explicit retries set, but even there the example is given that you could set your own retry for particular query when retries are turned off in defaultOptions.retry. Which falls into general semantics of term "default options".

  2. When passing empty option ignores default options provided by you it is totally unclear because of semantics of term "default options", as passing your own default options usually implies that they are used instead of lib defaults.

  3. Also passing undefined as an option value could be not as clear as in your examples. I have already shown how it could be view as "not providing" option here where retry is passed from spreaded argument. Yes, the way spreading argument works is by creating a variable which is undefined here and yes this way we "under the hood" pass retry: undefined. But, in our heads we consider this the same as not passing at all because we never write retry: undefined here.

  4. Also, if following "we just object spread what you give us" there a discrepancy in that passing undefined as option skips user-passed default options but not the lib-defined default options. If the rule is "we just object spread what you give us" I would expect that no lib default would kick in either.

So considering all the points above I think it is better to explicitly state that passing undefined option will lead to overwriting defaultOptions one so that people would not spend time debugging 😄

vdvukhzhilov avatar May 25 '24 08:05 vdvukhzhilov

If that is by design, I would say that this should be pointed out in documentation.

sure, feel free to amend the docs.

have not found any information in docs or your blog that would explain this particular behaviour

it's not like this question has come up a lot - this is the first time in 4 years for me 😅 . So I guess most people just don't pass something: undefined to useQuery.

I have already shown how it could be view as "not providing" option

Do you mean this example?

function useSomeQuery({ retry }: {retry?: number}) {
  return useQuery({
    retry
  })
}

Because with exactOptionalPropertyTypes turned on, you couldn't pass retry:undefined to your custom hook. { retry?: number } is different from { retry: number | undefined }, and since you clearly care about this distinction, I'd suggest turning that TypeScript feature on.

TkDodo avatar May 25 '24 16:05 TkDodo

exactOptionalPropertyTypes

I agree that would help to prevent explicitly passing undefined as value, but it not the case here. The ones using this custom hook do not pass undefined like useSomeQuery({ retry: undefined }), they do useSomeQuery({}). It is the way speading objects works in JS that results in retry variable inside useSomeQuery being defined (with value undefined).

Basically

function useSomeQuery({ retry }: {retry?: number}) {
  return useQuery({
    retry
  })
}

And

function useSomeQuery(props: {retry?: number}) {
  const retry = props.retry;
  return useQuery({
    retry
  })
}

is the same.

vdvukhzhilov avatar May 26 '24 12:05 vdvukhzhilov