queryFn is called twice in Suspense mode

[simoncarpinter]

Per the title, when using useQuery with { suspense: true }, the supplied query function will be called twice.

See simoncarpinter@cc09457 for details, opening a PR now that adds the test. I haven't yet spotted the actual bug.

[simoncarpinter]

It appears to be somewhere in query.fetch, maybe? handleSuspense in useQuery.js calls query.refetch, which calls query.fetch, which -- if I'm reading things right -- should return the existing promise, and only start a new one if there's nothing preexisting:

// Create a new promise for the query cache if necessary
      if (!query.promise) {
        query.promise = (async () => { ... })()
      }

And yet, if I debug/console.log (lazy), query.promise is undefined in the initial request (expected) but is still undefined in the second request (unexpected) -- despite the fact that debugging afterwards shows query.promise is set in both cases.

Possibly a race condition? Or possibly the query instance is changing, despite the fact it shouldn't be...

[simoncarpinter]

Ah. Removing delete query.promise (https://github.com/tannerlinsley/react-query/blob/master/src/queryCache.js#L394) resolves the issue, but presumably will re-open #216, which is what that line closed. I think my wizardry has been exhausted for the evening.

I'd love to shake my head ruefully and say "ah, it's because the tests are just too damn fast!" but this happens in production too sadly, even with slow APIs.

[tannerlinsley]

This is indeed a funky issue. Still thinking here... keep up the info, it's helping.

[joshuap233]

same problem😂

[ofer-h]

Hey @tannerlinsley
Seems like there is a known issue around this topic as useState / useMemo ... , are not behaving as expected while inside a suspend comp as it may initializer multiple times.

As I'm not 100% sure this is the same case here I will just share here the issues I refer to

useMemo gets called twice with Suspense
useState initializer called multiple times with suspense

[ofer-h]

So, I was able to reproduce this thanks to @simoncarpinter fork and the cuse was this line:

https://github.com/tannerlinsley/react-query/blob/8a6ec246deaef9f4a0c1b32298874e3e74dc93c5/src/utils.js#L134
Triggering suspense with rejected promise causes re-render instead of error boundary.

.
.

As I'm a bit new to react and js in general, I'm not sure why we need throw this Promise in general?

[lindskogen]

I found this answer by Dan the Man, on another github issue that seems to suggest a recommended solution facebook/react#14490 (comment)

[simoncarpinter]

@ofer-h we need to throw the promise because that's how Suspense works :). If a promise is thrown by a child react component, a parent Suspense will catch it and deal with it.

From what I can tell from reading the code, what should be happening is that we start fetching (great). Then, we throw whatever refetch returns -- which should be the original promise as it checks if query.promise exists internally. Instead, it appears that a reference is lost somehow and that we start a new fetch and throw a new promise. refetch is a good name, but if you were to name it for the logic internally it'd be like checkIfWereAlreadyFetchingAndThenThrowThatPromiseButStartANewFetchIfWeArent which is a bit wordy ;).

From my understanding of those linked tickets, the useX stuff getting called twice in Suspense mode is interesting... but it doesn't seem to be the root cause here. I suspect that if we were affected by that design, we should actually see this happen 4 times (maybe? not entirely sure).

@lindskogen that could be useful -- using a ref for storing the promise might be the right thing to do. Good find, if @tannerlinsley doesn't beat me to it, I'll try and hack up a proof-of-concept with that this afternoon.

[simoncarpinter]

Legend, thanks Tanner :). I tried the refs but didn't get very far.

[Grimones]

Hi all
Seems to be 7d786c7 created a regression with not deactivating instances in suspense mode

Please check the codesandbox

As you can see with 1.3.0 it creates 2 instances of query. And when you unmount component they are still active. In my project only 1 gets deactivated and the other one stays active, but I was not able to reproduce it here. It creates problem for me because that query is not cached and it should be removed as soon as it gets inactive but it doesn't happen.

If you change react-query to 1.2.9 version the issue is not reproducible. And also you can see in console that query function was called only once. May be it's something wrong with test simoncarpinter@cc09457

Moved to separate issue as this is not the cause
#459