Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

invalidateQueries inconsistently cancels fetchQuery #8060

Open
SebKranz opened this issue Sep 15, 2024 · 6 comments
Open

invalidateQueries inconsistently cancels fetchQuery #8060

SebKranz opened this issue Sep 15, 2024 · 6 comments

Comments

@SebKranz
Copy link
Contributor

SebKranz commented Sep 15, 2024

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 observer1 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.

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

Footnotes

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

@TkDodo
Copy link
Collaborator

TkDodo commented Sep 15, 2024

the sandbox seems to be private

@SebKranz
Copy link
Contributor Author

the sandbox seems to be private

Apologies. Should be public now.

@TkDodo
Copy link
Collaborator

TkDodo commented Sep 15, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

SebKranz commented Sep 15, 2024

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
Copy link
Contributor

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:

  1. Clicking "Invalidate then Navigate" three times quickly triggers the CancelledError
  2. Clicking "Navigate then Invalidate" does not immediately break the app - seems like the query runs twice so it's in fetching for twice as long
  • TODO fix styling

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants