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(react-query): prevent type errors and improve inference for dynamic queries on useQueries, useSuspenseQueries and createQueries #8624

Merged
merged 25 commits into from
Feb 21, 2025

Conversation

gs18004
Copy link
Contributor

@gs18004 gs18004 commented Feb 8, 2025

closes #7974

Previously, using useQueries and useSuspenseQueries with a dynamic array of mixed queries caused type errors. This commit ensures that type errors no longer occur and that the returned data is correctly inferred (e.g., as (number | boolean | undefined)[]) instead of unknown[].

스크린샷 2025-02-08 오후 9 54 30

…ic queries on useQueries and useSuspenseQueries

Previously, using useQueries and useSuspenseQueries with a dynamic array of mixed queries caused type errors. This commit ensures that type errors no longer occur and that the returned data is correctly inferred (e.g., as (number | boolean | undefined)[]) instead of unknown[].
Copy link

nx-cloud bot commented Feb 12, 2025

View your CI Pipeline Execution ↗ for commit d3cce4c.

Command Status Duration Result
nx affected --targets=test:sherif,test:knip,tes... ✅ Succeeded 4m 25s View ↗
nx run-many --target=build --exclude=examples/*... ✅ Succeeded 1m 13s View ↗

☁️ Nx Cloud last updated this comment at 2025-02-21 09:41:24 UTC

Copy link
Collaborator

@TkDodo TkDodo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have a similar / the same type for useQueries in other framework adapters - could you fix them there too please?

@@ -1621,4 +1621,65 @@ describe('useQueries', () => {
),
)
})

it('should return correct data for dynamic queries with mixed result types', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type only tests should please go towards the dedicated .test-d.tsx files:

  • packages/react-query/src/__tests__/useQueries.test-d.tsx
  • packages/react-query/src/__tests__/useSuspenseQueries.test-d.tsx

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I will fix this.

Copy link

pkg-pr-new bot commented Feb 12, 2025

Open in Stackblitz

More templates

@tanstack/angular-query-devtools-experimental

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

@tanstack/angular-query-experimental

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

@tanstack/eslint-plugin-query

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

@tanstack/query-broadcast-client-experimental

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

@tanstack/query-async-storage-persister

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

@tanstack/query-core

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

@tanstack/query-devtools

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

@tanstack/query-persist-client-core

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

@tanstack/query-sync-storage-persister

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

@tanstack/react-query

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

@tanstack/react-query-devtools

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

@tanstack/react-query-next-experimental

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

@tanstack/react-query-persist-client

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

@tanstack/solid-query

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

@tanstack/solid-query-devtools

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

@tanstack/solid-query-persist-client

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

@tanstack/svelte-query

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

@tanstack/svelte-query-devtools

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

@tanstack/svelte-query-persist-client

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

@tanstack/vue-query

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

@tanstack/vue-query-devtools

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

commit: d3cce4c

@gs18004
Copy link
Contributor Author

gs18004 commented Feb 12, 2025

we have a similar / the same type for useQueries in other framework adapters - could you fix them there too please?

@TkDodo I don't know vue.js😢 Can you merge this one and let another guy fix the vue's useQueries?

@TkDodo
Copy link
Collaborator

TkDodo commented Feb 12, 2025

if you search for type MAXIMUM_DEPTH, you will see that it comes up 6 times. They are all very likely to just be copy-pastes of each other, maybe with different names. Would be good to have them not diverge too much

@gs18004
Copy link
Contributor Author

gs18004 commented Feb 12, 2025

if you search for type MAXIMUM_DEPTH, you will see that it comes up 6 times. They are all very likely to just be copy-pastes of each other, maybe with different names. Would be good to have them not diverge too much

How should I refactor this? Actually, I did not change the MAXIMUM_DEPTH part.

@TkDodo
Copy link
Collaborator

TkDodo commented Feb 12, 2025

this was just a pointer to get you to the right files. The TypeScript types should be very similar, so I guess you’d just do the same thing again?

@TkDodo
Copy link
Collaborator

TkDodo commented Feb 12, 2025

but some type-tests are failing so I suggest you look at them first

@gs18004
Copy link
Contributor Author

gs18004 commented Feb 12, 2025

this was just a pointer to get you to the right files. The TypeScript types should be very similar, so I guess you’d just do the same thing again?

I solved the type-tests failing, and also fixed vue-query. @TkDodo

@TkDodo
Copy link
Collaborator

TkDodo commented Feb 12, 2025

angular-query, solid-query and svelte-query have that type too 😅

@gs18004
Copy link
Contributor Author

gs18004 commented Feb 12, 2025

angular-query, solid-query and svelte-query have that type too 😅

Oh, I missed them. I will work on them. Thank you!

Copy link

codecov bot commented Feb 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.46%. Comparing base (9ac54b1) to head (d3cce4c).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #8624       +/-   ##
===========================================
+ Coverage   46.28%   80.46%   +34.18%     
===========================================
  Files         199       94      -105     
  Lines        7549     1628     -5921     
  Branches     1729      383     -1346     
===========================================
- Hits         3494     1310     -2184     
+ Misses       3675      263     -3412     
+ Partials      380       55      -325     
Components Coverage Δ
@tanstack/angular-query-devtools-experimental ∅ <ø> (∅)
@tanstack/angular-query-experimental 88.65% <ø> (ø)
@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 95.37% <ø> (ø)
@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.01% <ø> (ø)
@tanstack/vue-query-devtools ∅ <ø> (∅)

@gs18004
Copy link
Contributor Author

gs18004 commented Feb 12, 2025

@TkDodo I failed to understand the Angular testing syntax, so I couldn't work on it.
Also, it's weird that solid-query's test fails. VS Code does not show type errors on the test. Can you find the reason?

@gs18004 gs18004 requested a review from TkDodo February 14, 2025 04:19
@TkDodo
Copy link
Collaborator

TkDodo commented Feb 16, 2025

right, I’d just add an assertion that data is | undefined (inside expectTypeOf)

this is still missing please

@gs18004
Copy link
Contributor Author

gs18004 commented Feb 16, 2025

right, I’d just add an assertion that data is | undefined (inside expectTypeOf)

this is still missing please

I've fix this one. I'm not sure if it is correct. Can you check it out?

Doing like

expectTypeOf(result[0].data).toEqualTypeOf<number | boolean | undefined>()

made the test fail because result[0] is possibly undefined, so I added the if statement.
스크린샷 2025-02-16 오후 9 35 08

@TkDodo
Copy link
Collaborator

TkDodo commented Feb 16, 2025

does optional chaining not work?

expectTypeOf(result[0]?.data).toEqualTypeOf<number | boolean | undefined>()

@gs18004
Copy link
Contributor Author

gs18004 commented Feb 16, 2025

does optional chaining not work?

expectTypeOf(result[0]?.data).toEqualTypeOf<number | boolean | undefined>()

Nope, it doesn't work :(

@gs18004
Copy link
Contributor Author

gs18004 commented Feb 16, 2025

does optional chaining not work?

expectTypeOf(result[0]?.data).toEqualTypeOf<number | boolean | undefined>()

Nope, it doesn't work :(

Actually I tried it again, and it works on useQueries! Sorry for the confusion.
It does not work for useSuspenseQueries because we want the result[0].data be number | boolean, not number | boolean | undefined.
Should I change the assertion to number | boolean | undefined for the useSuspenseQueries? I think this is not what we want though.

@gs18004
Copy link
Contributor Author

gs18004 commented Feb 18, 2025

const result = useSuspenseQueries({
  queries: [...queries1List, { ...Queries2.get() }],
})

expectTypeOf(result[0].data).toEqualTypeOf<number | boolean>()

The type error is shown because result[0] is possibly undefined. However, this test passes.

expectTypeOf(result).toEqualTypeOf<
  [
    ...Array<UseSuspenseQueryResult<number, Error>>,
    UseSuspenseQueryResult<boolean, Error>,
  ]
>()

Then, it means that result has at least one element. It means result[0] should not be undefined.
My VS Code works like this, but the test doesn't.

Should I just remove the test? I think the useSuspenseQueries.ts is not a problem.

@TkDodo

@TkDodo
Copy link
Collaborator

TkDodo commented Feb 18, 2025

ok

@gs18004
Copy link
Contributor Author

gs18004 commented Feb 19, 2025

@TkDodo The test failed on Nx agents, can you check on this?
In my local, it works.

@gs18004 gs18004 requested a review from TkDodo February 19, 2025 16:48
@TkDodo
Copy link
Collaborator

TkDodo commented Feb 20, 2025

angular is still erroring

@gs18004
Copy link
Contributor Author

gs18004 commented Feb 21, 2025

@TkDodo I rollbacked injectQueries to original code. I guess my method does not work on angular-queries by some reason. Can you merge it and let somebody work on angular-queries? I tried for almost a week but could not solve it :(

@arnoud-dv
Copy link
Collaborator

Hi, when I (or somebody else who wants to contribute that) will rewrite injectQueries on Angular I will make sure this type fix will be included for Angular.

It's probably not worth it right now to put much effort into this as injectQueries is broken on Angular. There's a PR open for injectQueries but that won't fix it either.

I regret I haven't been able to work on injectQueries for a long time, it's just a lot of work and very complicated typing which takes considerable effort to get a good mental overview on.

@gs18004
Copy link
Contributor Author

gs18004 commented Feb 21, 2025

Hi, when I (or somebody else who wants to contribute that) will rewrite injectQueries on Angular I will make sure this type fix will be included for Angular.

It's probably not worth it right now to put much effort into this as injectQueries is broken on Angular. There's a PR open for injectQueries but that won't fix it either.

I regret I haven't been able to work on injectQueries for a long time, it's just a lot of work and very complicated typing which takes considerable effort to get a good mental overview on.

Got it. I hope the issue gets resolved soon.

@TkDodo TkDodo merged commit f63ba16 into TanStack:main Feb 21, 2025
8 of 9 checks passed
@gs18004 gs18004 changed the title fix(react-query): prevent type errors and improve inference for dynamic queries on useQueries and useSuspenseQueries fix(react-query): prevent type errors and improve inference for dynamic queries on useQueries, useSuspenseQueries and createQueries Feb 21, 2025
@jeremiasjutz
Copy link

Hi @gs18004 @TkDodo 😄
First of all, thank you for your outstanding work on this library!

With this release I noticed that it breaks the types, when using useSuspenseQueries used together with spreaded queryOptions.

Simplified examlpe:
I have queryOptions defined somewhere, and i want to add the select option for example.

  const [{ data: myData }] = useSuspenseQueries({
    queries: [
      {
        ...myQueryOptions(),
        select(data: MyData[]) {
          return data.filter((a) => a.id === id);
        },
      },
      {
        ...
      }
    ],
  });

  function myQueryOptions() {
    return queryOptions({
      queryKey: ["my-key"],
      queryFn: async () => {
        const res = await fetch(...);
        return await res.json();
      },
    });
  }

Now the types don't work anymore. The error I get is this:

Type '(data: MyData[]) => MyData[]' is not assignable to type '(data: unknown) => unknown'.
  Types of parameters 'data' and 'data' are incompatible.
    Type 'unknown' is not assignable to type 'CredentialSchemaLite[]'.

245         select(data: MyData[]) {

Also, I don't get any autocompletion inside the queries array of the useSuspenseQueries hook.

Would be great if this issue could be resolved, in the meantime I will just use version 5.66.8 😄

@gs18004
Copy link
Contributor Author

gs18004 commented Feb 26, 2025

Okay, I will fix this. Thank you😄

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