Infinite refetching when using usePrefetchQuery (ensureQueryData) with Suspense

[NMinhNguyen]

Describe the bug

It appears that when using Suspense and the documented usePrefetchQuery (which is essentially queryClient.ensureQueryData() in the render phase), if the query function throws an error, it will keep getting retried and the error boundary is never shown.

Your minimal, reproducible example

https://codesandbox.io/p/devbox/funny-cohen-t4wjhh?file=%2Fsrc%2Fmain.tsx%3A54%2C28

Steps to reproduce

1. Open the sandbox
2. Notice it keeps showing the Suspense fallback
3. If you open the network tab, you’ll see lots of requests getting fired
4. Comment out usePrefetchQuery and you’ll see that the error boundary renders an error

Expected behavior

I’d expect it not to keep retrying the query and for the error boundary to render an error

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

• OS: macOS
• Browser: Chrome
• Version: 124

Tanstack Query adapter

react-query

TanStack Query version

5.36.0

TypeScript version

No response

Additional context

No response

[TkDodo]

yeah I think this is what happens when you think the rules of react don't apply / we can bend them. Doing something during render isn't really safe. In case of ErrorBoundaries specifically, react re-renders the component another time (I think to get the error stack trace or something). In that case, the side-effect matters, because it will trigger another fetch, thus going infinite.

To really only trigger it once, we would need to wrap the call to ensureQueryData into an if statement:

if (!queryClient.getQueryState(arg.queryKey)) {
  queryClient.ensureQueryData(arg);
}

because we need to only call it once per query, not per query data. As you've seen, in case of error, the query is created, but data isn't there yet.

I think at least, we should change the docs around this pattern, or maybe even remove it completely from the docs. @Ephem FYI

[NMinhNguyen]

Thanks for your quick reply!

I think at least, we should change the docs around this pattern, or maybe even remove it completely from the docs. @Ephem FYI

Do you think it also makes sense to add an empty .catch handler to ensureQueryData to avoid unhandled promise rejections, especially in the render phase?

if (!queryClient.getQueryState(arg.queryKey)) {
  queryClient.ensureQueryData(arg).catch(() => {
    // Avoid unhandled promise rejection
  });
}

The docs mention this hook eventually moving into the library, so perhaps a battle-tested version of it could be exported at some point?

I think that prefetching in render is fine, e.g. https://react.dev/reference/react-dom/prefetchDNS does that, but what’s not fine is setState getting called repeatedly as part of this process somewhere 😅

[TkDodo]

Do you think it also makes sense to add an empty .catch handler to ensureQueryData to avoid unhandled promise rejections, especially in the render phase?

yes, that looks good.

The docs mention this hook eventually moving into the library, so perhaps a battle-tested version of it could be exported at some point?

Maybe at some point; for now, I think it's enough if we update the example to the version you have proposed. I think ideally, prefetching would happen on route level or in event handlers instead of during render.

Would you like to update the docs?

[Ephem]

Thanks for the bug report and the ping, makes sense this fails and I agree we should update the docs.

The reason it fails is a bit more straightforward then mentioned above though. It's not because of an extra render because of the ErrorBoundary or anything like that, it's simply because ensureQueryData (and fetchQuery that it relies on) never throws in render and ErrorBoundary only catches errors thrown in render. The endless loop is simply from endless Suspending by the useSuspenseQuery I think.

(The throwOnError is by necessity part of the react-query package, not part of the core package where fetchQuery lives.)

I think ideally, prefetching would happen on route level or in event handlers instead of during render.

I agree with this, but I've also seen enough codebases where this is hard to implement and a (working) usePrefetch is at least a step in the right direction. 😄

[NMinhNguyen]

The endless loop is simply from endless Suspending by the useSuspenseQuery I think.

If you comment out the prefetch, the suspense query does settle though.

[Ephem]

If you comment out the prefetch, the suspense query does settle though.

Oh yeah, the ensureQueryData is definitely involved. I think I saw the useSuspenseQuery retry 3 times before failing. What I believe is happening (but haven't verified all the way) is that the ensureQueryData just fires the request, it doesn't care about retries or anything. Then useSuspenseQuery sees there is already a fetch in flight and throws that existing promise. When that resolves, we rerender, triggering a new fetch via ensureQueryData, etc.

At least something along those lines.