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

feat(types): enable types in useQueries #2634

Merged
merged 14 commits into from
Oct 23, 2021

Conversation

artysidorenko
Copy link
Contributor

Hi folks, thanks for the great library, have been using it a lot recently!

I've had a stab at issue #1675 , taking inspiration from PR #1527 but with a slightly different approach:

  • useQueries gets a type parameter in the shape of some array (representing the data types returned from the queryFn). This is inferred from the passed-in queries array.
  • the queries array itself is a typed as a mapped generic tuple, so the individual elements' types are preserved in the correct order.

It also required tweaking some of the options property types slightly - namely initialData and placeholderData, to enforce the type that was set on the queryFn. Adding some comments to the PR diff to explain further.

I think it should be backwards-compatible.

Let me know what you think.

@vercel
Copy link

vercel bot commented Sep 3, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/tanstack/react-query/5ydQs6QhdipMm4aZefQPyaFnL2Fr
✅ Preview: https://react-query-git-fork-artysidorenko-typed-use-queries-tanstack.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 3, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 4ec115c:

Sandbox Source
tannerlinsley/react-query: basic Configuration
tannerlinsley/react-query: basic-typescript Configuration

src/react/useQueries.ts Outdated Show resolved Hide resolved
@TkDodo
Copy link
Collaborator

TkDodo commented Sep 4, 2021

Thank you for working on this 🙏

prettier formatting seems to be off, which is why the pipeline fails, please have a look.

Also, I played around with this a bit, here are my findings:

  • input passed to useQueries should be readonly, otherwise, I can't pass tuples created by const assertions to it, e.g. this fails:
useQueries([
    {
      queryKey: "key1",
      queryFn: () => "string",
    }
  ] as const);
  • it's awesome that tuple inputs and positions are preserved 🚀
  • as said above, select doesn't work:
  const [shouldBeString] = useQueries([
    {
      queryKey: "key2",
      queryFn: () => 1,
      initialData: 1,
      select: data => String(data)
    },
  ]);

result type should be an Observer of string, but it still says QueryObserverResult<number>

  • if I input two different select options, typeScript complains that they are both implicit any:
useQueries([
    {
      queryKey: "key2",
      queryFn: () => 1,
      initialData: 1,
      select: data1 => String(data1)
    },
    {
      queryKey: "key2",
      queryFn: () => "1",
      initialData: "1",
      select: data2 => Number(data2)
    },
  ]);

here, data1 and data2 are both implicit any. It works with just one element though 🤔

  • 🚨 it seems like I can't use dynamic lengths, e.g. mapping over an array, which is the main use-case of useQueries:
 const queries = useQueries(
    myInput.map((i) => [
      {
        queryKey: ["key", i] as const,
        queryFn: () => i + 10,
      }
    ])
  );

this errors with:

Type '{ queryKey: readonly ["key", number]; queryFn: () => number; }[]' has no properties in common with type 'InferredQueryOptions<unknown>'

@artysidorenko
Copy link
Contributor Author

artysidorenko commented Sep 5, 2021

Thanks for the feedback; I'm gunna have to think about the solution a bit more as it sounds like I had only covered the set of TQueryFnData types, and need to work out the TData types as another dimension (that missing TData is related to those other issues you mentioned)

Experimenting with a second type parameter of equal length but I may need to do some work to pass the types through the QueriesObserver

function useQueries<
  TQueriesFnData extends unknown[],
  TQueriesData extends {[E in keyof TQueriesFnData] : TQueriesData[E]},
>(
  queries: [...QueriesOptions<TQueriesFnData, TQueriesData>] // TQueriesData analogous to TData
)

Will keep you posted; it's an interesting puzzle 😄

@artysidorenko
Copy link
Contributor Author

artysidorenko commented Sep 11, 2021

Hi again,

Sorry took a while to explore. I think I've come up with something that's workable. Originally I had hoped I could do it with a single mapped type, however, because it needs to track multiple types that can be different for each element, I've ended up having to write a bunch of helpers + recursion to cycle through the array 1 entry at a time (there's a lot of these nested "X extends infer Y?" statements unfortunately)

Anyways, the good news:

  • it infers all three types (fnData/error/data) for each element if no type param is provided. There is validation; the one piece that isn't working 100% smoothly is the callback parameter types - see below, my response to your noImplicitAny comment.

  • you can also add explicit type params, either as a nested array, something like useQueries<[[TQueryFnData1, TError1, TData1], [TQueryFnData2, TError2, TData2]]>, or as an array of objects useQueries<[{queryFnData: X, error: Y, data: Z}, {queryFnData: A, error: B, data: C}]>. I've kept both options here to show what they look like. My preference would be to keep the nested array one only, which will shave off a bit of the code/complexity.

  • fyi there's a depth limit of 20, so if your array has 20+ entries, it will give up and return the usual unknown UseQueryOptions[]. Without this limit TS would give a Type instantiation is excessively deep and possibly infinite. error after ~25 entries.

