query icon indicating copy to clipboard operation
query copied to clipboard

invalidateQueries not invalidating outgoing first request

Open lgenzelis opened this issue 11 months ago • 9 comments

Describe the bug

Per the docs, the default behavior of invalidateQueries should be that if a query gets invalidated while it's being fetched, then the queryFn should be called again (i.e., the query should be restarted). However, currently that's only the case if the request has already been executed in the past (so, if the current query execution is a refech). If we're talking about a query that has never been run before, then invalidating it mid-flight does nothing.

Your minimal, reproducible example

https://stackblitz.com/edit/tanstack-query-igjd7d57?file=src%2Findex.tsx

Steps to reproduce

  1. Open the devtools, so that you can see the console.log output
  2. Go to the MWE
  3. You'll see >>>> query start in the console.
  4. Before the query finishes (5 seconds), click the "Invalidate Query" button as many times as you want. You'll see nothing in the console: this is the bug, you should see >>>> query start each time you click it.
  5. After the query finishes, the behavior is correct: if you click the button, and click it again, you'll see >>>> query start each time you click it.

Expected behavior

The query should be invalidated even if it's fetching for the first time.

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

  • OS: Macos
  • Browser: Chrome

Tanstack Query adapter

react-query

TanStack Query version

5.64.1

TypeScript version

5.7.2

Additional context

No response

lgenzelis avatar Jan 13 '25 14:01 lgenzelis

seems like this is on purpose - cancelRefetch is only taken into account when we have data already:

https://github.com/TanStack/query/blob/5edd617eac7d27ef4850b101f31281d6079c9de2/packages/query-core/src/query.ts#L357-L359

I guess that’s why it’s called cancelRefetch and not cancelFetch 😅 . This is the only place where cancelRefetch is checked, so I think removing the data check should do what you want. However, I think that would be a breaking change.

I wrote the docs where it says:

Per default, a currently running request will be cancelled before a new request is made
When set to false, no refetch will be made if there is already a request running.

because I thought that’s what’s happening, and I always wondered why it’s not named cancelFetch or just cancel.

Maybe the best we can do now is change the documentation to reflect what is actually happening ?

TkDodo avatar Jan 14 '25 07:01 TkDodo

I also felt the name cancelRefetch was misleading, and it ended up being pretty accurate xD I believe the current behavior is pretty unintuitive, and given that it also doesn't do what the docs state, I'd say changing it would be a fix and not a breaking change. But of course that's just my just opinion... if anyone relies on the "buggy" behavior, then yes, it'd be a breaking change for them 🙈 (mandatory xkcd reference ).

lgenzelis avatar Jan 14 '25 12:01 lgenzelis

I wonder why that condition ( if (this.state.data !== undefined) is there in the first place, I can't think of a scenario where this would be desired. Maybe it's there to prevent the case..

  1. A query fires and we show a loading spinner
  2. Before the request finishes, we get e.g. a ws event that invalidates the request
  3. We invalidate the request, so that it is restarted
  4. Iterate 2-3 a few times
  5. Bad Ux: user sees the spinner for a long time

?

If that was the original intention, maybe a better strategy would be to accept the new data (instead of canceling the request) but still fire the new request in the background, as soon as "invalidateQueries" is called. 🤔

lgenzelis avatar Jan 14 '25 12:01 lgenzelis

given that it also doesn't do what the docs state, I'd say changing it would be a fix and not a breaking change.

It was like that since v3, then I added those sentences to the docs somewhen last year only. I think it would also be unintuitive if the setting called cancelRefetch would cancel more than a refetch, like also canceling an initial fetch :/

maybe a better strategy would be to accept the new data (instead of canceling the request) but still fire the new request in the background, as soon as "invalidateQueries" is called.

I can also only guess the intent, but yes, I think getting data earlier is better and frequent calls to an imperative refetch would delay that. Your suggestion would mean we would have to queue up the refetch, which is something that we don’t currently do. It also sounds unrelated to cancelRefetch itself.

TkDodo avatar Jan 14 '25 13:01 TkDodo

I think it would also be unintuitive if the setting called cancelRefetch would cancel more than a refetch, like also canceling an initial fetch :/

Yes, I totally agree there. Maybe a good middle ground would be to add a cancelInitialFetch option, which would be false by default?

Your suggestion would mean we would have to queue up the refetch, which is something that we don’t currently do.

Scrap that suggestion then, it seems it'd increase complexity a lot 😅

lgenzelis avatar Jan 14 '25 15:01 lgenzelis

What’s your use-case for needing to cancel the initial fetch ?

Yes, we could add that option and then later consolidate it to a cancelFetch: boolean | 'refetchOnly' option, where the default could be 'refetchOnly', but I'd like to know why this could be a good idea :)

TkDodo avatar Jan 14 '25 16:01 TkDodo

Sure! My use case (for invalidation in general) is the same: the BE sends ws messages to the web client, which mark that some data is stale.

To use an example from the docs, let's say github exposes an endpoint GET /{repo-id}/stars that gives you the list of stars from the repo, and also sends a ws message 'stars_count_updated'. I want to display the stars count, and always have the latest count value. So, whenever a client receives the 'stars_count_updated', it knows it needs to refresh the count: if a request is already in flight, it needs to be refetched (doesn't matter whether it's a "fetch" or a "refetch"), since the value the server is sending us in the reply might already be stale.

Let me invert the question: when would it be a good idea to cancel a refetch but not a fetch? 🤔

lgenzelis avatar Jan 14 '25 22:01 lgenzelis

Would you like me to give a PR a go? Or do you prefer to ponder this a bit more first? :)

