feat(types): useQueries to flow through types

[johnnyreilly]

Hello @tannerlinsley!

Merry Christmas and thank you for react-query; it's awesome! 🎄

I've just been trying out useQueries in react-query 3 and it's really cool! However one of the things I bumped on was the return type reliably being QueryObserverResult<unknown, unknown>[]. This means I have to assert the type before I can use it.

This can be improved by supporting generics for useQueries just as useQuery does. This PR adds that support; very much modelled on the code of useQuery. It also adds a test which asserts that types flow through in the way we'd hope for.

This should be a backwards compatible change; all existing users will be experiencing QueryObserverResult<unknown, unknown>[] and then having to subsequently assert types. That should all still work, but if they would like, they'll no longer need that code.

What do you think?

[vercel]

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/tannerlinsley/react-query/m1scq9ht1
✅ Preview: https://react-query-git-fork-johnnyreilly-master.tannerlinsley.vercel.app

[johnnyreilly]

This had to move inline as it relies upon the generic types inside the QueriesObserver

[johnnyreilly]

Ah - I can see I've upset the rules of hooks in the test. I'll fix that but have to break off for now.

[johnnyreilly]

Fixed.

[boschni]

Hi @johnnyreilly, thanks for the PR! It seems like all queries need to return the same data? Think most of the time each query would require a different type. And is it possible to infer the return types from the given options?

[johnnyreilly]

It seems like all queries need to return the same data? Think most of the time each query would require a different type

The examples of useQueries suggest usage with .map operations in which case the type would be the same each time. However as we'll see - that's by the by!

And is it possible to infer the return types from the given options?

The way it works is through type inference, and so yes, if there are various types returned, that should flow through too!

[boschni]

Great, could you add some tests for those cases?

Same type:

const result = useQueries([
  { queryKey: key1, queryFn: () => 1 },
  { queryKey: key2, queryFn: () => 2 },
])

expectType<number | undefined>(result[0].data)
expectType<number | undefined>(result[1].data)

Different type:

const result = useQueries([
  { queryKey: key1, queryFn: () => 1 },
  { queryKey: key2, queryFn: () => 'data' },
])

expectType<number | undefined>(result[0].data)
expectType<string | undefined>(result[1].data)

[johnnyreilly]

Yup, the same type case is covered by the test I added. It's a good shout to cover the different type case as well. I'll try to add when I'm next near a keyboard

[johnnyreilly]

