query icon indicating copy to clipboard operation
query copied to clipboard

fetchQuery and useQuery options

Open artem-malko opened this issue 3 years ago • 15 comments

Describe the bug

Actually, I'm not sure, that it is a bug.

So, if I have a fetchQuery and my own query (which uses useQuery) for the one entity (newsItem in the example), and my own query has specific options (fetchQuery uses default options, own query overrides cacheTime and staleTime). My own query executes on the NewsItem component mount. But, if a data for newsItem is fetched via fetchQuery, my own query is not working for that case. So, in a queryCache that query has options from default, not from my own query.

I understand, that my own query won't start its fetcher, if the data is in the cache. But it was quite unexpected, that options from my own query are not used. Check out the example from codesandbox.

Your minimal, reproducible example

https://codesandbox.io/s/react-query-fetchquery-usequery-g18l4y

Steps to reproduce

  1. Go to the codesandbox example
  2. Click to the button with text: "fetch newsitem with id ..." In the ReactQuery devtools you will see, that there is a query with cacheTime and staleTime from default settings.
  3. Click to the button with text "Toggle newsItemComp". The NewsItem will be shown.
  4. Options from the query "useNewsItem", which is used there won't be applied.

Expected behavior

As a user, I expected that useQuery will rewrite options of the query in the reactQuery cache but i am seeing, that options from fetchQuery is used (default options).

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

It doesn't depend

react-query version

4.0.0-beta.23

TypeScript version

4.7.3

Additional context

No response

artem-malko avatar Jun 13 '22 15:06 artem-malko

hmmm, staleTime for example is definitely applied, because if you click the toggle button, you can see that the query shows up as fresh in the devtools, not stale.

cacheTime is an interesting case. It seems to be set upon creation, and then only updated once a fetch occurs. If you add a refetch button:

<button onClick={() => queryResult.refetch()}>refetch</button>

you can see that the cacheTime gets applied upon refetching. It seems that we only set observer level options (like staleTime) upon mounting of a new observer, but not cache level properties, like cacheTime:

https://github.com/TanStack/query/blob/e01a7bb7e76e85115d45689f555ad2e8bf8b015c/src/reactjs/useBaseQuery.ts#L97-L101

I think this is a bug, but not sure what problems we would introduce if we also updated cache level options in that effect 🤔

TkDodo avatar Jun 13 '22 15:06 TkDodo

hmmm, staleTime for example is definitely applied, because if you click the toggle button, you can see that the query shows up as fresh in the devtools, not stale.

Yes, it works in case of my own query usage. But, if I click to the "fetch newsitem with id ..." button, and after that show the NewsItem component — staleTime won't be 99999, it will be 0, cause of default options from fetcher.

I think this is a bug, but not sure what problems we would introduce if we also updated cache level options in that effect 🤔

Yeah, I have same thoughts) But, I described a real case: fetcher uses the default options, query uses its own options. It is not expected, that data is refetched, when component is mounted, case in its query there are options like:

{
  cacheTime: 99999,
  staleTime: 99999,
}

These numbers are just for an example)

I think, that the latest execution (fetcher or useQuery) should update query options in a queryCache. And it's ok for another story, when useQuery is used before fetcher. What do you think?

artem-malko avatar Jun 13 '22 16:06 artem-malko

Yes, it works in case of my own query usage. But, if I click to the "fetch newsitem with id ..." button, and after that show the NewsItem component — staleTime won't be 99999, it will be 0, cause of default options from fetcher.

no, that's not the case for me. Am I doing something wrong?

  • click fetch item with id
  • wait till the query is resolved
  • click the toggle button

--> query shows up as fresh, which means staleTime is picked up correctly

screencast 2022-06-13 19-05-57

It also works if you're toggling while you are still fetching.

Again, I think that's because staleTime is a per-observer option, and mounting a component that has useQuery will create a new observer, so staleTime should always be respected.

TkDodo avatar Jun 13 '22 17:06 TkDodo

@TkDodo yeah, you're right, my bad) staleTime works like a charm. So, yeah, the main problem here is a cacheTime. Moreover, I do not know, if there are problems with other options.

By the way, how do you do such videos?)

artem-malko avatar Jun 13 '22 17:06 artem-malko

I do not know, if there are problems with other options.

I think all observer options should be fine:

https://github.com/TanStack/query/blob/f77f6f4f924d7aa1107cbb9bb2dfd09f81beddd8/src/core/types.ts#L100

and all options on the query cache itself should be affected:

https://github.com/TanStack/query/blob/f77f6f4f924d7aa1107cbb9bb2dfd09f81beddd8/src/core/types.ts#L65-L97

Also, the bug only manifests if you're having a staleTime set, because if staleTime is zero, the mount would trigger a background refetch, which would in turn cause the options to update:

https://github.com/TanStack/query/blob/270efe6283ca7a6d0397ca048ea1869c3fa5e743/src/core/query.ts#L339-L341

By the way, how do you do such videos?)

A tool called monosnap: https://monosnap.com/

TkDodo avatar Jun 13 '22 17:06 TkDodo

Also, the bug only manifests if you're having a staleTime set, because if staleTime is zero, the mount would trigger a background refetch, which would in turn cause the options to update:

There is an exception from this rule — refetchOnMount: false in a defaultOptions. As I understand, even staleTime is zero, a query won't be refetched on mount.

artem-malko avatar Jun 13 '22 17:06 artem-malko

So what we could try is to have the function setOptions on the QueryObserver just call this.currentQuery.setOptions(this.options) so that whenever we set observer level options, the one on the query are updated as well. setOptions is currently private so we'd need to make it public. What do you think? @artem-malko ?

