TanStack / query

🤖 Powerful asynchronous state management, server-state utilities and data fetching for the web. TS/JS, React Query, Solid Query, Svelte Query and Vue Query.
https://tanstack.com/query
MIT License
41.76k stars 2.84k forks source link

Define when suspense queries should be refetched and when they should throw #1059

Closed trevorwhealy closed 3 years ago

trevorwhealy commented 4 years ago

Describe the bug When a user does client-side navigation to return to a previously visited page (react-router-dom), the useQuery hook is not invoked again, even if the query has been marked as invalid. This is an issue because the server value could change in a different browser session or device and the useQuery data value would no longer be in sync.

To Reproduce Created a reproducible environment for testing the bug here: https://codesandbox.io/s/ping-pong-react-query-client-guqhn Important files: src/index.js and src/components/Demo.js

Steps to reproduce the behavior:

  1. Visit Page 1, note the number of pings, and click the increment button.
  2. In the mutation's onSuccess handler, we invalidate the queryCache and navigate to Page 2.
  3. Open a new tab and simulate the ping mutation by simply hitting this URL a few times: -- https://1675g.sse.codesandbox.io/ping-increase
  4. Navigate back to Page 1 via Link or browser back button and note that the value is only incremented by 1.
  5. If you refresh Page 1, you'll notice that the value now reflects the actual server value.

Expected behavior On return to Page 1 from Page 2, I would expect the useQuery hook to fetch the latest server value.

tannerlinsley commented 4 years ago

Can't remember what I changed, but is this example working for you as expected?

https://codesandbox.io/s/ping-pong-react-query-client-forked-pbc4e?file=/src/components/Demo.js

trevorwhealy commented 4 years ago

@tannerlinsley thanks for the really quick reply! I had the same problem - maybe I'm not doing a good job of illustrating.

When I navigate back to Page 1 from Page 2, you'll notice that the Query stays in an Inactive state:

ezgif com-video-to-gif (1)

I was expecting it to re-fetch this query when the component re-mounts.

trevorwhealy commented 4 years ago

I think that would require the query to suspend if it is in the isFetching state in addition to the isLoading state

boschni commented 4 years ago

Think there are two issues here:

  1. Devtools is not notified when observers unsubscribe (if you reselect the query then it shows the correct state).
  2. There is some specific code to prevent refetching when suspense is enabled. This is probably to prevent double fetches because the default stale time is 0 and the hook does not know if it is rendering right after a suspense or after some page switch. Unfortunately this means suspended queries are never refetched. Is my assumption correct @tannerlinsley ?
tannerlinsley commented 4 years ago

Yep I think you’re right on both counts

Tanner Linsley On Sep 19, 2020, 11:17 AM -0600, Niek Bosch notifications@github.com, wrote:

Think there are two issues here:

  1. Devtools is not notified when observers unsubscribe (if you reselect the query then it shows the correct state).
  2. There is some specific code to prevent refetching when suspense is enabled. This is probably to prevent double fetches because the default stale time is 0 and the hook does not know if it is rendering right after a suspense or after some page switch. Unfortunately this means suspended queries are never refetched. Is my assumption correct @tannerlinsley ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

boschni commented 3 years ago

I think we need to define the behavior when using suspense. Currently it will only trigger suspense when it is loading for the first time. Any additional mounts will not trigger a refetch, which does not sound desirable. Maybe we should set a default stale time when suspense is enabled? Then the query will at least be refetched again if it re-mounts after some time. Then the other question is, when do we want to trigger suspense? Only on first load, or also on refetches? Or does it need to be configureable?

flybayer commented 3 years ago

I'm pretty sure that once useEffect() fires, you know for sure that it has fully mounted. From my understanding, useEffect will not be called multiple times during suspense rendering/re-rendering. It only triggers once the render has fully finished.

boschni commented 3 years ago

The main problem is that when a component is mounted, it starts a fetch, and because of the way suspense works, the component immediately gets unmounted. When the fetch is complete, the component will mount again, but because the default stale time is 0, it will trigger another fetch because the data is already considered stale. The double fetching is something we want to avoid.

Currently I can think of these solutions:

  1. Mention in the docs that when using suspense, it is recommended to also set a staleTime to prevent double fetches.
  2. Set some default stale time when suspense is enabled and staleTime is not set.
  3. Require users to assign some application wide unique ID to a query which can be used to track a specific query.

Besides this issue, if users also want to use suspend when refetching, maybe we should add a suspendOnRefetch option?

tannerlinsley commented 3 years ago

1 sounds like the fastest way forward for now.

2 is interesting, but finding a default that would work for everyone would be difficult given all of the async rendering happening with suspense.

3 would definitely solve it, but that DX sounds pretty lame :(

In previous versions of RQ, we had some mutable properties on the query itself that would allow observers to detect if the last fetch was because of a suspension, and thus not trigger the fetch again on mount. I wonder if that same idea could work better now that the observers have become a bit smarter.

Alexandredc commented 3 years ago

Just found out this issue. I'm working on react-native project with react-query. Adding stale time doesn't trigger a new invokation of useQuery when a component is already suspended once :/

nihgwu commented 3 years ago

@tannerlinsley any plan to fix this issue, I think this issue just block the use of suspense with react-query, as the query will never invalidate, that breaks the SWR(stale while revalidate) which is the biggest benefit of react-query

think about it, you have a query in one page, and then navigate to another page, the query should get invalidate and send a request again, but it doesn't when suspense enabled

flybayer commented 3 years ago

Our current workaround in Blitz is to call invalidateQueries on page navigation events.

boschni commented 3 years ago

Is anyone still having trouble with suspense in v3?

flybayer commented 3 years ago

I haven't had a chance to try v3 yet

nihgwu commented 3 years ago

I tried it's fixed in V3

nghiepdev commented 3 years ago

@boschni I had tried 3.11.0 but the suspense issue still again https://github.com/tannerlinsley/react-query/issues/1582

rickyalmeidadev commented 3 years ago

I faced the issue #1582 in version 3.17.0.

I built a simple demo in Code Sandbox where it is possible to reproduce it. Any plans to have it fixed?

TkDodo commented 3 years ago

Is this the same as https://github.com/tannerlinsley/react-query/issues/2367?