fix(QueriesObserver): fix improper sorting in QueriesObserver's #find… #8351

jmtoung · 2024-11-25T18:05:23Z

…MatchingObservers method

This commit fixes a bug in the QueriesObserver's #findMatchingObservers. The bug is present when there are duplicate unsorted queries, for example, ['A', 'B', 'A']. The bug results in #findMatchingObservers to return the queries in sorted order, or ['A', 'A', 'B'], which results in a mismatch between Array and Array. This bug was introduced in 5499577

…MatchingObservers method This commit fixes a bug in the QueriesObserver's #findMatchingObservers. The bug is present when there are duplicate unsorted queries, for example, ['A', 'B', 'A']. The bug results in #findMatchingObservers to return the queries in sorted order, or ['A', 'A', 'B'], which results in a mismatch between Array<QueryObserverOptions> and Array<QueryObserverMatch>. This bug was introduced in TanStack@5499577

nx-cloud · 2024-11-25T18:35:57Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 6d34f02. 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.

TkDodo

Having the same key inside useQueries multiple times is not something that we support. I’m sure we have a discussion / issue about this where we mentioned this. Maybe documenting this is the better way to go.

packages/query-core/src/queriesObserver.ts

TkDodo · 2024-11-25T18:42:15Z

oh, there should even be a warning in development mode. are you not seeing that ?

fix(useQueries): warn on duplicate keys #8246

pkg-pr-new · 2024-11-25T18:42:32Z

Open in Stackblitz

More templates

@tanstack/angular-query-devtools-experimental

npm i https://pkg.pr.new/@tanstack/angular-query-devtools-experimental@8351

@tanstack/angular-query-experimental

npm i https://pkg.pr.new/@tanstack/angular-query-experimental@8351

@tanstack/eslint-plugin-query

npm i https://pkg.pr.new/@tanstack/eslint-plugin-query@8351

@tanstack/query-broadcast-client-experimental

npm i https://pkg.pr.new/@tanstack/query-broadcast-client-experimental@8351

@tanstack/query-core

npm i https://pkg.pr.new/@tanstack/query-core@8351

@tanstack/query-async-storage-persister

npm i https://pkg.pr.new/@tanstack/query-async-storage-persister@8351

@tanstack/query-devtools

npm i https://pkg.pr.new/@tanstack/query-devtools@8351

@tanstack/query-persist-client-core

npm i https://pkg.pr.new/@tanstack/query-persist-client-core@8351

@tanstack/query-sync-storage-persister

npm i https://pkg.pr.new/@tanstack/query-sync-storage-persister@8351

@tanstack/react-query

npm i https://pkg.pr.new/@tanstack/react-query@8351

@tanstack/react-query-devtools

npm i https://pkg.pr.new/@tanstack/react-query-devtools@8351

@tanstack/react-query-next-experimental

npm i https://pkg.pr.new/@tanstack/react-query-next-experimental@8351

@tanstack/react-query-persist-client

npm i https://pkg.pr.new/@tanstack/react-query-persist-client@8351

@tanstack/solid-query

npm i https://pkg.pr.new/@tanstack/solid-query@8351

@tanstack/solid-query-devtools

npm i https://pkg.pr.new/@tanstack/solid-query-devtools@8351

@tanstack/solid-query-persist-client

npm i https://pkg.pr.new/@tanstack/solid-query-persist-client@8351

@tanstack/svelte-query

npm i https://pkg.pr.new/@tanstack/svelte-query@8351

@tanstack/svelte-query-devtools

npm i https://pkg.pr.new/@tanstack/svelte-query-devtools@8351

@tanstack/svelte-query-persist-client

npm i https://pkg.pr.new/@tanstack/svelte-query-persist-client@8351

@tanstack/vue-query

npm i https://pkg.pr.new/@tanstack/vue-query@8351

@tanstack/vue-query-devtools

npm i https://pkg.pr.new/@tanstack/vue-query-devtools@8351

commit: 6d34f02

jmtoung · 2024-11-25T18:51:41Z

Having the same key inside useQueries multiple times is not something that we support. I’m sure we have a discussion / issue about this where we mentioned this. Maybe documenting this is the better way to go.

Not supporting same key is fine, but this bug was introduced by this recent change 5499577

Prior to this change, duplicate keys wouldn't result in a mismatch between input and output.

TkDodo · 2024-11-25T21:06:53Z

Prior to this change, duplicate keys wouldn't result in a mismatch between input and output.

but “duplicate keys” and “same keys” are the same thing? Our warning even says that the behaviour is potentially unexpected.

jmtoung · 2024-11-25T22:44:26Z

oh, there should even be a warning in development mode. are you not seeing that ?

fix(useQueries): warn on duplicate keys #8246

I am seeing the warning, but I think we should still fix it to return the correct order, even in spite of duplicate queries.

