Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

invalidateQueries not invalidating outgoing first request #8530

Open
lgenzelis opened this issue Jan 13, 2025 · 7 comments
Open

invalidateQueries not invalidating outgoing first request #8530

lgenzelis opened this issue Jan 13, 2025 · 7 comments

Comments

@lgenzelis
Copy link
Contributor

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

@TkDodo
Copy link
Collaborator

TkDodo commented Jan 14, 2025

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

if (this.state.data !== undefined && fetchOptions?.cancelRefetch) {
// Silently cancel current fetch if the user wants to cancel refetch
this.cancel({ silent: true })

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 ?

@lgenzelis
Copy link
Contributor Author

lgenzelis commented Jan 14, 2025

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
Copy link
Contributor Author

lgenzelis commented Jan 14, 2025

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. 🤔

@TkDodo
Copy link
Collaborator

TkDodo commented Jan 14, 2025

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.

@lgenzelis
Copy link
Contributor Author

lgenzelis commented Jan 14, 2025

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 😅

@TkDodo
Copy link
Collaborator

TkDodo commented Jan 14, 2025

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 :)

@lgenzelis
Copy link
Contributor Author

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? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants