fix: type issue when void or undefined is returned from query function

[yss14]

Resolves #3516

Problem with current implementation is that T | Promise<T> is passed to QueryFunctionData<T> in types.ts, but Promise<T> does not extend undefined, so in that case the conditional type fails its intent.

useQuery(key, () => undefined) // errors, as expected ✅
useQuery(key, async () => undefined) // errors, as expected ✅

useQuery(key, () => 'hello world') // no errors, as expected ✅
useQuery(key, async () => 'hello world') // no errors, as expected ✅

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
react-query	✅ Ready (Inspect)	Visit Preview	May 4, 2022 at 5:34PM (UTC)

[codesandbox-ci]

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 6984adc:

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

[TkDodo]

thank you for the contribution 🙌 . Could you maybe add some type-level tests for useQuery and useInfiniteQuery that assure that the types work as expected, so that we can't inadvertently break them. A good starting point would be around here for useQuery:

https://github.com/tannerlinsley/react-query/blob/5562cfaaa04bbcea141c553dbdb211fec8224357/src/reactjs/tests/useQuery.test.tsx#L30

and here for useQueries:

https://github.com/tannerlinsley/react-query/blob/5562cfaaa04bbcea141c553dbdb211fec8224357/src/reactjs/tests/useQueries.test.tsx#L601

---

also, something doesn't compile at the moment:

