query icon indicating copy to clipboard operation
query copied to clipboard

`vue-query` client `invalidateQueries` doesn't invalidate immediately

Open kazcw opened this issue 1 year ago • 9 comments
trafficstars

Describe the bug

The Vue implementation of QueryClient.invalidateQueries does not invalidate queries "immediately", as described in the documentation.

Your minimal, reproducible example

https://codesandbox.io/p/devbox/vue-query-query-invalidation-w2kn9y

Steps to reproduce

Run the CodeSandbox; the rendered page shows the difference in behavior between the Vue QueryClient and the base implementation (the latter seems more consistent with the documentation).

Expected behavior

The documentation for QueryClient.invalidateQueries says that "By default, all matching queries are immediately marked as invalid and active queries are refetched in the background." I understand this to mean that, after calling the function, matched queries should be seen to be invalidated, even if the function's returned Promise was not awaited. This is the case when using the base QueryClient of @tanstack/query-core.

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

  • OS: Not operating-system specific
  • Browser: Not browser-specific
  • Browser version: Not browser-version specific

Tanstack Query adapter

vue-query

TanStack Query version

v5.51.0

TypeScript version

No response

Additional context

A workaround I'm using is to override invalidateQueries with an implementation that performs the invalidations immediately, deferring only the query refetches (as required to fix #6414).

kazcw avatar Jul 08 '24 21:07 kazcw

Your approach was very effective. I simplified the implementation based on your solution:

export class QueryClient extends QC
  invalidateQueries(
    filters: MaybeRefDeep<InvalidateQueryFilters> = {},
    options: MaybeRefDeep<InvalidateOptions> = {},
  ): Promise<void> {
    const filtersCloned = cloneDeepUnref(filters)
    const optionsCloned = cloneDeepUnref(options)

    super.invalidateQueries({ ...filtersCloned, refetchType: 'none' }, optionsCloned)
    return nextTick().then(() => {
      return super.refetchQueries(filtersCloned, optionsCloned)
    })
  }
}

What do you think?

cc @@DamianOsipiuk

Mini-ghost avatar Jul 09 '24 04:07 Mini-ghost

Your approach was very effective. I simplified the implementation based on your solution:

export class QueryClient extends QC
  invalidateQueries(
    filters: MaybeRefDeep<InvalidateQueryFilters> = {},
    options: MaybeRefDeep<InvalidateOptions> = {},
  ): Promise<void> {
    const filtersCloned = cloneDeepUnref(filters)
    const optionsCloned = cloneDeepUnref(options)

    super.invalidateQueries({ ...filtersCloned, refetchType: 'none' }, optionsCloned)
    return nextTick().then(() => {
      return super.refetchQueries(filtersCloned, optionsCloned)
    })
  }
}

What do you think?

cc @@DamianOsipiuk

I think filtersCloned.refetchType needs to be propagated to the filter type in refetchQueries.

kazcw avatar Jul 09 '24 13:07 kazcw

https://github.com/TanStack/query/blob/14d9c49fba6715a21e62dd473c11c8454cdc51f9/packages/query-core/src/types.ts#L505-L507

https://github.com/TanStack/query/blob/14d9c49fba6715a21e62dd473c11c8454cdc51f9/packages/query-core/src/queryClient.ts#L288-L290

It seems that refetchType exists to determine whether invalidateQueries needs to perform a refetch.

This approach only defers refetchQueries to the next micro task.

Mini-ghost avatar Jul 09 '24 14:07 Mini-ghost

Hi,

I'm not sure if its related or not to your issue but I'm seeing successive calls to clientQuery.invalidateQueries take 4 times longer than the last.

ie: I have a query that lists all entities, I have a mutator that add another entity, I await for that to complete then call invalidateQueries passing in the queryKey, if I do that the timings increase ~4 fold.

0.052secs 0.13secs 0.45secs 1.919secs 48.239secs 233.824secs