I've added a test that covers the different type use case as well. It's actually subtly different than I anticipated, where there is a variety of return types you need to specify the variety of types explicitly (or do useQueries<unknown> if you want the existing behaviour. So the typed API does slightly change here (although the executed JavaScript remains identical).

It's arguable that this would make it a breaking change from a type perspective (though for the atypical use case).

[boschni]

Ideally users should not have to specify generics explicitly and it would be great if the types could be inferred correctly for each query instead of creating a union. Would you be able to get that working?

[johnnyreilly]

I agree that would be ideal - I've had a go but I've been unsuccessful. It may not be possible with the TypeScript type system as is - or beyond my abilities.

The plus with this approach is that specifying the generics is only necessary for the edge case. Not perfect but an improvement on the current casting always scenario

[boschni]

No problem! Although not perfect, it does add typing for some use cases. Does it default to unknown when not specifying the generic and using queries with different types?

const result = useQueries([
  { queryKey: key1, queryFn: () => 1 },
  { queryKey: key2, queryFn: () => 'two' },
])

[johnnyreilly]

To take your specific case:

      const resultWithoutUsingMap = useQueries([
        { queryKey: key1, queryFn: () => 1 },
        { queryKey: key2, queryFn: () => 'two' },
      ])

The compiler will complain that the second array element is not a match for the first; so no defaulting to unknown:

Type '() => string' is not assignable to type 'QueryFunction<number>'.
  Type 'string' is not assignable to type 'number | Promise<number>'.

This is remedied by supplying the types like so:

      const resultWithoutUsingMap = useQueries<number | string>([
        { queryKey: key1, queryFn: () => 1 },
        { queryKey: key2, queryFn: () => 'two' },
      ])

However, there's a better choice; using .map. It works out like this:

      const resultWithAllTheSameTypes = useQueries(
        [1, 2].map(x => ({ queryKey: `${x}`, queryFn: () => x }))
      )
      // resultWithAllTheSameTypes: QueryObserverResult<number, unknown>[]

      const resultWithDifferentTypes = useQueries(
        [1, 'two', new Date()].map(x => ({ queryKey: `${x}`, queryFn: () => x }))
      )
      // resultWithDifferentTypes: QueryObserverResult<string | number | Date, unknown>[]

Type inference in all cases that use map which is really lovely; the TypeScript compiler very much working in our favour here.

[TkDodo]

Type inference in all cases that use map which is really nice.

Hmm, I guess we will have to supply the types with generics then a lot, because I don't think that e.g.

QueryObserverResult<number | string, unknown>[]

will be helpful a lot. The first element in the Array will be number | string' , as will the second one. If I want to pass the first result to a component that accepts a numberand the second one to a component that accepts astring, I still can't do that. Unions like string | number` are very hard to work with without further type narrowing.

What's also sub-optimal about the .map approach is that with TS 4.1 and --noUncheckedIndexedAccess on, I will also get a potentially undefined value when accessing something from the Array. If the length of the array we pass in is known / static / a tuple, it would be awesome if we could get a tuple back.

[johnnyreilly]

Unions like string | number are very hard to work with without further type narrowing.

True - but this is already the case with useQueries current signature of QueryObserverResult<unknown, unknown>[]. Manual narrowing or casting is required to get to a place where you can perform an operation. That's what this PR ideally hopes to reduce; you should need to do that less as a consequence. Only in certain use cases as opposed to all use cases.

it would be awesome if we could get a tuple back.

Agree it would be, it doesn't look like the compiler supports this use case as yet. Certainly as far as I'm aware.

What's also sub-optimal about the .map approach is that with TS 4.1 and --noUncheckedIndexedAccess on, I will also get a potentially undefined value when accessing something from the Array.

It probably depends how you're using it. I've a project that heavily uses useQueries. In my own case I've always been mapping over the results of useQueries; I've not had the need to index into the results.

Small side note: it was that project that lead me to contributing this PR. My motivation was disatisfaction with the current choice between lack of type safety with recasting after each useQueries call and reperforming type narrowing to satisfy the compiler that a type was still a given type even though that hasn't changed.

[TkDodo]

@johnnyreilly I agree that this PR makes the current situation a lot better, because queries are now typable at all as compared to not-at-all before, so thanks for your contribution 👍 . I just wanted to see if we can take it any further, but it doesn't seem easily doable. The .map use-case with the same return type will probably be what useQueries is used most, because you can create a dynamic amount of queries (likely for the same endpoint) with it. For a static amount, separate useQuery calls are also possible.

[boschni]

I do like the additional typing, but I do not like that it will error in a valid use case:

const result = useQueries([
  { queryKey: key1, queryFn: () => 1 },
  { queryKey: key2, queryFn: () => 'two' },
])
// compiler error

This will probably surprise users and they might not know how to fix it...

[johnnyreilly]

This will probably surprise users and they might not know how to fix it...

That's a reasonable concern. I'd be happy to add documentation which covers this.

[johnnyreilly]

Proposed docs to add to the useQueries page:

TypeScript users

If you're using react-query with TypeScript then generally you'll be able to benefit from type inference:

      const resultWithAllTheSameTypes = useQueries(
        [1, 2].map(x => ({ queryKey: `${x}`, queryFn: () => x }))
      )
      // resultWithAllTheSameTypes: QueryObserverResult<number, unknown>[]

      const resultWithDifferentTypes = useQueries(
        [1, 'two', new Date()].map(x => ({ queryKey: `${x}`, queryFn: () => x }))
      )
      // resultWithDifferentTypes: QueryObserverResult<string | number | Date, unknown>[]

In both the examples above, no types were specified and the compiler correctly inferred the types from the array passed to useQueries.

However, if you pass an array literal where different elements have a queryFn with differing return types then you may encounter a compiler error. Consider:

      const resultWithoutUsingMap = useQueries([
              { queryKey: key1, queryFn: () => 1 },
              { queryKey: key2, queryFn: () => 'two' },
      ])

This will result in an error along the lines of Type '() => string' is not assignable to type 'QueryFunction<number>'.

This can be remedied by supplying the union of possible data types as a type parameter; like so:

      const resultWithoutUsingMap = useQueries<number | string>([
              { queryKey: key1, queryFn: () => 1 },
              { queryKey: key2, queryFn: () => 'two' },
      ])

[johnnyreilly]

I've added the docs - happy to tweak.

[boschni]

I don't think the .map example is very useful in practice because it is basically defining one query function with all the different types in the array as return value. It does not happen often that the result is the same as the input.

I'm not entirely sure how to proceed here. The PR adds typing for single query functions, but at the same time it introduces a compiler error when using multiple query functions. And even when users fix the compiler error by explicitly specifying the return type union of all query functions, it wouldn't be that useful because users would still need to narrow down the results.

Personally I would prefer an implementation which covers all cases, or if not possible, one which degrades gracefully. This is a matter of opinion though, maybe others think differently. Would just adding the ability to specify a return type without type inference be an option? That would at least be a bit more robust.

[johnnyreilly]

Think you can remove the spread operator from the queries argument to keep the interface backwards compatible

No - it's variadic tuple types that allows the strong types to flow through in the positional way desired (see discussion above). So it would be a breaking change if taken.

[johnnyreilly]

On the other points:

It is not possible to define the error types.

I think this tweak resolves that:

import { useQueries, UseQueryOptions, UseQueryResult } from 'react-query';

type Awaited<T> = T extends PromiseLike<infer U> ? Awaited<U> : T;

export function useQueriesTyped<
    TQueries extends readonly UseQueryOptions<TData, TError>[],
    TQueryFnData = unknown,
    TError = unknown,
    TData = TQueryFnData
>(
    ...queries: [...TQueries]
): {
    [ArrayElement in keyof TQueries]: UseQueryResult<
        Awaited<ReturnType<NonNullable<Extract<TQueries[ArrayElement], UseQueryOptions>['queryFn']>>>,
        TError
    >;
} {
    // eslint-disable-next-line @typescript-eslint/no-explicit-any
    return useQueries(queries as UseQueryOptions<unknown, unknown, unknown>[]) as any;
}

The callbacks like onSuccess are untyped.

I don't think this is possible - I have experimented without success.

The select type transformation is not supported.

Not sure what this refers to.

[boschni]

No - it's variadic tuple types that allows the strong types to flow through in the positional way desired (see discussion above). So it would be a breaking change if taken.

If I strip the spread operator from ...queries then it does seem to work here.

I think this tweak resolves that

Can you give an example on how the usage would look like?

I don't think this is possible - I have experimented without success.

This also applies to other properties like initialData, they would be untyped which means you can set anything. Might not be a deal breaker but would be great to get it working properly.

Not sure what this refers to.

It refers to the select functionality. It's currently not possible to select data like: select: data => data.someProp.

[johnnyreilly]

If I strip the spread operator from ...queries then it does seem to work here.

Yeah - you're right; I've applied the change to the PR. My proposed solution around errors didn't work out so I've undone that.

With regards the select / initialData / onSuccess etc it's all in the same bracket; I don't think we can flow the types through there; which is a shame but not the end of the world.

So the PR is now non-breaking changes; just a feature.

[boschni]

With regards the select / initialData / onSuccess etc it's all in the same bracket; I don't think we can flow the types through there; which is a shame but not the end of the world.

The one I am most concerned about is the select property because the data type would be incorrect:

const results = useQueries([{ queryKey: 'key', queryFn: () => ({ prop: 'value' }), select: x => x.prop }])
results[0].data.prop // <-- TS thinks this is fine while data is actually a string

While I would love to see additional type inference for useQueries, I personally don't think it should come at the expense of compiler errors or incorrect types (excluding unknown) in certain use cases...

[johnnyreilly]

While I would love to see additional type inference for useQueries, I personally don't think it should come at the expense of compiler errors or incorrect types (excluding unknown) in certain use cases...

As I understand it there's no change in behaviour here. This PR doesn't solve the problem of type inference for select etc but it provides type inference for the general case.

I've tested with the present useQueries and the amended one with this PR and I can't replicate your experience.

Both versions error out with this:

Object is of type 'unknown'.ts(2571)

on

select: x => x.prop

Where we try to x.prop - that's where the error is thrown, as x: unknown.

So this PR doesn't change the experience of using select with useQueries; it remains identical. This PR does provide type safety generally. You will still need to use type assertions / type narrowing with select as you do presently.

[johnnyreilly]

Oh wait - I see what select does. It's (unsurprisingly) a selector on the data. Hmmm... Not sure what to suggest here.

results[0].data.prop // <-- TS thinks this is fine while data is actually a string

You're right - this is a problem

[johnnyreilly]

Okay I've been digging away on this and it looks like flowing through the select type isn't possible because TypeScript doesn't support existential generic types. See detail here and here.

So I've taken a different tack. If a select is provided in one of the queries, then the return type of that query falls back to unknown. This is the same behaviour as at present.

But if select is not provided then you get type safety. How does this sound? I've added a test for this new behaviour.

[tannerlinsley]

Seems like you would be able to use the return type of the select function conditionally, but I'm not sure... I need to learn more TS 😂

[johnnyreilly]

Let me experiment....

[johnnyreilly]

It works! There's one caveat but it's minor:

Okay I've been digging away on this and it looks like flowing through the select type isn't possible because TypeScript doesn't support existential generic types. See detail here and here.

The thing that isn't possible, is flowing through the data type to the input of the select. This is the same as the current API of useQueries; the input is unknown and requires type narrowing or type assertions.

However, as suggested by @tannerlinsley, we can get the return type of select and use it as the return type where it is present. Where select isn't supplied then the return type of the query will be inferred instead.

The PR has been updated to use this combined approach (and test has been added to cover it).

So this PR now represents the addition of strong typing to useQueries in a backwards compatible fashion.

[boschni]

Couple of thoughts:

Current implementation

1. Simple and predictable.
2. No type inference (adding support for generics like useQueries<string, Error> would be a good addition though).

This implementation

1. Type inference for the query result! 👍
2. It might confuse people as some things are inferred and some are not.
3. It's a bit hacky, more workarounds might be needed if other options are added.
4. If it does not work correctly for someone, it's not easy to fix it manually by specifying generics like useQueries<string, Error>.

This is probably as far as it can get without existential generic types. Both implementations have pros and cons, so I guess it comes down to what "we" think is more important.

[johnnyreilly]

Thanks for commenting @boschni. I agree the current implementation isn't perfect with regards to having to supply type assertions with select. However, as you say, that's a limitation of the language.

For my money the advantage of taking this is reduced bugs. Using the current implementation it's unfortunately easy to inadvertently create a bug by using type assertions incorrectly. I know this as I did. 😄 Others are bumping on this too - from the sounds of it @coffenbacher is in that category - as is @matthewdavidrodgers in #1675.

My view is that it would be a good idea to merge this as is. The typed approach has been working well for my own use cases and I'm not aware of any problematic use cases. But I appreciate I'm biased 😉

I wanted to suggest another approach that we could take. Rather than having this become the canonical useQueries, the existing one could stay as is and this updated one could land alongside it as useQueriesTyped. Alternatively, this could become the new useQueries and the existing one could continue existing but be renamed useQueriesRaw or similar.

The advantage of this approach is that it's "in-the-box" but opt-in or opt-outable (depending upon whether it's the non default or default implementation respectively).

Essentially it allows people to opt in and out as they so choose.

As I say, my view is that this is good to land as is. But pivoting to the dual approach would still be a good outcome.

[tannerlinsley]

Marking as stale and closing for now, but would like to see some more movement here in the future. Comment or reopen if/when more information comes.

[johnnyreilly]

If there's something I can do to advance this I'd love to. I've suggested a few proposed ways forward here: #1527 (comment)

If one of these works, or there's an alternative you have in mind then please do suggest it.