fix(infiniteQuery): do not consume AbortSignal unless user has consumed it

[TkDodo]

fixes #3492

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/tanstack/react-query/8KTq9NWRqnH7uacZU4iAt3tLBHoz
✅ Preview: https://react-query-git-fork-tkdodo-feature-3492-infini-b5100b-tanstack.vercel.app

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 0cec964:

Sandbox	Source
jovial-voice-tjdd3s	Issue #3492

[TkDodo]

@jcready I applied your patch here. There is one test failing:

should stop fetching additional pages when the component is unmounted

so we now don't stop fetching once the component unmounts. This should happen independent of the AbortSignal. Any ideas?

[jcready]

Hmmm. After looking at it for awhile I don't think there's a clean way to accomplish this w/o reverting part of #2996 such that (at least internally) the .cancel() method is still added to the promise. The problem is that we (internally) need to be able to abort the infinite query behavior's abort controller when the abort controller from query.ts is aborted. But when the internal promise .cancel() method was removed it meant that the only way for the infinite query behavior to get notified of the cancellation above was to, well, consume the abort signal.

So either there needs to be some way for infinite query behavior to reset abortSignalConsumed or it needs a way to get notified about the query.ts cancellation w/o using the abort signal in the queryFnContext.

[jcready]

Hmmm. After looking at it for awhile I don't think there's a clean way to accomplish this w/o reverting part of #2996 such that (at least internally) the .cancel() method is still added to the promise. The problem is that we (internally) need to be able to abort the infinite query behavior's abort controller when the abort controller from query.ts is aborted. But when the internal promise .cancel() method was removed it meant that the only way for the infinite query behavior to get notified of the cancellation above was to, well, consume the abort signal.

So either there needs to be some way for infinite query behavior to reset abortSignalConsumed or it needs a way to get notified about the query.ts cancellation w/o using the abort signal in the queryFnContext.

[TkDodo]

I think I know what the problem is:

• in v3, we attached a cancel method that was always called when the query unmounts. code looked like this:

        const finalPromiseAsAny = finalPromise as any

        finalPromiseAsAny.cancel = () => {
          cancelled = true
          abortController?.abort()
          if (isCancelable(promise)) {
            promise.cancel()
          }
        }

the important part is that always set cancelled: true, but only cancelled the original promise if it also had a cancel function.

in v4, we tried to do the same with Abort Signals. However, this lead to always consuming the signal, but it also made sure that we had cancelled: true.

now with this change, we only set cancelled: true when the signal is consumed. if we never do that, we never stop fetching even if components unmount.

I think the issue was partly there on v3 as well. because the promise always had a cancel function, we always reverted the state if the query unmounts instead of keeping fetching and putting the result into the queryCache, which is what we do with useQuery. This seems to be actually alright / wanted, because imagine we have 4 pages, and we re-fetch the first 2, then we unmount.

What currently happens is that we revert the state, discarding the 2 refetched pages. What we could also do is continue to fetch all pages, and put them in the cache for later use.

useQuery decides between the two cases by checking if the abort signal has been consumed. if yes, we also discard and revert. if no, we continue the fetch and put them in the cache for later use.

So with an infinite query, what we cannot do is keep the first two fetched pages in the cache for later use and also stop fetching further pages, because that would give us a partially invalid state.

What I'm trying to say is: It might actually be okay to treat infinite queries as "abort signal consumed", because we always want to revert to the previous state to avoid partially invalid states. That is, if we want to keep the behaviour from the test that says: should stop fetching additional pages when the component is unmounted. Because to do this, we need to revert anyways...

[TkDodo]

@jcready the more I think about it, the more I think that your fix is actually correct, including the failing test. If the user doesn't consume the AbortSignal, and the component unmounts while it is fetching 5 pages from the infinite query - why should it stop? I could see the 5 fetches as "one batch", because it goes to the same cache entry.

