feat(react-query): isLoading of type DefinedUseQueryResult is always false

[ronny1020]

Since there is always cache data, the isLoading of type DefinedUseQueryResult should be false. Besides, the Omit is not required.

[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 1f9b626:

Sandbox	Source
@tanstack/query-example-react-basic	Configuration
@tanstack/query-example-react-basic-typescript	Configuration

[TkDodo]

Why is the omit not required? It should be so that the type of data will be properly narrowed...?

[ronny1020]

I'm not sure what case will it not be appropriately narrowed.

type Prototype = { a: number | null };

type NewType = Prototype & { a: number };

type NewNeverType = Prototype & { a: string };

// OK
const a: NewType = { a: 0 };

// Error
const b: NewType = { a: null };

// Error, the type of property a is never
const c: NewNeverType = { a: '3' };

I think it will be fine. Besides, it can avoid using the type which is not in the prototype.

Moreover, if I used Omit, there would be overload error in the useQuery, or I have to write something like UseQueryResult<TData, TError> | DefinedUseQueryResult<TData, TError>.

[TkDodo]

I'm not sure what case will it not be appropriately narrowed.

Interesting! I'm pretty sure there was a time when you needed to remove a property from an object type before you could refine it, or the intersection wouldn't work properly. But I've tested this as far back as TS v3.3 and it all works fine 🤷

[TkDodo]

I'm still not sold on setting isLoading to false. If we do that, wouldn't we also need to remove 'loading' from the 'status' field? Why is it important?

[ronny1020]

I'm not sure what case will it not be appropriately narrowed.

Interesting! I'm pretty sure there was a time when you needed to remove a property from an object type before you could refine it, or the intersection wouldn't work properly. But I've tested this as far back as TS v3.3 and it all works fine 🤷

I think that could happen if given a type that does not extend from the original one. In that case Omit is required.

[ronny1020]

I'm still not sold on setting isLoading to false. If we do that, wouldn't we also need to remove 'loading' from the 'status' field? Why is it important?

In my opinion, I agree to remove the loading from the status. Since the type wouldn't happen, the type should not include it.

To be honest, the reason why I create this PR is that in my project I had used the previous version of react-query, and the data from the useQuery could be undefined so I write something like const value = data ?? defaultValue instead of using initialData. After migrating to the current version, I find out there is a fantastic type, DefinedUseQueryResult so I start to use this and the initialData, However, I forget to change the isLoading to isFetching which cause some bugs.

It's not a big deal, I just think that still could happen to some other guys. With a strict type, maybe that could be avoided. Moreover, I can't see a reason to allow a type that will not exist.

[TkDodo]

alright, please remove the loading status from the union, too

[ronny1020]

Hi, I think I found a better way to solve this issue. The Omit and loading things wouldn't be a problem anymore. Other properties should also work properly such as isLoadingError. Moreover, if there is some change related to the status, the type will still work properly.

[TkDodo]

oh yeah I like that solution 👍

[codecov-commenter]

Codecov Report

Merging #3988 (1f9b626) into main (eab6e2c) will increase coverage by 0.45%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3988      +/-   ##
==========================================
+ Coverage   96.36%   96.81%   +0.45%     
==========================================
  Files          45       57      +12     
  Lines        2281     2671     +390     
  Branches      640      784     +144     
==========================================
+ Hits         2198     2586     +388     
- Misses         80       83       +3     
+ Partials        3        2       -1     

Impacted Files	Coverage Δ
src/react/Hydrate.tsx
src/react/reactBatchedUpdates.ts
src/core/retryer.ts
src/core/onlineManager.ts
src/devtools/utils.ts
src/devtools/theme.tsx
src/react/logger.ts
src/react/useBaseQuery.ts
src/react/logger.native.ts
src/react/QueryClientProvider.tsx
... and 92 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.