Enabling a disabled query does not unsuspend

[hector]

Describe the bug

I am using the new mechanism to suspend explained here: https://tanstack.com/query/latest/docs/framework/react/guides/suspense#using-usequerypromise-and-reactuse-experimental

Disabled queries return a pending promise (which is ok), but the promise does not resolve when the query is enabled afterwards.
This happens because the query is not refetched when enabled changes from false to true.

Your minimal, reproducible example

https://codesandbox.io/p/devbox/mystifying-mopsa-qwhjwx?workspaceId=ws_5WeinpZuX6zrSSrhdSgo5C

Steps to reproduce

Click the button to enable the query

Expected behavior

When the query is enabled it should fetch and the promise should finally resolve

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

• OS: macOS
• Browser: Chrome
• Version: 131.0.6778.205

Tanstack Query adapter

react-query

TanStack Query version

@tanstack/react-query@5.62.11

TypeScript version

typescript@5.4.5

Additional context

No response

[TkDodo]

enabled is an option on the observer - but you unmount the observer completely when the Suspense fallback is rendered instead of the Example component. Since the promise was already passed to use, but the promise is also stored on the observer, there isn’t really a way for us to tell the Suspense Boundary that the promise has been resolved.

I tried switching to a conditional use:

const data = enabled ? use(promise) : null

This works a bit better - the component renders with null data when disabled, and when we trigger towards enabled, we suspend because we use the promise.

However, it still doesn’t trigger the fetch. The reason is that because a cache entry exists already, we can’t trigger the fetch during render without violating the rules of react:

query/packages/react-query/src/useBaseQuery.ts

Lines 146 to 150 in 6c2a055

	const promise = isNewCacheEntry
	? // Fetch immediately on render in order to ensure `.promise` is resolved even if the component is unmounted
	fetchOptimistic(defaultedOptions, observer, errorResetBoundary)
	: // subscribe to the "cache promise" so that we can finalize the currentThenable once data comes in
	client.getQueryCache().get(defaultedOptions.queryHash)?.promise

isNewCacheEntry is false, so we just subscribe to the query cache promise. However, since the only thing that would actually trigger the fetch is the useEffect that updates the options inside the Example component, we never get to trigger the fetch because this effect doesn’t run as the Example component is unmounted as a whole due to suspense 🙃 .

The only thing that works right now is to pass the promise as a prop to a child component, where only the child component then suspends, keeping the component with useQuery mounted. Here’s my fork:

https://codesandbox.io/p/devbox/hopeful-flower-f6t8qp?workspaceId=ws_XiBCv7MgHSszKyXHXG5ray

@KATT what do you think ? If we changed our code to call fetchOptimistic during render whenever we have no data yet and e.g. have no promise in the cache yet, I think we could get it to work. However, it’s a slippery slope to call side-effects during render unless they are guaranteed to be not observable side-effects. This is guaranteed right now because we only run it if the cache entry gets created by useQuery, which means there can’t be any other observer. The widening would throw this out the window. @Ephem probably also has opinions on this :)

[Ephem]

I don't have many opinions really, but I do have observations. Your explanation why this is happening is great and makes total sense, but if you only look at the public API, this still seems like a bug. I do think avoiding observable side effects in render is something we should strive for though, so where does that leave us?

