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

fix(vue-query): mark the watcher as post flush #7942

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RomainLanz
Copy link

@RomainLanz RomainLanz commented Aug 22, 2024

Hey there! 👋🏻

This PR changes the watcher mode for useQueries from sync to post.
The change is needed to avoid having the watcher run during component unmount.

Note

This change will probably be needed on useBaseQuery, but I haven't tested it yet.

Related VueJS issues:


Currently, using the route params to define some query params triggers a new fetch when leaving a route.

For example, let's say we have the following code that runs when we are on /players/:id.

const route = useRoute();
const paramId = computed(() => +route.params.id);

const queries = useMyViewData(paramId)
  • When going from /players to /players/1, TanStack Query will trigger a fetch for Player 1 (expected).
  • When going from /players/1 to /players, TanStack Query will trigger a fetch for Player undefined (unexpected).
  • When going from /players/1 to /transfers/12345, TanStack Query will trigger a fetch for Player 12345 (unexpected).

This is due to the watcher running when the page changes.

I haven't created an easy and sharable reproduction. Let me know if it is needed.

@RomainLanz
Copy link
Author

pnpm patch for the one who wants to test it:

diff --git a/build/modern/useQueries.js b/build/modern/useQueries.js
index c172ecd85df4646ab4869411725cc028ccc0e7e1..ec584988a4ecb9f24c073eb94a4b5cbc9e8eb255 100644
--- a/build/modern/useQueries.js
+++ b/build/modern/useQueries.js
@@ -81,7 +81,7 @@ function useQueries({
       );
       state.value = getCombinedResultPersisted();
     },
-    { flush: "sync" }
+    { flush: "post" }
   );
   onScopeDispose(() => {
     unsubscribe();

@RomainLanz
Copy link
Author

We deployed this patch to production on Monday and found no issues for the moment.

@RomainLanz RomainLanz changed the title fix(vue:queries): mark the watcher as post flush fix(vue-query): mark the watcher as post flush Aug 29, 2024
Copy link

nx-cloud bot commented Aug 30, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit c6443c4. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

Copy link

pkg-pr-new bot commented Aug 30, 2024

commit: c6443c4

@tanstack/angular-query-devtools-experimental

pnpm add https://pkg.pr.new/@tanstack/angular-query-devtools-experimental@7942

@tanstack/angular-query-experimental

pnpm add https://pkg.pr.new/@tanstack/angular-query-experimental@7942

@tanstack/eslint-plugin-query

pnpm add https://pkg.pr.new/@tanstack/eslint-plugin-query@7942

@tanstack/query-async-storage-persister

pnpm add https://pkg.pr.new/@tanstack/query-async-storage-persister@7942

@tanstack/query-broadcast-client-experimental

pnpm add https://pkg.pr.new/@tanstack/query-broadcast-client-experimental@7942

@tanstack/query-core

pnpm add https://pkg.pr.new/@tanstack/query-core@7942

@tanstack/query-devtools

pnpm add https://pkg.pr.new/@tanstack/query-devtools@7942

@tanstack/query-persist-client-core

pnpm add https://pkg.pr.new/@tanstack/query-persist-client-core@7942

@tanstack/query-sync-storage-persister

pnpm add https://pkg.pr.new/@tanstack/query-sync-storage-persister@7942

@tanstack/react-query

pnpm add https://pkg.pr.new/@tanstack/react-query@7942

@tanstack/react-query-devtools

pnpm add https://pkg.pr.new/@tanstack/react-query-devtools@7942

@tanstack/react-query-next-experimental

pnpm add https://pkg.pr.new/@tanstack/react-query-next-experimental@7942

@tanstack/react-query-persist-client

pnpm add https://pkg.pr.new/@tanstack/react-query-persist-client@7942

@tanstack/solid-query

pnpm add https://pkg.pr.new/@tanstack/solid-query@7942

@tanstack/solid-query-devtools

pnpm add https://pkg.pr.new/@tanstack/solid-query-devtools@7942

@tanstack/solid-query-persist-client

pnpm add https://pkg.pr.new/@tanstack/solid-query-persist-client@7942

@tanstack/svelte-query

pnpm add https://pkg.pr.new/@tanstack/svelte-query@7942

@tanstack/svelte-query-devtools

pnpm add https://pkg.pr.new/@tanstack/svelte-query-devtools@7942

@tanstack/svelte-query-persist-client

pnpm add https://pkg.pr.new/@tanstack/svelte-query-persist-client@7942

@tanstack/vue-query

pnpm add https://pkg.pr.new/@tanstack/vue-query@7942

@tanstack/vue-query-devtools

pnpm add https://pkg.pr.new/@tanstack/vue-query-devtools@7942

Open in Stackblitz

More templates

Copy link

codecov bot commented Aug 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.42%. Comparing base (c7fb5fd) to head (c6443c4).
Report is 28 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #7942       +/-   ##
===========================================
+ Coverage   44.64%   71.42%   +26.78%     
===========================================
  Files         187       19      -168     
  Lines        7090      462     -6628     
  Branches     1575      119     -1456     
===========================================
- Hits         3165      330     -2835     
+ Misses       3562      102     -3460     
+ Partials      363       30      -333     
Components Coverage Δ
@tanstack/angular-query-devtools-experimental ∅ <ø> (∅)
@tanstack/angular-query-experimental ∅ <ø> (∅)
@tanstack/eslint-plugin-query ∅ <ø> (∅)
@tanstack/query-async-storage-persister ∅ <ø> (∅)
@tanstack/query-broadcast-client-experimental ∅ <ø> (∅)
@tanstack/query-codemods ∅ <ø> (∅)
@tanstack/query-core ∅ <ø> (∅)
@tanstack/query-devtools ∅ <ø> (∅)
@tanstack/query-persist-client-core ∅ <ø> (∅)
@tanstack/query-sync-storage-persister ∅ <ø> (∅)
@tanstack/react-query ∅ <ø> (∅)
@tanstack/react-query-devtools ∅ <ø> (∅)
@tanstack/react-query-next-experimental ∅ <ø> (∅)
@tanstack/react-query-persist-client ∅ <ø> (∅)
@tanstack/solid-query ∅ <ø> (∅)
@tanstack/solid-query-devtools ∅ <ø> (∅)
@tanstack/solid-query-persist-client ∅ <ø> (∅)
@tanstack/svelte-query ∅ <ø> (∅)
@tanstack/svelte-query-devtools ∅ <ø> (∅)
@tanstack/svelte-query-persist-client ∅ <ø> (∅)
@tanstack/vue-query 71.42% <ø> (ø)
@tanstack/vue-query-devtools ∅ <ø> (∅)

@DamianOsipiuk
Copy link
Contributor

We could potentially remove sync watcher entirely.
We did it for other hooks some time ago.

#6043

@RomainLanz
Copy link
Author

I'm not entirely sure about the implications either. If removing this watch statement is the only change, I can create another patch and test how my application behaves.

@DamianOsipiuk
Copy link
Contributor

You would also need to override refetch to update observer options before actual refetch is called.

@RomainLanz
Copy link
Author

Then we could merge that one first and work towards removing all watchers in a follow-up PR/issue.

@DamianOsipiuk
Copy link
Contributor

I do not think merging this and immediately changing solution in a follow-up is the way to go.
Better way would be to fix it properly and add automated test that verifies the fix.
For the mean time, you can patch the package locally, as you already did.

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

Successfully merging this pull request may close these issues.

2 participants