In other words, after the bug was introduced, useQueries(['A', 'B', 'A']) returns ['A', 'A', B'], whereas it should return ['A', 'B', 'A']. This bug was recently introduced here 5499577.

In that commit, we refactored things to efficiently loop over Array<QueryObserverOptions> once, but we introduced a bad sort that sorts by options.queryHash

jmtoung · 2024-11-25T22:45:31Z

Prior to this change, duplicate keys wouldn't result in a mismatch between input and output.

but “duplicate keys” and “same keys” are the same thing? Our warning even says that the behaviour is potentially unexpected.

Yes, the warning does warn against having duplicate/same queries, but in the event that a caller has duplicate/same queries, wouldn't we want to return the correct answer to the best of our abilities? The library used to, but only recently started to return the wrong value after this change.

codecov · 2024-11-26T11:10:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.90%. Comparing base (dfe2cd1) to head (6d34f02).
Report is 1 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #8351       +/-   ##
===========================================
+ Coverage   45.97%   62.90%   +16.93%     
===========================================
  Files         200      136       -64     
  Lines        7509     4799     -2710     
  Branches     1718     1348      -370     
===========================================
- Hits         3452     3019      -433     
+ Misses       3680     1540     -2140     
+ Partials      377      240      -137

Components	Coverage Δ
@tanstack/angular-query-devtools-experimental	`∅ <ø> (∅)`
@tanstack/angular-query-experimental	`87.13% <ø> (ø)`
@tanstack/eslint-plugin-query	`∅ <ø> (∅)`
@tanstack/query-async-storage-persister	`43.85% <ø> (ø)`
@tanstack/query-broadcast-client-experimental	`∅ <ø> (∅)`
@tanstack/query-codemods	`∅ <ø> (∅)`
@tanstack/query-core	`94.52% <100.00%> (+0.82%)`	⬆️
@tanstack/query-devtools	`4.78% <ø> (ø)`
@tanstack/query-persist-client-core	`57.73% <ø> (ø)`
@tanstack/query-sync-storage-persister	`84.61% <ø> (ø)`
@tanstack/react-query	`95.54% <ø> (ø)`
@tanstack/react-query-devtools	`10.00% <ø> (ø)`
@tanstack/react-query-next-experimental	`∅ <ø> (∅)`
@tanstack/react-query-persist-client	`100.00% <ø> (ø)`
@tanstack/solid-query	`78.20% <ø> (ø)`
@tanstack/solid-query-devtools	`∅ <ø> (∅)`
@tanstack/solid-query-persist-client	`100.00% <ø> (ø)`
@tanstack/svelte-query	`87.33% <ø> (ø)`
@tanstack/svelte-query-devtools	`∅ <ø> (∅)`
@tanstack/svelte-query-persist-client	`100.00% <ø> (ø)`
@tanstack/vue-query	`71.45% <ø> (ø)`
@tanstack/vue-query-devtools	`∅ <ø> (∅)`

github-actions bot added the package: query-core label Nov 25, 2024

jmtoung mentioned this pull request Nov 25, 2024

useQueries returns results in incorrect order when duplicate unsorted queries exist #8352

Closed

Merge branch 'main' into fix-bug

181f490

ci: apply automated fixes

de9bc99

TkDodo requested changes Nov 25, 2024

View reviewed changes

packages/query-core/src/queriesObserver.ts Show resolved Hide resolved

Aghassi approved these changes Nov 26, 2024

View reviewed changes

Merge branch 'main' into fix-bug

6d34f02

TkDodo merged commit 3b7556f into TanStack:main Nov 26, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(QueriesObserver): fix improper sorting in QueriesObserver's #find… #8351

fix(QueriesObserver): fix improper sorting in QueriesObserver's #find… #8351

jmtoung commented Nov 25, 2024

nx-cloud bot commented Nov 25, 2024 •

edited

Loading

TkDodo left a comment

TkDodo commented Nov 25, 2024

pkg-pr-new bot commented Nov 25, 2024 •

edited

Loading

jmtoung commented Nov 25, 2024

TkDodo commented Nov 25, 2024

jmtoung commented Nov 25, 2024

jmtoung commented Nov 25, 2024

codecov bot commented Nov 26, 2024 •

edited

Loading

fix(QueriesObserver): fix improper sorting in QueriesObserver's #find… #8351

fix(QueriesObserver): fix improper sorting in QueriesObserver's #find… #8351

Conversation

jmtoung commented Nov 25, 2024

nx-cloud bot commented Nov 25, 2024 • edited Loading

☁️ Nx Cloud Report

TkDodo left a comment

Choose a reason for hiding this comment

TkDodo commented Nov 25, 2024

pkg-pr-new bot commented Nov 25, 2024 • edited Loading

jmtoung commented Nov 25, 2024

TkDodo commented Nov 25, 2024

jmtoung commented Nov 25, 2024

jmtoung commented Nov 25, 2024

codecov bot commented Nov 26, 2024 • edited Loading

Codecov Report

nx-cloud bot commented Nov 25, 2024 •

edited

Loading

pkg-pr-new bot commented Nov 25, 2024 •

edited

Loading

codecov bot commented Nov 26, 2024 •

edited

Loading