`invalidateQueries` inconsistently cancels `fetchQuery`

SebKranz commented 2 months ago

Describe the bug

fetchQuery may throw an CancelledError { silent: true, revalidate: undefined } under the following conditions:

the query is stale (otherwise the fetch won't run).
the query is invalidated at least one tick after the fetch started.
the query is in an active state. For example because a component with useQuery is still mounted.

In the real world, this might happen in combination with react-router, when the user submits a form and then navigates to a new (or the same) page. In my case, we were updating a search param to close a modal after a form submission, which then triggered the loader function containing a fetchQuery call. We're also using a mutation observer[^1] to trigger invalidations, which is why the invalidation happened only after the navigation began.

I fixed the issue by using ensureQueryData instead of fetchQuery. This is the better choice anyways.

Still, this seems like a footgun since it is difficult to catch while testing. Further, if the intended behavior for fetchQuery is to throw a CancelledError on invalidation, it should always do that and not only when a revalidation is triggered.

[^1]: inspired by https://tkdodo.eu/blog/automatic-query-invalidation-after-mutations

Your minimal, reproducible example

https://codesandbox.io/p/sandbox/react-query-cancellederror-on-invalidation-d4g259

Steps to reproduce

See steps above

Expected behavior

Option A: The returned promise should consistently reject, regardless of whether an unrelated observer exists. Option B: The returned promise should consistently resolve with the data returned from this fetch Option C: The QueryClient should treat pending promises as observers, revalidate automatically and always resolve with the result from the latest revalidation.

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

*

Tanstack Query adapter

react-query

TanStack Query version

5.50.1

TypeScript version

No response

Additional context

No response

TkDodo commented 2 months ago

the sandbox seems to be private

SebKranz commented 2 months ago

the sandbox seems to be private

Apologies. Should be public now.

TkDodo commented 2 months ago

The issue seems to be specific to fetchQuery because it's the only imperative API that throws per default (on purpose). I guess we could also reproduce with queryClient.refetchQueries when passing throwOnError: true. Or by simply calling fetchQuery followed by queryClient.cancelQueries()

The Cancellation itself is on purpose, as calling invalidateQueries() will cancel any ongoing fetch. If that comes from fetchQuery, that function will throw.

You could also work around it by passing cancelRefetch: false to invalidateQueries. In that case, it would "pick up" the already running promise instead of cancelling + re-starting.

Option A: The returned promise should consistently reject, regardless of whether an unrelated observer exists.

I'd have to dig deeper to understand why the cancel error is only thrown if there is an active observer 🤔 . I generally agree that it should consistently reject.

Option B: The returned promise should consistently resolve with the data returned from this fetch

The problem is that we can't just catch the error in fetchQuery because what should happen then? We might not have data we can resolve to, so I think this isn't possible.

Option C: The QueryClient should treat pending promises as observers, revalidate automatically and always resolve with the result from the latest revalidation.

That's too big a change, as the only way to get an observer is by creating a QueryObserver, which happens in useQuery.

so I guess we'll have to look into option A :)

SebKranz commented 2 months ago

Thanks for your time.

I'd have to dig deeper to understand why the cancel error is only thrown if there is an active observer 🤔

From my understanding, invalidation doesn't cancel fetches by itself, but it might trigger a revalidation of active queries. The revalidation always cancels the previous fetchQuery call.

https://github.com/TanStack/query/blob/763abd1628c09a7c7513e55cb30e64041ea1ac8d/packages/query-core/src/queryClient.ts#L292C6-L296C6

So the question is: is there any reason why invalidation should not always cancel any ongoing fetch. If there isn't, perhaps we could add Query.cancel({ silent: true }) here:

invalidateQueries(
    filters: InvalidateQueryFilters = {},
    options: InvalidateOptions = {},
  ): Promise<void> {
    return notifyManager.batch(() => {
      this.#queryCache.findAll(filters).forEach((query) => {
        query.invalidate()
+       query.cancel({ silent: true })
      })

      if (filters.refetchType === 'none') {
        return Promise.resolve()
      }

SebKranz commented 2 months ago

A few more thoughts:

if invalidation cancels all queries, ensureQueryData will also be affected. The reason why it's safe to call currently, is that it only triggers a refetch if the Query is new, therefore bypassing that stale & active case of fetchQuery.
~~Another feature that would be impacted is useSuspenseQuery, since it throws the promise returned by Query.fetch(). Currently, if a suspending query invalidates, the fetch continues and the component renders the outdated data. If we implement Option A, suspending components might throw unexpectedly.~~ (EDIT: no, it doesn't care whether the promise succeeds or fails)

Could we perhaps implement Option C by catching a CanceledError in fetchQuery and replacing it with another query.fetch(defaultedOptions, { cancelRefetch: false })?

Naive approach:

  fetchQuery<
    TQueryFnData,
    TError = DefaultError,
    TData = TQueryFnData,
    TQueryKey extends QueryKey = QueryKey,
    TPageParam = never,
  >(
    options: FetchQueryOptions<
      TQueryFnData,
      TError,
      TData,
      TQueryKey,
      TPageParam
    >,
  ): Promise<TData> {
    const defaultedOptions = this.defaultQueryOptions(options)

    // https://github.com/tannerlinsley/react-query/issues/652
    if (defaultedOptions.retry === undefined) {
      defaultedOptions.retry = false
    }

    const query = this.#queryCache.build(this, defaultedOptions)

    return query.isStaleByTime(
      resolveStaleTime(defaultedOptions.staleTime, query),
    )
-     ? query.fetch(defaultedOptions)
+     ? query.fetch(defaultedOptions).catch((e) => {
+         if (isCancelledError(e)) return query.fetch(defaultedOptions, { cancelRefetch: false })
+         else throw e
+      })
      : Promise.resolve(query.state.data as TData)
  }

The above solution doesn't cover multiple scenarios:

useSuspenseQuery goes directly to query.fetch() and therefore would still need to be handled somehow.
What if there are multiple invalidations? Maybe this would need to be done recursively. ]

But in principle something like that might work without a major change.

DogPawHat commented 2 months ago

I've been playing around with this issue a bit, as part of which I redid the react router example with tanstack router:

https://stackblitz.com/edit/tanstack-router-invalidated-cancel-error

A few differences from @SebKranz rr example:

Clicking "Invalidate then Navigate" three times quickly triggers the CancelledError
Clicking "Navigate then Invalidate" does not immediately break the app - seems like the query runs twice so it's in fetchingfor twice as long

[ ] TODO fix styling

TanStack / query