Error: src/persistQueryClient/tests/PersistQueryClientProvider.test.tsx([15](https://github.com/tannerlinsley/react-query/runs/6107368533?check_suite_focus=true#step:5:15)0,15): error TS23[22](https://github.com/tannerlinsley/react-query/runs/6107368533?check_suite_focus=true#step:5:22): Type 'string | Promise<string> | undefined' is not assignable to type 'ReactNode'.
  Type 'Promise<string>' is not assignable to type 'ReactNode'.
    Type 'Promise<string>' is missing the following properties from type 'ReactPortal': key, children, type, props
Error: src/reactjs/tests/useQueries.test.tsx(785,13): error TS[23](https://github.com/tannerlinsley/react-query/runs/6107368533?check_suite_focus=true#step:5:23)22: Type '(a: string) => void' is not assignable to type '(data: string | Promise<string>) => void'.
  Types of parameters 'a' and 'data' are incompatible.
    Type 'string | Promise<string>' is not assignable to type 'string'.
      Type 'Promise<string>' is not assignable to type 'string'.
Error: src/reactjs/tests/useQueries.test.tsx(794,56): error TS2345: Argument of type 'UseQueryResult<string | Promise<string>, unknown>' is not assignable to parameter of type 'QueryObserverResult<string, unknown>'.
  Type 'QueryObserverLoadingErrorResult<string | Promise<string>, unknown>' is not assignable to type 'QueryObserverResult<string, unknown>'.
    Type 'QueryObserverLoadingErrorResult<string | Promise<string>, unknown>' is not assignable to type 'QueryObserverLoadingErrorResult<string, unknown>'.
      Type 'string | Promise<string>' is not assignable to type 'string'.
        Type 'Promise<string>' is not assignable to type 'string'.

[yss14]

@TkDodo Yes I see. A lot of code and complex types has been build on top of the assumption that the QueryFunctionData is correct. Let's see if I can find a way to fix all the type paths. The useQueries types are looking pretty complex though 😂

[yss14]

@TkDodo In the meantime, some questions have been arisen:

1st:

A lot of test cases use a query function with return type Promise<never>, which results from constructs like

const queryFn = () => Promise.reject('<error>') // typeof queryFn = () => Promise<never>

Is this a valid use case for react-query usage in the wild which we need to consider or just a thing occurring in internal test cases? If last, to please the compiler I could add a explicit function return types to all affected test cases like so:

const queryFn = ():Promise<unknown> => Promise.reject('<error>') // pleases tsc

2nd:

In the end what the compiler tells the user is something like Type 'Promise<void>' is not assignable to type 'never', where I am not sure if this is good DX since the user might wonder where the never type comes from, since the specified return type of the query function is Promise<void>.
However, as long as typescript does not offer some kind of custom error message I guess this is the only way to solve it at the moment besides the concept of type branding, which is also not great.

3rd:

At some locations in the codebase any is used for generic parameters, e.g. QueryClient.setQueryDefault, where all four generics of QueryObserverOptions are set to any, even thought QueryObserverOptions has default values for its generics TQueryFnData and TError set to unknown. This causes some problems with the new restrictions on the desired query function return type, since conditional types and any aren't good friends at all.
Would it be ok to change the first generic of QueryClient.setQueryDefault from any to unknown, since conditional types can handle unknown much better than any type and the first generic here is propagated into the QueryFunction<T>type.

E.g.

setQueryDefaults(
    queryKey: QueryKey,
    options: QueryObserverOptions<unknown, unknown, any, any> // change first two generics from any to unknown
  ): void {
    const result = this.queryDefaults.find(
      x => hashQueryKey(queryKey) === hashQueryKey(x.queryKey)
    )
    if (result) {
      result.defaultOptions = options
    } else {
      this.queryDefaults.push({ queryKey, defaultOptions: options })
    }
  }

[yss14]

Types for useQuery hook are working as expected now.

Regarding useQueries hook maybe @artysidorenko can elaborate if he sees any way how to accomplish it there?
Problem here is I guess that there is no direct constraint of the TQueryFnData generic from useQuery down to QueryFunction<T> since there can be n different return types which are inferred by complex recursive types.
By now, it's still possible to have a query function with return type void | undefined | Promise<void> | Promise<undefined>.

[artysidorenko]

Hi @yss14 , thanks for jumping on this 💪

On the useQueries bit, I think we could defined the type we want to guard against and bake that into the type-checker. It means another conditional on that logic, which I was hoping to avoid, but ran into similar issues you mentioned when trying to check lower down the chain at QueryFunction

type InvalidQueryFn = QueryFunction<undefined | Promise<undefined> | Promise<undefined> | void | Promise<void>>

something like this - artysidorenko@10f5585. Let me know what you think.

sidenote: interestingly, I think TS will make this kinda thing easier to deal with in the next version by allowing us to specify constraints on infer clauses. quote from the PR: "If you are testing multiple other conditions, this could result in an unmanageable branching structure which results in the need to define additional type aliases for repeated branches" which feels like what's going on here 😅

[TkDodo]

A lot of test cases use a query function with return type Promise, which results from constructs like
const queryFn = () => Promise.reject('') // typeof queryFn = () => Promise

If possible, I think we should allow Promise<never>, as it's different to Promise<void>. It's not a real production case I think, but the easiest way to test a failing query / mutation in codesandbox for example, and the type error would be hindering.

[yss14]

@TkDodo Thanks for the reply. I was thinking a lot about it during the past days and I guess the "bad" news is that this would not be possible at the moment.
QueryFunction<T> is using the generic for the data type returned by query function, not the return type of the query function. This is important because typescript leverages this constraint to automatically infer the data type of a query, independent of returning the data sync (T) or async (Promise<T>). If we would change this on a query function return type level, this would break the constraint and typescript would always infer T | Promise<T> as query function return type, and thus also as TData of every query, which is obviously not desired.
Further, I guess for sake of consistency we should not allow both never and Promise<never>, otherwise it could be a little bit confusing to other users of the library why never is not allowed, but Promise<never> is.

Regarding your use case of failing query functions in code sandboxes, I guess there is an easy fix to make it work in the future:

- useQuery(['key'], () => Promise.reject(new Error('Some error')))
+ useQuery(['key'], () => Promise.reject<unknown>(new Error('Some error')))

Furthermore, I implemented the suggested solution for useQueries (many thanks to @artysidorenko for your help!) and added test cases for both useQuery and useQueries.

Would appreciate a first code review of both of you guys :)

[yss14]

@TkDodo I rebased my branch with latest upstream beta branch and fixed the typescript errors.

How can I run test cases locally? Executing yarn test results in a TypeError: Cannot set property CancelledError of [object Object] which has only a getter error. Sounds like an error related to es modules?

[TkDodo]

Oh you get those errors locally, too? So weird, I've been having these for months - so from before the esm bundling changes. I found no way to fix it except doing a clean checkout to a new directory - then it works again. Sadly I have no clue how to properly fix it - maybe you do?

[codecov]

Codecov Report

Merging #3541 (6984adc) into beta (fdbc002) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             beta    #3541   +/-   ##
=======================================
  Coverage   96.93%   96.93%           
=======================================
  Files          47       47           
  Lines        2381     2381           
  Branches      709      709           
=======================================
  Hits         2308     2308           
  Misses         71       71           
  Partials        2        2           

Impacted Files	Coverage Δ
src/core/queryClient.ts	100.00% <ø> (ø)
src/reactjs/useQueries.ts	95.45% <ø> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fdbc002...6984adc. Read the comment docs.

[yss14]

@TkDodo I think #3521 fixed the issue. It seems like the issue was caused by separate ./es build directory. I had a stale version still on disk because I checked out the beta branch before #3521 was merged. Removing it solves the issue and I can run tests locally.
That would also explain why is has been working for you when working on a clean checkout :D

[TkDodo]

@yss14 thanks, that sounds like it could be the issue. I'm not getting it at the moment, but I also think it happened when switching between master / beta, which I have been doing a lot recently 😅 . Can't wait to finally release v4 🙌

[tannerlinsley]

🎉 This PR is included in version 4.0.0-beta.9 🎉

The release is available on:

• npm package (@beta dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[markivancho]

@TkDodo @yss14 I'm struggling with this update, maybe you can help me with that, it seems like I'm missing some point, but I don't know where)

so, I have a query that goes like

const query = useQuery(['some-key', someId], () => someFunc(someId));

someFunc return type is Promise<any>
TS complains that Type 'Promise<any>' is not assignable to type '"queryFn must not return undefined or void"'.
How do I refactor my code to satisfy new changes?

[yss14]

@markivancho
Best solution of course would be to resolve the any type. But I guess this is not possible for some reason?
Are you in control of the someFunc return type? If so, simply change it to Promise<unknown>.
If not, you can try to explicitly set the TData generic of useQuery to unknown like useQuery<unknown>.

[yss14]

@TkDodo I guess we should add some notes to the breaking change log of v4 migration guide?

[TkDodo]

@yss14 this looks like a regression imo. Is there any way we can allow any / Promise<any> ?

[yss14]

@TkDodo I don't think so, since undefined | void is a subset of any type. So we can't say undefined | void should be restricted, but any should pass.

If there are scenarios where the query function returns any and there is no way of changing the return type, explicitly specifying the TQueryFnData as unknown would still allow using such a query function.

const fetchAnyData = (): Promise<any> => Promise.resolve()

function SomePage(){
  useQuery<unknown>(['mydata'], () => fetchAnyData()) // no typescript error
}

So in the end this change is a regression, since we restricted the allowed type space of the query function return type by "removing" undefined and void from it. And since both are a subset of any, we also eliminated any from the allowed type space.

[TkDodo]

I see. Personally, I think it's still a good change. The problem is if you just use:

useQuery(key, () => axios.get(...)

it will still default to any, which will now type error. Some people do:

useQuery<MyType, Error>(key, () => axios.get(...)

which is fine and will still work.

---

I'm inclined to keep the change, because returning any from your queryFn is likely not what you want, as it's a leaking any that you might have missed. Maybe we can:

• make the error message a bit better (is not assignable to type '"queryFn must not return undefined or void"') to maybe include that any` can also not be returned?
• add this to the migration docs, potentially here: https://react-query-beta.tanstack.com/guides/migrating-to-react-query-4#undefined-is-an-illegal-cache-value-for-successful-queries
• also add this to the typescript docs: https://react-query-beta.tanstack.com/typescript with the proposed workaround of at least using unknown over any.

what do you think?

[markivancho]

thanks, after further digging, my actual problem was options type;
my custom hook goes like

const getEntity = (gid: string): Promise<{ entity: Entity }> => api.get(`/api/entity/${gid}`);
const useEntity = (gid?: string, options?: UseQueryOptions<any, any, any, any>) =>
  useQuery(['entity_by_gid', gid], () => getEntity(gid as string), {
    ...options,
    enabled: !!gid,
    select: data => data.entity
  });

With this code, typescript complains Type 'Promise<{ entity: Entity }>' is not assignable to type '"queryFn must not return undefined or void"'. which confused me the very first time

after changing options type to UseQueryOptions<{ entity: Entity }, any, any, any> everything is fine

by the way, it would be very nice if you could include in the react-query + typescript docs best practices regarding generics usage

[TkDodo]

by the way, it would be very nice if you could include in the react-query + typescript docs best practices regarding generics usage

I'll try to improve the docs. In the meantime, have a read here: https://tkdodo.eu/blog/react-query-and-type-script

[markivancho]

@TkDodo thanks! your blog is a golden mine of react-query

[TkDodo]

@yss14 here is another reproduction that works fine on v3, but fails on v4:

https://codesandbox.io/s/serene-panini-6o70bk?file=/src/hooks/useApi.ts

[yss14]

@TkDodo Thanks, I will work on a follow-up PR in the coming days regarding migration docs and typescript docs, as well as tackling the reproduction example you provided.

[yss14]

@TkDodo A little update from my side. I'm still investigating why typescript is complaining here, since your abstract data fetcher example looks fine to me. I already posted some question in r/typescript and the FullStackTypeScript Slack Community in hope anyone can give me some hint.

I also boiled down the use case into a minimal typescript playground

Maybe @artysidorenko has some clue why typescript is complaining here? :)

[artysidorenko]

Hi @yss14, yeah your first example passes because TS sees a string and can make a direct comparison with the QueryFunction constraints. However soon as you deal with a factory function it becomes more context-sensitive (without more information it assumes TQueryFnData could be anything). It's not clever enough to realise you want to enforce the “must not be undefined or void” constraint on the factory. All it sees is (1) the factory will accept a function that returns a promise, so for e.g. () => Promise<void> is a valid candidate, and (2) since there's at least this 1 candidate that gives never when applied to QueryFunction<T>, it won’t let you pass that "maybe-never" argument to useQuery at all.

Not ideal, but if we want to keep those undefined/void constraints in place, then one workaround for now could be to re-use the QueryFunction type in the factory itself to apply the same constraints upstream (special thanks also go to ReturnType<T>).

here's what it looks like forking the codesandbox

// fails
fetcher: (params: TQueryKey[1], token: string) => Promise<TQueryFnData>

// passes
fetcher: (params: TQueryKey[1], token: string) => ReturnType<QueryFunction<TQueryFnData>>,

[TkDodo]

@artysidorenko would it maybe bet helpful to extract ReturnType<QueryFunction<TQueryFnData>> into an extra type like TQueryFnResult<T> and separately export that, and document that this is the type that custom generic Query Functions should return?

[TkDodo]

@artysidorenko another thing that might be a bit weird is that I can't return Promise<any>, which is what you get by fetch or axios per default. See this codesandbox reproduction, where I've inlined the queryFn to just call axios directly.

Error is:

Type 'Promise<any>' is not assignable to type '"queryFn must not return undefined or void"'

While I'm all for avoiding leaking anys like that, I think it goes a bit against the mantra of "the queryFn just needs to return a Promise to work". I'm not sure this is doable with the current types though, but not even useQuery<any> is going to fix it.

With that in mind, do you think it would be better to just remove the type level check again?

[artysidorenko]

Hi @TkDodo yeah, I think I agree with you.

While I'm able to fix that particular Promise<any> issue for useQuery (by adding the TS equivalent of an "early return" in case of Promise<any>, got a forked branch here where I'm running the tests)

... I keep finding more and more edge cases that throw inconsistent results - for example the above solution doesn't work for useQueries yet, and neither does the "wrapping customer fetcher factory with the TQueryFnResult<T> helper".

Am mindful that you'll probably keep getting more and more of these issues reported, generating a lot of noise.

I'll put up a PR to remove the type-check later this weekend if there's no other opposition to that.

*longer term will see if it's possible to think of all the edge cases and potentially bring it later if we feel confident, but don't have an ETA on that

[TkDodo]

I'll put up a PR to remove the type-check later this weekend if there's no other opposition to that.

yes please, that would be ideal. I'm totally fine with adding that stricter type checks later if we get to a solution that we all agree upon 👍