Could be user error (I'm new to tanstack) - happy to create a new issue/reproducer

adamhenderson avatar Aug 07 '24 11:08 adamhenderson

@Mini-ghost if the goal is to perform the marking as invalidated immediately, but do the refetching in the "next tick", then your implementation looks correct. looking at the comment in the code left by @DamianOsipiuk:

https://github.com/TanStack/query/blob/febbde676d0888d8dd4868177868a3316686cb58/packages/vue-query/src/queryClient.ts#L178-L179

I can only assume that what needs to be deferred is the refetch, not the mark as invalidated, because only the refetch itself runs the queryFn.

sadly, in the PR that introduced the fix, we didn't add a test case:

  • https://github.com/TanStack/query/pull/6561/files

So I'm unsure what still needs to "work" after the fix. But maybe we can add a test case now and look at the original issue posted in:

  • https://github.com/TanStack/query/issues/6414

@Mini-ghost would you like to PR that?

TkDodo avatar Aug 07 '24 14:08 TkDodo

The refetchType property of InvalidateQueryFilters can be used in two ways:

  • It can be set to none to prevent refetch.
  • It can be set to a refetch type, which should be passed to the refetchQueries function.

This implementation ignores the refetchType entirely; see comments added here:

export class QueryClient extends QC
  invalidateQueries(
    filters: MaybeRefDeep<InvalidateQueryFilters> = {},
    options: MaybeRefDeep<InvalidateOptions> = {},
  ): Promise<void> {
    const filtersCloned = cloneDeepUnref(filters)
    const optionsCloned = cloneDeepUnref(options)

    // Here we call `super.invalidateQueries` with `refetchType` overridden, so the `refetchType` is not used.
    super.invalidateQueries({ ...filtersCloned, refetchType: 'none' }, optionsCloned)
    return nextTick().then(() => {
      // Now:
      // - We are calling `refetchQueries` even if `refetchType` is `none`
      // - We are passing the filters to `refetchQueries`, but it ignores `refetchType`, because that is a property
      //  of `InvalidateQueryFilters` that is not present in the `refetchQueries` filters.
      return super.refetchQueries(filtersCloned, optionsCloned)
    })
  }
}

In order to support those two functionalities offered by the refetchType property, we need to replicate the logic that super.invalidateQueries would have performed if we didn't override refetchType:

  • We should skip refetching if the value is none:

https://github.com/TanStack/query/blob/14d9c49fba6715a21e62dd473c11c8454cdc51f9/packages/query-core/src/queryClient.ts#L288-L290

  • When we don't skip refetching, any refetchType value provided should set the type field passed to refetchQueries (because it reads type, not refetchType):

https://github.com/TanStack/query/blob/14d9c49fba6715a21e62dd473c11c8454cdc51f9/packages/query-core/src/queryClient.ts#L291-L294

kazcw avatar Aug 07 '24 15:08 kazcw

@TkDodo Thank you for your response. I am happy to address this issue and will review various operation combinations to add test cases for these scenarios.

Mini-ghost avatar Aug 07 '24 17:08 Mini-ghost

@kazcw After reviewing the core code implementation, I realize you are correct. I will check if refetchType is none to determine whether to execute super.refetchQueries.

Mini-ghost avatar Aug 08 '24 06:08 Mini-ghost

sadly, in the PR that introduced the fix, we didn't add a test case: https://github.com/TanStack/query/pull/6561/files So I'm unsure what still needs to "work" after the fix. But maybe we can add a test case now and look at the original issue posted in:

Issue is that reactive values in vue are queued in the event loop, because we have to update the observer options

const updater = () => {
    observer.setOptions(defaultedOptions.value)
    updateState(state, observer.getCurrentResult())
  }

  watch(defaultedOptions, updater)

This produces inconsistency if invalidateQueries is run immediately:

const foo = ref('foo');
useQuery({
  queryKey: ['foo', foo],
  queryFn: async ({ queryKey }) => {
    console.log(queryKey[1], foo.value);
    return {};
  },
});

const queryClient = useQueryClient();

setTimeout(() => {
  foo.value = 'bar';
  queryClient.invalidateQueries();
}, 2000);

In this case queryKey[1] which is taken from queryFn context has stale value. While foo.value is correct.

And yes, the solution to this might be to re-implement invalidate function to defer only refetch if necessary.

DamianOsipiuk avatar Aug 20 '24 08:08 DamianOsipiuk