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

Invalidate/Refetch Promises not resolving when paused #5054

Closed
alexanderdavide opened this issue Feb 28, 2023 · 4 comments
Closed

Invalidate/Refetch Promises not resolving when paused #5054

alexanderdavide opened this issue Feb 28, 2023 · 4 comments
Labels
bug Something isn't working package: query-core v5

Comments

@alexanderdavide
Copy link

alexanderdavide commented Feb 28, 2023

Describe the bug

When invalidating/refetching a query without internet connection, the corresponding promises don't resolve until the connection reestablishes and the query itself resolves.

Your minimal, reproducible example

https://codesandbox.io/s/react-query-refetch-invalidate-promises-not-resolving-when-paused-hsk41e

Steps to reproduce

  1. Use the provided Code Sandbox.
  2. Mock offline behavior using React Query Devtools.
  3. Click on Invalidate/Refetch buttons.
  4. Understand that the promises aren't resolving. You can refer to the logs in console.
  5. Disable mocking offline behavior.
  6. Understand that the promises now resolve.

Expected behavior

I suspect invalidateQueries to only mark queries as stale which is also what Query Invalidation documentation explains. This can lead to refetching of queries but invalidateQueries isn't bound to refetching or its success. In so far, invalidateQueries should resolve after having marked the query as stale. Since I understand refetch just as a more 'aggressive' invalidateQueries, it should behave the same.

How often does this bug happen?

Only the first time when fetching without internet. Fetching again while the previous promise still isn't resolving, results in a resolved promise.

Screenshots or Videos

No response

Platform

  • OS: Linux, Android
  • Platform: Browser, React Native
  • 110.0.5481.178, 0.68.2

TanStack Query version

4.24.10

TypeScript version

4.3.5

Additional context

No response

@alexanderdavide alexanderdavide changed the title [React Query] Invalidate/Refetch Promises not resolving when paused Invalidate/Refetch Promises not resolving when paused Feb 28, 2023
@TkDodo
Copy link
Collaborator

TkDodo commented Feb 28, 2023

I suspect invalidateQueries to only mark queries as stale which is also what Query Invalidation documentation explains. This can lead to refetching of queries but invalidateQueries isn't bound to refetching or its success. In so far, invalidateQueries should resolve after having marked the query as stale.

That's not true, the docs say two things:

When a query is invalidated with invalidateQueries, two things happen:

It is marked as stale. This stale state overrides any staleTime configurations being used in useQuery or related hooks
If the query is currently being rendered via useQuery or related hooks, it will also be refetched in the background

In the code, this is implemented as:

  • mark all matching queries as stale
  • call refetchQueries and refetch all active queries:

return notifyManager.batch(() => {
this.#queryCache.findAll(filters).forEach((query) => {
query.invalidate()
})
if (filters.refetchType === 'none') {
return Promise.resolve()
}
const refetchFilters: RefetchQueryFilters = {
...filters,
type: filters.refetchType ?? filters.type ?? 'active',
}
return this.refetchQueries(refetchFilters, options)
})

That's the reason why it returns a Promise. If it should resolve after having marked the queries as stale, it could be a synchronous function, as marking queries as stale is not an async operation.

Since I understand refetch just as a more 'aggressive' invalidateQueries, it should behave the same.

This also doesn't make a lot of sense, conceptually. If invalidateQueries should resolve after marking queries as stale in your opinion, and refetch should "work the same", that kinda implies that refetch also marks them as stale, which it does not. It would also mean you'd expect both functions to return immediately, which would break a lot of things, for example await prefetchQuery in SSR or ensureQueryData for router loader integration.


right now, the imperative fetch methods work the same as useQuery does: They give you a Promise that you can await, so that you can do navigations after successful invalidation.

One thing we could think about is returning a resolved promise immediately if you cannot fetch (because you are offline), but are required to (because of networkMode). We've done a similar fix lately for paused mutations continuation:

continue: () => {
const didContinue = continueFn?.()
return didContinue ? promise : Promise.resolve()
},

This would certainly work for invalidateQueries and refetchQueries, because we aggregate the Promises into a Promise<void> anyways. But it wouldn't work for fetchQuery (and thus prefetchQuery, as it uses the same method under the hood), because we return a Promise<TData> here, meaning you'd expect data available after awaiting that one. So we have no choice but to wait for the real Promise to be done.

We could make a distinction here with a flag but it's something I'd need to think about

@alexanderdavide
Copy link
Author

Okay, so I had the misconception that

If the query is currently being rendered via useQuery or related hooks, it will also be refetched in the background

is a side note of an effect happening after calling invalidateQueries rather than being directly bound to it.

In the code, this is implemented as:

  • mark all matching queries as stale
  • call refetchQueries and refetch all active queries:

I understand and yes, I better should have familiarized myself with the source code.

It would also mean you'd expect both functions to return immediately, which would break a lot of things

There's obviously a lot in React Query I haven't touched so far. Excuse my yet superficial understanding.

One thing we could think about is returning a resolved promise immediately if you cannot fetch (because you are offline), but are required to (because of networkMode).

That's basically what I wanted to bring up for discussion. This would certainly help in my case: I'm using the workaround for RefreshControl flickering discussed in #2380 and if the user refreshes when being offline, the indicator doesn't stop until the promise is resolved.

After resolving immediately when offline, would the mutation/query still be queued for execution when coming back online or canceled?

@TkDodo
Copy link
Collaborator

TkDodo commented Mar 1, 2023

There's obviously a lot in React Query I haven't touched so far. Excuse my yet superficial understanding.

No worries. Thank you for the bug report. You're definitely on to something.

After resolving immediately when offline, would the mutation/query still be queued for execution when coming back online or canceled?

yes, provided that refetchOnReconnect hasn't been turned off.


I'm inclined to fix this for refetchQueries, invalidateQueries and prefetchQuery, as they already return Promise<void>, but not for fetchQuery, because we need data. Have to think about the best implementation though :)

@TkDodo TkDodo added bug Something isn't working package: query-core labels Mar 1, 2023
@alexanderdavide
Copy link
Author

I'm inclined to fix this for refetchQueries, invalidateQueries and prefetchQuery, as they already return Promise, but not for fetchQuery, because we need data. Have to think about the best implementation though :)

Sounds good! I'm happy this bug report was somewhat helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working package: query-core v5
Projects
None yet
Development

No branches or pull requests

2 participants