Skip to content

fix(useQuery): let context infer queryKey #1955

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.

Sign up for GitHub

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

Closed
onursagir wants to merge 1 commit into from
Closed

fix(useQuery): let context infer queryKey #1955

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/core/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@ import type { QueryFilters } from './utils'

export type QueryKey = string | readonly unknown[]

export type QueryFunction<T = unknown> = (
context: QueryFunctionContext<any>
) => T | Promise<T>
export type QueryFunction<
T = unknown,
TQueryKey extends QueryKey = QueryKey
> = (context: QueryFunctionContext<TQueryKey>) => T | Promise<T>

export interface QueryFunctionContext<
TQueryKey extends QueryKey = QueryKey,
Expand Down
14 changes: 12 additions & 2 deletions src/react/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import {
InfiniteQueryObserverResult,
MutateOptions,
MutationStatus,
QueryFunction,
QueryKey,
QueryObserverOptions,
QueryObserverResult,
} from '../core/types'
Expand All @@ -18,8 +20,16 @@ export interface UseBaseQueryOptions<
export interface UseQueryOptions<
TQueryFnData = unknown,
TError = unknown,
TData = TQueryFnData
> extends UseBaseQueryOptions<TQueryFnData, TError, TData> {}
TData = TQueryFnData,
TQueryKey extends QueryKey = QueryKey
>
extends Omit<
UseBaseQueryOptions<TQueryFnData, TError, TData>,
'queryFn' | 'queryKey'
> {
Comment on lines +26 to +29
Copy link
Collaborator

@TkDodo TkDodo Mar 12, 2021

Choose a reason for hiding this comment

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

why not change this on UseBaseQueryOptions directly? As far as I can see, this is the only interface that extends UseBaseQueryOptions....

Copy link
Author

@onursagir onursagir Mar 12, 2021

Choose a reason for hiding this comment

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

there is a function in src/react/useBaseQuery.ts:9 that uses that uses the UseBaseQueryOptions interface, I mainly did it this way to keep the impact minimal

Copy link
Collaborator

@TkDodo TkDodo Mar 12, 2021

Choose a reason for hiding this comment

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

I think this goes hand-in-hand with the request for infinite query support, because src/react/useBaseQuery.ts:9 is only used for useQuery - which you have already adapted, and useInfiniteQuery. With regards to the queryKey, they are identical, so if you add useInfiniteQuery support to this PR, you could also get rid of the Omit I think.

Copy link
Author

@onursagir onursagir Mar 12, 2021

Choose a reason for hiding this comment

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

Yes I see what you mean now. I will update it accordingly.

Copy link
Author

@onursagir onursagir Mar 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I had a check and I don't think I can make it work like that the interfaces are put together like this

interface UseQueryOptions extends UseBaseQueryOptions extends QueryObserverOptions {}
interface UseInfiniteQueryOptions extends InfiniteQueryObserverOptions extends QueryObserverOptions {}

While it is true that useQuery and useInfiniteQuery do both use the useBaseQuery function, I don't think that matters when it comes to inferring the queryKey (which is either inferred through QueryFunction or UseQueryOptions).

Copy link
Collaborator

@TkDodo TkDodo Mar 13, 2021

Choose a reason for hiding this comment

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

@boschni what do you think?

Copy link
Collaborator

@boschni boschni Mar 13, 2021

Choose a reason for hiding this comment

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

Think we should try to keep the API consistent and implement things like this library wide instead of in one hook

Copy link
Author

@onursagir onursagir Mar 13, 2021

Choose a reason for hiding this comment

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

@boschni This pr includes no API changes, only a minor change to an interface to resolve a minor TypeScript inconvenience. I am not saying the interfaces for ouseInfiniteQuery can't be changed in order for inference to work. I'm saying it can't be done as @TkDodo describes

Copy link
Collaborator

@boschni boschni Mar 14, 2021

Choose a reason for hiding this comment

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

If someone is opting in to use this feature, they most likely want to use it throughout their codebase, and not only when using useQuery. At this moment you might only use useQuery, but others are also using queryClient.fetchQuery or useInfiniteQuery. If we provide support for this pattern, users should be able to expect that they can use it anywhere.

Copy link
Collaborator

@TkDodo TkDodo Mar 14, 2021

Choose a reason for hiding this comment

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

I’ll try to take another stab at this if i find the time - i think we’re already on the right track here 👍

queryKey?: TQueryKey
queryFn?: QueryFunction<TQueryFnData, TQueryKey>
}

export interface UseInfiniteQueryOptions<
Copy link
Collaborator

@TkDodo TkDodo Mar 12, 2021

Choose a reason for hiding this comment

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

could you maybe also add the support for infinite queries as well?

Copy link
Author

@onursagir onursagir Mar 12, 2021

Choose a reason for hiding this comment

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

I could in the future, but I think I would prefer to do that on a separate branch (and perhaps another one for useMutation?).

TQueryFnData = unknown,
Expand Down
14 changes: 8 additions & 6 deletions src/react/useQuery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,20 @@ export function useQuery<
export function useQuery<
TQueryFnData = unknown,
TError = unknown,
TData = TQueryFnData
TData = TQueryFnData,
TQueryKey extends QueryKey = QueryKey
>(
queryKey: QueryKey,
options?: UseQueryOptions<TQueryFnData, TError, TData>
queryKey: TQueryKey,
options?: UseQueryOptions<TQueryFnData, TError, TData, TQueryKey>
): UseQueryResult<TData, TError>
export function useQuery<
TQueryFnData = unknown,
TError = unknown,
TData = TQueryFnData
TData = TQueryFnData,
TQueryKey extends QueryKey = QueryKey
>(
queryKey: QueryKey,
queryFn: QueryFunction<TQueryFnData>,
queryKey: TQueryKey,
queryFn: QueryFunction<TQueryFnData, TQueryKey>,
options?: UseQueryOptions<TQueryFnData, TError, TData>
): UseQueryResult<TData, TError>
export function useQuery<TQueryFnData, TError, TData = TQueryFnData>(
Expand Down