Let me know what you think. There's a bit of complexity, but I've tried to add tests to cover all the possibilities; it should be pretty stable unless the underlying types change significantly? (in which case we'd need to update the utility types here as well).

Covering last week's comments specifically:

input passed to useQueries should be readonly, otherwise, I can't pass tuples created by const assertions to it

Think this should be working now

as said above, select doesn't work

Updated it to discriminate between queryFn / select

if I input two different select options, typeScript complains that they are both implicit any

I'm afraid I've hit a wall on this noImplicitAny issue. On the bright side I'm able to type and validate the callbacks, but it won't allow you to pass the argument without a type. (same applies to onSuccess / onSettled). I think what I have is enough to work with, but let me know if it's a blocker.

useQueries([
  {
    queryKey: "key1",
    queryFn: () => 1,
    initialData: 1,
    select: data1 => String(data1) // TS will complain about noImplicitAny
  },
  {
    queryKey: "key2",
    queryFn: () => 1,
    initialData: 1,
    select: (data1: number) => String(data1) // TS will allow this, and my IDE even suggests the number type on hover, but I need to add it explicitly to avoid the noImplicitAny
  },
  {
    queryKey: "key3",
    queryFn: () => "1",
    initialData: "1",
    select: (data2: number) => data2 // TS will complain as it should
  },
]);

it seems like I can't use dynamic lengths, e.g. mapping over an array

I believe Array.map / similar array methods all return arrays rather than a tuples, so this process can't handle them like array literals. However I've updated it so that if it can't infer a tuple, it will revert to a regular unknown UseQueryOptions[] as before.

Comment on lines 10 to 27
// Map params from object {queryFnData: TQueryFnData, error: TError, data: TData}
T extends {
queryFnData: infer TQueryFnData
error?: infer TError
data: infer TData
}
? UseQueryOptions<TQueryFnData, TError, TData>
: T extends { queryFnData: infer TQueryFnData; error?: infer TError }
? UseQueryOptions<TQueryFnData, TError>
: T extends { data: infer TData; error?: infer TError }
? UseQueryOptions<unknown, TError, TData>
: // Map params from tuple [TQueryFnData, TError, TData]
T extends [infer TQueryFnData, infer TError, infer TData]
? UseQueryOptions<TQueryFnData, TError, TData>
: T extends [infer TQueryFnData, infer TError]
? UseQueryOptions<TQueryFnData, TError>
: T extends [infer TQueryFnData]
? UseQueryOptions<TQueryFnData>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this + the same section of GetResults can be slashed in half if we keep one of the two type parameter structures

Comment on lines 65 to 77
type QueriesOptions<
T extends any[],
Result extends any[] = [],
Depth extends ReadonlyArray<number> = []
> = Depth['length'] extends MAXIMUM_DEPTH
? UseQueryOptions[]
: T extends []
? []
: T extends [infer Head]
? [...Result, GetOptions<Head>]
: T extends [infer Head, ...infer Tail]
? QueriesOptions<[...Tail], [...Result, GetOptions<Head>], [...Depth, 1]>
: T
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to add a bit more context, why I ended up going down this recursive-array-map route...

Originally I had hoped I could do something a bit more elegant with two array type parameters TQueryFnData[] and TData[] mapped by a single operation, but ran into a brick wall as I can't index them with a single key. (or if I try and force it, it breaks the tuple magic)

Idea 1:

// ERROR: "E cannot be used to index TData"
type QueriesOptions<TQueryFnData, TData> = { [E in keyof TQueryFnData]: InferredQueryOptions<TQueryFnData[E], TData[E]> }

useQueries<TQueryFnData extends unknown[], TData extends unknown[]>(queries: [...QueriesOptions<TQueryFnData, TData>])

Idea 2

// something like this can be made to work, but QueriesOptions is no longer an array-type
type QueriesOptions<TQueryFnData, TData> = keyof TData extends keyof TQueryFnData ? { [E in keyof TQueryFnData]: InferredQueryOptions<TQueryFnData[E], TData[E]> } : never

// ERROR: "a rest parameter must be of an array type"
useQueries<TQueryFnData extends unknown[], TData extends unknown[]>(queries: [...QueriesOptions<TQueryFnData, TData>])

I think until TS introduce existential types and/or loosen their rules around the spread operator, I'm dependent on the "brute-force" recursive approach

@artysidorenko
Copy link
Contributor Author

Clicking the re-request review button just in case there was no notification sent out with the last updates I pushed (should have done this sooner but was travelling last couple of weeks). Any feedback let me know.

@TkDodo
Copy link
Collaborator

TkDodo commented Oct 4, 2021

sorry @artysidorenko, it really slipped past me and I didn't get any notifications about the new commits :(

I'll try to have a look this week

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.

Thanks again for working on this, and sorry again that I missed your commits.

  • ✅ const assertions now work
  • ✅ select now works
  • 🟡 it's a bit unfortunate that data passed to select is implicitly any, e.g. here:
[
    {
      queryKey: "key2",
      queryFn: () => 1,
      select: (data) => String(data)
    }
  ]

but I guess this isn't an easy fix?

  • ✅ success and error look good, even though we have to type it explicitly. I guess same restriction as with select?
  • 🔴 as commented on the test, useQueries with an array.map infers to <unknown, unknown>. I think getting inference here would be crucial

src/react/tests/useQueries.test.tsx Outdated Show resolved Hide resolved
src/react/tests/useQueries.test.tsx Outdated Show resolved Hide resolved
@artysidorenko
Copy link
Contributor Author

artysidorenko commented Oct 10, 2021

Hi @TkDodo , thanks for looking at it. Pushed an update that covers Array.map, and cleans up the other test comment (also added a few more tests and tried to clarify the code comments a bit)

it's a bit unfortunate that data passed to select is implicitly any

I'm afraid I'm stuck on this one indeed - I think it's related to how TS evaluates the contextual vs context-sensitive types in a sequential manner. If it's dealing with an array of context-sensitive types I reckon the compiler must be doing multiple rounds of evaluation which is preventing it from inferring the param types upfront. The best I could get was "enforce" rather than "infer"

I guess it would mean if this gets released it might trigger some type errors in users' code, where people were using callbacks without typing the params.

@TkDodo
Copy link
Collaborator

TkDodo commented Oct 10, 2021

I think this looks really good. Complex (I don't fully understand everything tbh 😅) but really good, and very well tested 👍

quick last question: is there a minimum typescript version that we'd need to get this going? Is there any new language feature that you're using that will make it impossible for older versions of TypeScript (like 3.x) to work with it?

The docs are currently saying:

Types currently require using TypeScript v3.8 or greater

and if that changes, we'd need to at least update the docs.

@TkDodo
Copy link
Collaborator

TkDodo commented Oct 10, 2021

I'm afraid I'm stuck on this one indeed

Don't worry about it, I don't think it's required to get this PR shipped. I know we have similar issues when using useQuery together with e.g. onSuccess and select

@artysidorenko
Copy link
Contributor Author

artysidorenko commented Oct 11, 2021

is there a minimum typescript version that we'd need to get this going? Is there any new language feature that you're using that will make it impossible for older versions of TypeScript (like 3.x) to work with it?

Ah indeed, it might be another trade-off as all this requires TS 4.1+ (for its recursive conditional types, used to cycle through the tuple 1 element at a time). With older versions - including 4.0 - it will fail and give you any as the result.

I had a think if there's anything we could do... if some users really can't upgrade to 4.1 but also really need useQueries, I could add a second definition that works with older versions. For example a second overloaded definition like this

function useQueries<_ extends unknown, T extends any[] = []>(queries: readonly UseQueryOptions[]): UseQueryResult[];
  • doesn't mess with the 4.1 definition because we still have some T that fits the bill, and
  • means you could write useQueries<unknown> to narrow the type and force it to use the 3.8 definition to get the original types (QueryObserverResult<unknown, unknown>[])

But it might just be confusing/need documentation etc

@TkDodo
Copy link
Collaborator

TkDodo commented Oct 15, 2021

I think that overload is a good compromise. But please also amend the TypeScript docs that currently say you need 3.8, to something that says that 4.1 is needed if you want proper typings for useQueries.

The important part is that users with 3.8 will not get any type errors in their code if they are using useQueries at the moment, and especially if they are only using useQuery :)

@artysidorenko
Copy link
Contributor Author

Aha, so I did a bit more research (I wasn't very happy with the compromise I suggested after trying it out - it was looking quite fiddly trying to get the overload right, and there is enough complexity in there as it is 😅)... I found that we can get full backwards-compatibility after all:

Added a directive in package.json to tell older TS versions to pick up a compatible useQueries definition. It means an extra file in your src directory, but I figured you could also leverage this if any similar b/c issues come up in the future.

Let me know if it works for you. Added a snippet to the docs as well (let me know if it's worth adding something extra in the community section as well, to cover a bit more how to add the explicit type parameters?)

Comment on lines +1 to +4
import { useQueries } from './useQueries'
export { useQueries }

export * from '..'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

named export will always take priority over the wildcard export

Comment on lines +78 to +80
"typesVersions": {
"<4.1": { "types/*": ["types/ts3.8/*"] }
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know much about this feature tbh, but did a bunch of testing and it seems to work really well.

more info: https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html#version-selection-with-typesversions

@TkDodo TkDodo merged commit 2c1d74b into TanStack:master Oct 23, 2021
@tannerlinsley
Copy link
Collaborator

🎉 This PR is included in version 3.28.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@bennettdams
Copy link
Contributor

@artysidorenko I just wanted to take a moment to say thank you. The use of as gives me anxiety and with this PR I can remove the last occurrence of it in my codebase.

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

Successfully merging this pull request may close these issues.

None yet

4 participants