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

better type narrowing for initialData #3310

Closed
TkDodo opened this issue Feb 18, 2022 · 16 comments · Fixed by #3834
Closed

better type narrowing for initialData #3310

TkDodo opened this issue Feb 18, 2022 · 16 comments · Fixed by #3834
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed types v4

Comments

@TkDodo
Copy link
Collaborator

TkDodo commented Feb 18, 2022

when providing initialData directly on a query, the type of data is guaranteed to be TData, not TData | undefined:

const { data } = useQuery(['test'], () => Promise.resolve('test'), { initialData: 'something' })

// 🚨 Object is possibly 'undefined'.(2532)
data.toUpperCase()

TypeScript playground

ideally, we could narrow the type when initialData is provided. This only works for initialData because initialData is executed in the constructor of the query, and because it is synchronous. Also, it can only work on v4 because in v3, undefined is technically still valid for the TData generic, so we cannot remove it from the union. In v4, undefined is illegal for successful queries, so that would now work.

Keep in mind that initialData can also be a function that returns TData | undefined, so this should also narrow if the function returns TData.

It does NOT work for:

  • placeholderData, because that will revert back to undefined when a query errors
  • initialData being provided globally, because TS cannot know that on an instance level. It needs to be explicitly passed to useQuery.
  • suspense or any other case because a query can be cancelled while it is initially fetching, in which case it will be reset to its initial state. queryClient.resetQueries also does that. Also, without network connection in the default network mode, the query will be in fetchStatus: 'paused' and will also not have data.

I guess we could potentially do this with another overload that has initialData provided and removes undefined from the result union, but we would also have do this for useQueries.
I would like to avoid having to add another generic to useQuery, as we already have four of them.

Because this is for v4, PRs should go to the beta branch please.

@TkDodo TkDodo added enhancement New feature or request help wanted Extra attention is needed types v4 labels Feb 18, 2022
@kettanaito
Copy link

So whenever initialData is provided, the returned data property is never undefined?

@TkDodo
Copy link
Collaborator Author

TkDodo commented Feb 19, 2022

@kettanaito yes, that is correct. cached data is never removed and initialData synchronously populates the cache. Even if you programmatically remove the query and re-render, it will be recreated with initialData. Even if the query errors, the data is still kept.

I see no scenario where data can be undefined if initialData is provided :)

@TkDodo
Copy link
Collaborator Author

TkDodo commented Feb 19, 2022

Correction: select can give you undefined, but that's a separate generic. So it correctly should mean that the TQueryFnData generic can never be undefined. TData defaults to TQueryFnData and is only different if select is provided.

@henribru
Copy link
Contributor

I see no scenario where data can be undefined if initialData is provided :)

You can still pass a function to initialData that returns undefined, I assume? E.g. if it gets the initial data from cache you might not have what you need in the cache. So this narrowing shouldn't apply in that case, right?

@TkDodo
Copy link
Collaborator Author

TkDodo commented Feb 19, 2022

@henribru yes, but I don' see a difference between:

initialData: undefined
initialData: 'foo'

and

initialData: () => undefined
initialData: () => 'foo'

as the signature for initialData is:

initialData?: TData | (() => TData | undefined)

so, yes, to be absolutely correct: As long as initialData is set to a value of type TData or a function that returns TData, you will always have a value in the cache.

I think we would need to look at the type of initialData, and if it's a function, look at the ReturnType instead. If that does not contain undefined in the union, we can perform the narrowing on the result.

@prateek3255
Copy link
Contributor

@TkDodo I'd like to work on this issue

@yss14
Copy link
Contributor

yss14 commented Apr 12, 2022

I guess we could potentially do this with another overload that has initialData provided and removes undefined from the result union, but we would also have do this for useQueries.
I would like to avoid having to add another generic to useQuery, as we already have four of them.

To my knowledge using an overload-based approach is not possible with typescript at the moment (see this playground). A return type of a function can not be inferred based on whether a certain object property is set or not. This can only be solved by introducing a generic for the query options and using conditional types based on that generic for the function return type.

But maybe some typescript guru can prove me wrong :)

@prateek3255
Copy link
Contributor

@TkDodo Update here, I was trying to do it via conditional types with generic but that wasn't working as expected. So I have asked a question for it on StackOverflow. And as mentioned by @yss14 even the overload based approach also doesn't seem to be working as expected. Do you have any other ideas on what else we can try out here?

@yss14
Copy link
Contributor

yss14 commented Apr 17, 2022

@prateek3255 I answered your SO question :)

Here is my working playground for the useQuery hook.

@prateek3255
Copy link
Contributor

Thanks @yss14. Will check it out 🙌

@TkDodo
Copy link
Collaborator Author

TkDodo commented Apr 20, 2022

initialData can be a function as well, but apart from that, the playground seems to work well 👍 . It can also be a function that returns undefined - not sure if that makes it more complicated?

@CoLdAs1cE
Copy link

Question not relate or related?
But when using suspense globally
Data type is of Type | undefined,
But shouldn't data be of Type only?
Since if error it will trow to error boundary, so it cannot be undefined if my type does not allow undefined?

(i just started using react-query so i am not well know about how it works, was searching issues and this was the closest related to my problem 😄 )

@TkDodo
Copy link
Collaborator Author

TkDodo commented Apr 22, 2022

Doesn't work. You can always say enabled: false and you won't have data. Also networking offline might put you in paused state. Also the edge case about cancelQueries I mentioned in the issue description.

@prateek3255
Copy link
Contributor

So I tried using the solution by @yss14 but the type narrowing still doesn't seem to be working as expected when I try using it with the basic-typescript example. Also I get a few type errors due to the return useBaseQuery hook because of the conditional types we added, and we I try to fix that by updating the types for QueryObserverResult as well then it starts causing type errors at a bunch of other places, so avoiding updating the types at all those places for now, by using a ts-expect-error in useQuery for now.

I have also updated the basic-typescript a little bit to test the changes. Opened a draft PR with #3557

@prateek3255
Copy link
Contributor

I tinkered a little bit more and was able to fix the type errors and get one of the overloads working that @yss14 demonstrated with the playground, have pushed the updated code in the same PR, but now the problem is that one of a few other overloads are not working at all, and breaking the existing types.

For instance the useQuery overload where it just takes one argument which are all the options, returns unknown as the type for data when I make the generic changes to the options. Here is the same playground that @yss14 shared which uses the overload that returns unknown type for the data.

Although there are some other type errors within, the tests as well with this approach, but we'll be able to figure them out, once we figure out what's causing this issue. @TkDodo @yss14 Do you have any idea on what's wrong with the above playground?

@mattpocock
Copy link
Contributor

Likely a naive attempt here:

#3834

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed types v4
Projects
None yet
7 participants