On the one hand, maybe this should lead us to question the current implementation, why are we triggering the fetch in the observer and not on the outside of it? I do think Suspense has been pushing the current RQ implementation for a while now. (It's interesting that <Suspense> could actually be seen as a new type of observer that only observes a single promise)

On the other hand, I think we have something like three cases:

• The query is already in the cache and has data - Enabling should resolve the promise to that
• The query is already in the cache and pending
  • It's pending because it is already fetching - Enabling should resolve with the data from that when it finishes
  • It's pending because it was previously disabled - Maybe in this case triggering the fetch in render is still safe because it wont be observable?

I don't have strong arguments why exactly, but I am starting to feel the current model/implementation chafing a bit.

[KATT]

I don't see a big issue with the current behaviour - ideally, we'd reuse the same thenable and resolve it when it's enabled, but with the React compiler / proper memoization there'd be little impact.

For the current user experience, this seems fine to me: https://codesandbox.io/p/devbox/priceless-surf-ygrqd9?file=%2Fsrc%2Findex.tsx%3A61%2C19

Modified @TkDodo's example to always render the <Data>-component.

tl;dr - I think it's a legit bug, but a quite low-priority one that I don't see have big impact?

[KATT]

I tried adding a test for this in #8501 and it seems we're doing it right already?

[KATT]

Oh yeah, it's because the promise is colocated in the same component, I missed looking at the original reproduction 🤔

Agreed this is a bug

[Ephem]

FWIW I agree this specific bug is a bit niche and there's indeed a good userland workaround, hoist the useQuery to outside the Suspense boundary as you demonstrate, so I agree it's not super high priority and better to fix "right" than "fast". 😄

[hector]

I don't know about the internals of the library but I believe this is how it should work:

1. First render:
   On the first render of Example component: query is disabled, query.promise is pending and the component suspends
2. Second render:
   When enabled prop is set to true, Example component renders again but "from zero", meaning that since it was suspended you need to think as if all the hooks (useRef, useEffect, etc...) run for the first time. No state from the first render is there.
   This is what React docs say:
   React does not preserve any state for renders that got suspended before they were able to mount for the first time. When the component has loaded, React will retry rendering the suspended tree from scratch.
   In this render, you should make useQuery return a new promise that will resolve once the data is fetched
   Again, in this render, the promise will be pending in the beginning and the component will suspend
3. Third render:
   When the promise finally resolves because the data has been fetched, React will render the component from scratch but since the query will be in the cache it will just read it and render normally.

[hector]

By the way I found another workaround for userland, I am sharing it here:
https://codesandbox.io/p/devbox/morning-tree-drm5xf?workspaceId=ws_5WeinpZuX6zrSSrhdSgo5C

In short, it consists on issuing a refetch if the query becomes enabled and hasn't been fetched before:

if (enabled && isPending) refetch();

Probably has its issues, point them out if needed.

[hector]

Also, in case context helps, what I am trying to do is simply extending my hooks to be able to do this:

const { data: user } = useQuery(...).suspense();

suspense function is basically a shortcut to:

React.use(useQuery(...).promise)

So then I have my custom hooks and can use them with or without suspense:

const user = useUser().data; // user might be null
// or
const user = useUser().suspense().data; // user cannot be null

Then I can easily use suspense but with parallel fetching:

// Start both queries
const userQuery = useUser();
const articlesQuery = useArticles();

// Suspend until both queries have data
userQuery.suspend();
articlesQuery.suspend();

// Natively it would have been:
React.use(Promise.all([userQuery.promise, articlesQuery.promise]));

The thing is that all this logic works inside hooks, I cannot create suspense boundaries to make it work.

[TkDodo]

but I believe this is how it should work:

@hector what you describe is not how it works. In the second render, enabled is true, but fetching will happen in an effect. That effect never executes because use(promise) will unmount the component that has the effect that would trigger the fetch. experimental_prefetchInRender only triggers a fetch during render IF there is no cache entry yet, which is only true on the first render (where we don’t want a fetch due to enabled:false), but not on the second render, as the entry exists already. As it exists, it can potentially be observed by another component, so strictly speaking, the rules of react don’t allow us to trigger a fetch in render here.

It's pending because it was previously disabled - Maybe in this case triggering the fetch in render is still safe because it wont be observable?

@Ephem I think I disagree here. The cache entry could definitely be observed by a different component, for example, the devools will show it. We could try to bend the rules here but I’m not really a fan.

---

Generally, it seems that relying on useQuery to trigger the fetch, while at the same time, calling use(promise) in the same component unconditionally feels like an anti-pattern to me. This is literally the same as what useSuspenseQuery does at the moment, and it’s what we want to move away from because of potential waterfalls. There’s also a reason why enabled doesn’t exist on useSuspenseQuery, but with use + useQuery, it’s kinda “coming back”.

I think all of this would be easier for us if use(observable) would exist. Maybe it will exist in the future, but it doesn’t right now. The best thing would be to trigger the fetch once the promise is passed to use, because that’s when we can guarantee that the component will suspend, but we don’t know about that either. Proxies also don’t help here because destructing promise from useQuery is enough to trigger the read.

---

I’m all in all more inclined to leave this pattern as-is, and document it, or even go the opposite direction and remove experimental_prefetchInRender to enforce splitting the promise initialization and suspension.

[hector]

@TkDodo I see, thanks for the explanation.

experimental_prefetchInRender only triggers a fetch during render IF there is no cache entry yet, which is only true on the first render (where we don’t want a fetch due to enabled:false), but not on the second render, as the entry exists already. As it exists, it can potentially be observed by another component, so strictly speaking, the rules of react don’t allow us to trigger a fetch in render here.

What if you did not create the cache entry for a disabled query? Seems like it would work fine then.

[KATT]

I need to think about this issue some more, but, I kinda think that we should have a .promise that always rejects whenever the query is disabled - having a spinner that loads forever is a bit confusing DX which will easily creep out into apps.

I'm not convinced what the right thing to do is but I fixed it so it works according to current expectations in #8501.

[Ephem]

The cache entry could definitely be observed by a different component, for example, the devools will show it. We could try to bend the rules here but I’m not really a fan.

This is very true, I was thinking nothing could be observing the data in this specific circumstance, but you are right the query itself could be observed.

Generally, it seems that relying on useQuery to trigger the fetch, while at the same time, calling use(promise) in the same component unconditionally feels like an anti-pattern to me. This is literally the same as what useSuspenseQuery does at the moment, and it’s what we want to move away from because of potential waterfalls.

It is only an anti-pattern if you have not prefetched the query elsewhere, which is the common pattern with loaders. So if we want to at some point deprecate useSuspenseQuery in favour of this pattern, I think we'd need to fully support useQuery+use in the same component?

There’s also a reason why enabled doesn’t exist on useSuspenseQuery, but with use + useQuery, it’s kinda “coming back”.

Can you remind me about the reason again? I know there are some implementation trickiness involved here, but I kind of think we should support this long term if we can. I've been struggling with this myself, how would we solve dependent queries with Suspense otherwise? I don't think I've seen a good pattern currently. To be fair I think this is trickier with useSuspenseQuery than useQuery+use, because:

I kinda think that we should have a .promise that always rejects whenever the query is disabled

@KATT Yes! I think this makes perfect sense. Trying to read from a disabled query does feel like it should be an error. Since a common reason for disabling a query is that it's a dependent one, it makes sense to me to have to make that check both when you declare the fetch and when you try to read. This also solves the type issue that data should always be defined with Suspense.