lgenzelis avatar Jan 26 '25 12:01 lgenzelis

Sorry, I’m still thinking about this 😅

TkDodo avatar Apr 06 '25 15:04 TkDodo

I ran into the same issue. My use case for this is. I have a query which has staleTime: Infinity aka it should only refetch on manual invalidation, I invalidate the query as messages come in from a network stream.

vanillacode314 avatar Oct 15 '25 09:10 vanillacode314

I think a good workaround would be to call queryClient.cancelQueries() manually before invalidating it if you want to really ensure that all fetches get cancelled.

I don’t think I want to add this as an option as it’s quite the edge case. In most situations, you will want data as soon as possible.

TkDodo avatar Oct 15 '25 09:10 TkDodo

Thanks for giving it some consideration @TkDodo :) However, I'm still curious and would like to understand the rationale behind the current behavior. Don't you think that having invalidate cancel refetches but not "fetches" is unintuitive? Also, when would that be the desired behavior for the user? ("cancel this query and restart it, but only if it's a refetch, otherwise ignore the call to invalidate")

lgenzelis avatar Oct 15 '25 12:10 lgenzelis

the difference is that when it’s a refetch, you already have data that can be shown to the user. While you do the initial fetch, you show the pending fallback. If you get to an invalidation while you’re doing the initial fetch, you would prolong showing the pending fallback to the user.

the only situation where this seems to matter is websocket subscriptions, where I still don’t fully understand the scenario:

  1. A query fires and we show a loading spinner
  2. Before the request finishes, we get e.g. a ws event that invalidates the request

Shouldn’t the websocket subscription only be set-up after you have fired the initial query? And if you have a global subscription, calling cancelQueries() in one place should be an acceptable workaround.

But we can think about globally changing this for v6. There won’t be any changes in v5 though, sorry.

TkDodo avatar Oct 16 '25 12:10 TkDodo

Oh, no apologies needed Tk, I just wanted to understand your reasoning :) Makes sense, I understand now why the current behavior might be desirable, my only criticism is that I see it as unintuitive.

if you have a global subscription, calling cancelQueries() in one place should be an acceptable workaround.

yep, it's a global subscription in my case. What I don't like about that workaround is that it'd only work for the initial fetch but not for a refetch. So, I'd have to check "is X query refetching? Then invalidate. Otherwise, is it fetching for the first time? Then cancel it". If you ever want to change this, I believe a flag passed to invalidateQueries makes the most sense. It could be something like invalidateQueries(..., { invalidateOnlyIfCachedDataPresent: boolean }), where invalidateOnlyIfCachedDataPresent defaults to true to keep the current behavior as is.

lgenzelis avatar Oct 20 '25 12:10 lgenzelis