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

vue-query client invalidateQueries doesn't invalidate immediately #7694

Closed
kazcw opened this issue Jul 8, 2024 · 9 comments · Fixed by #7930
Closed

vue-query client invalidateQueries doesn't invalidate immediately #7694

kazcw opened this issue Jul 8, 2024 · 9 comments · Fixed by #7930

Comments

@kazcw
Copy link

kazcw commented Jul 8, 2024

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

@Mini-ghost
Copy link
Contributor

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

@kazcw
Copy link
Author

kazcw commented Jul 9, 2024

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.

@Mini-ghost
Copy link
Contributor

Mini-ghost commented Jul 9, 2024

export interface InvalidateQueryFilters extends QueryFilters {
refetchType?: QueryTypeFilter | 'none'
}

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

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

This approach only defers refetchQueries to the next micro task.

@adamhenderson
Copy link

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

@TkDodo
Copy link
Collaborator

TkDodo commented Aug 7, 2024

@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:

// (dosipiuk): We need to delay `invalidate` execution to next macro task for all reactive values to be updated.
// This ensures that `context` in `queryFn` while `invalidating` along reactive variable change has correct value.

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:

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:

@Mini-ghost would you like to PR that?

@kazcw
Copy link
Author

kazcw commented Aug 7, 2024

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:

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

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

const refetchFilters: RefetchQueryFilters = {
...filters,
type: filters.refetchType ?? filters.type ?? 'active',
}

@Mini-ghost
Copy link
Contributor

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

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

@DamianOsipiuk
Copy link
Contributor

DamianOsipiuk commented Aug 20, 2024

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.

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

Successfully merging a pull request may close this issue.

5 participants