With a normal query, we also don't stop fetching, we just put the result in the cache. We only stop fetching if the AbortSignal is consumed.

With infinite queries, it seems like we always "aborted" ongoing batches. 2 pages have been fetched, we don't fetch the other 3 - so we have to discard the result and revert back.

If we want to keep this behaviour, we can close the PR. But it would at least mean that we'd need to document it here, because the current text is not true for infinite queries, as the data will not be available in the cache (also not in v3!):

By default, queries that unmount or become unused before their promises are resolved are not cancelled. This means that after the promise has resolved, the resulting data will be available in the cache. This is helpful if you've started receiving a query, but then unmount the component before it finishes. If you mount the component again and the query has not been garbage collected yet, data will be available.

If we want to change the behaviour to be in line with that documentation, I think we need to continue to fetch all pages, and then put them in the cache. I think your PR does this already, so we'd just need to change the existing test and maybe add a new one for infinite queries + abort signal.

What do you think?

[jcready]

@TkDodo I agree with that. Though I do think with some modification to the context it might also be possible to keep the "old" behavior. For example one relatively simply way to do this would be to pass the plain AbortSignal instance to the context as something like cancellationSignal where accessing it wouldn't result in abortSignalConsumed being changed to true. I'm not sure if the cancellationSignal should be passed to both the FetchContext and QueryFunctionContext, or perhaps just the FetchContext.

// query.ts

// Trigger behavior hook
const context: FetchContext<TQueryFnData, TError, TData, TQueryKey> = {
  fetchOptions,
  options: this.options,
  queryKey: this.queryKey,
  state: this.state,
  fetchFn,
  meta: this.meta,
  cancellationSignal: abortController?.signal, // new
}

addSignalProperty(context)

this.options.behavior?.onFetch(context)

This would then allow infiniteQueryBehavior to add the event listener to cancellationSignal which could set its cancelled variable to true. Obviously this would also require the docs to be updated and require a bit more code, but it could enable react-query to keep the existing test passing and prevent infiniteQueryBehavior from "consuming" the abort signal which would revert the cache.

I'm happy with either outcome.

[TkDodo]

Tough I do think with some modification to the context it might also be possible to keep the "old" behavior.

I think we have the "old" behaviour right now on v4. The fetches get "cancelled" and that internal state is reverted. This has the (known) issue with react 18, strict effects in strict mode and query cancellation that you'll see two fetches, because react fires the subscription twice. This is not an issue without cancellation, because the fetches will just be deduped.

[TkDodo]

but it could enable react-query to keep the existing test passing and prevent infiniteQueryBehavior from "consuming" the abort signal which would revert the cache.

hm, what I don't get is: If we don't revert the cache, but we keep the current behaviour and abort after 2 of 5 page fetches - how could that work / what state would the query be in? I think we have to either finish all the fetches and put them in the cache OR abort the fetches and revert.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
react-query	✅ Ready (Inspect)	Visit Preview	May 25, 2022 at 4:50PM (UTC)

[codecov]

Codecov Report

Merging #3507 (0cec964) into beta (270efe6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             beta    #3507   +/-   ##
=======================================
  Coverage   97.11%   97.11%           
=======================================
  Files          49       49           
  Lines        2388     2392    +4     
  Branches      707      708    +1     
=======================================
+ Hits         2319     2323    +4     
  Misses         67       67           
  Partials        2        2           

Impacted Files	Coverage Δ
src/core/infiniteQueryBehavior.ts	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 270efe6...0cec964. Read the comment docs.

[TkDodo]

I think the new behaviour is fine, as this is also what is documented (queries will just continue to fetch). If you want cancellation, you have to use the AbortSignal. Also, it fixes the reported issue, so I'm going to merge this.

[tannerlinsley]

🎉 This PR is included in version 4.0.0-beta.16 🎉

The release is available on:

• npm package (@beta dist-tag)
• GitHub release

Your semantic-release bot 📦🚀