TkDodo avatar Sep 18 '22 19:09 TkDodo

@TkDodo do you mean something like that:

const { setOptions } = useQuery();

setOptions({
 ...
})

artem-malko avatar Sep 19 '22 11:09 artem-malko

no what I mean was, when setOptions on the observer is called:

https://github.com/TanStack/query/blob/7e51776bff00b2f6f8496a7a64b3a048238a4ee7/packages/query-core/src/queryObserver.ts#L143

we could also do:

this.currentQuery.setOptions(this.options)

probably somewhere here:

https://github.com/TanStack/query/blob/7e51776bff00b2f6f8496a7a64b3a048238a4ee7/packages/query-core/src/queryObserver.ts#L178-L179

This would ensure that every time when the observer options are updated (where we've identified that this works well, e.g. with `staleTime' ) that query options are also picked up correctly, even if no fetch is happening.

TkDodo avatar Sep 19 '22 12:09 TkDodo

Aaa, now I understand) Looks really nice)

artem-malko avatar Sep 19 '22 13:09 artem-malko

do you want to contribute it @artem-malko ?

TkDodo avatar Sep 19 '22 13:09 TkDodo

I can do it somewhere in the end of October, in 20th. As I understand, it is not a critical right now, so, it can wait till I'll be free?

artem-malko avatar Sep 19 '22 15:09 artem-malko

Sure, thanks 🙏

TkDodo avatar Sep 19 '22 17:09 TkDodo

@artem-malko any updates on this issue? If you're not working on it, please unassign yourself, thanks.

TkDodo avatar Dec 09 '22 19:12 TkDodo

Sorry for the delay) I've started it a couple days ago

artem-malko avatar Dec 11 '22 07:12 artem-malko

I think this is affecting us right now. We use fetchQuery to prefetch a query from within getServerSideProps in Next.js. We then use useQuery with a matching queryKey and any time we run invalidateQueries (either via the dev tools or programmatically), a default query function is used instead of the queryFn that is being passed into the useQuery.

For what it is worth, the issue sometimes fixes itself when I save changes locally and fast refresh kicks in. The invalidation query then picks up the right queryFn.

mareksuscak avatar Aug 08 '23 17:08 mareksuscak

@mareksuscak not sure if this is really related. Can you show a separate codesandbox reproduction for this please?

TkDodo avatar Aug 19 '23 10:08 TkDodo

@TkDodo

Here you go: https://codesandbox.io/p/sandbox/bold-banzai-7hynrj

Steps to reproduce:

  1. Open the React Query DevTools
  2. Click Home in the nav
  3. Click Refresh to reload the framed page
  4. Click Trigger Bug in the nav

Expected: The Query refreshes using the custom query function Current: The Query refreshes using the default query function which doesn't work

Note: Please note that any source changes that trigger the hot reload somehow fix this issue and clicking the Trigger Bug button then works as expected. For us it also works when we click the button a few times in a row after it failed for the first time and used all the retry attempts available. Please let me know if I should open a new ticket.

https://github.com/TanStack/query/assets/613647/c57f5b02-b736-4b73-bd73-eb48b00bd4a1

mareksuscak avatar Sep 05 '23 13:09 mareksuscak

@mareksuscak yeah, it's a different issue :)

we actually have a "protection" to use the options (including queryFn) from the first observer if the query itself doesn't a QueryFunction:

https://github.com/TanStack/query/blob/13b17ee45de3299dd3d3a2dfc3d23ad2f024882a/packages/query-core/src/query.ts#L347-L354

The comment even mentions:

This can happen when the query is hydrated or created with setQueryData.

The problem is that in your case, we have a function set, because you have defined a default query function.

one potential workaround I've found is to explicitly pass queryFn: undefined as hydrate options to avoid that the default queryFn is being picked up for that query:

<Hydrate state={pageProps.dehydratedState} options={{
  defaultOptions: {
    queries: {
      queryFn: undefined
    }
  }
}}>

Here's a working fork of your sandbox: https://codesandbox.io/p/sandbox/bold-banzai-7hynrj?file=/pages/_app.tsx:48,1-54,10

TkDodo avatar Sep 05 '23 13:09 TkDodo

@TkDodo Thanks! Looks like you've shared my sandbox, did you mean to share a link to your fork?

EDIT: Nevermind, I just tried this in our project and it seems to have resolved the issue, thanks again! As an aside, is this fixable so the workaround is not needed? While we do intend to get rid of the default function (that's how we started before realizing custom query fns were the way to go), it'll take us some time to migrate the old code.

mareksuscak avatar Sep 05 '23 21:09 mareksuscak

sorry, here's the working version:

https://codesandbox.io/p/sandbox/practical-wood-9ykp2p?file=/pages/_app.tsx:50,1-55,13

TkDodo avatar Sep 06 '23 06:09 TkDodo

Looks like we can close that issue) I can live with current behaviour and looks like it was just my problem)

artem-malko avatar Oct 22 '23 15:10 artem-malko

reopening this issue because I think it is really an issue, and it was reported again here:

  • https://github.com/TanStack/query/discussions/7056

It seems that, as analyzed, options on the Query itself are not updated, while options on the observer are. That means options like retryDelay are not reactive - if you set them depending on state, they will not change.

I think this is quite confusing indeed, and I think it should be fixed. In the meantime, mutations were also changed to have all options updated, so it would make sense to streamline.

TkDodo avatar Mar 11 '24 16:03 TkDodo