-
Notifications
You must be signed in to change notification settings - Fork 30.3k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Update react-query type definition for v1 #42830
Conversation
@lukaszfiszer Thank you for submitting this PR! 🔔 @JaceHensley @matteofrana - please review this PR in the next few days. Be sure to explicitly select If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
@lukaszfiszer The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
Lot's of code style errors - will fix them soon |
@lukaszfiszer The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
Since you're a listed owner and the build passed, this PR is fast-tracked. A maintainer will merge shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped! |
@lukaszfiszer what do you think about a discriminated union on |
@purge you mean discriminated union of QueryResult interface on status and date fields? |
@purge - discriminated union implemented |
@lukaszfiszer nice! i checked the source earlier and it does appear that data can still be set for states other than 'success' so you might wanna make that an |
@rbuckton can you mark the changes you'd requested as resolved? Otherwise it won't be merged I guess |
🔔 @rbuckton - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate? |
New version of react-query has just been released
|
|
@rbuckton - can you review and approve? |
@lukaszfiszer - It appears Travis did not correctly run on this PR! /cc @RyanCavanaugh to investigate and advise. |
I have two problems with latest typings of |
@nucleartux you're right. The docs were not very clear on that. I took some time trying to fix the types, but failed so far. It would probably require a rewrite of |
@@ -36,107 +53,218 @@ export interface QueryOptions<TResult> { | |||
refetchOnWindowFocus?: boolean; | |||
onError?: (err: any) => void; | |||
onSuccess?: (data: TResult) => void; | |||
onSettled?: (data: TResult, err: any) => void; | |||
suspense?: boolean; | |||
initialData?: TResult; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initialData can be a function
queryFn: QueryFunction<TResult, TVariables>, | ||
options?: QueryOptions<TResult> | ||
): QueryResult<TResult, TVariables>; | ||
export function useQuery<TResult, TQueryKey extends QueryKey>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useQuery can accept object as argument (same for usePaginatedQuery and useInfiniteQuery)
useQuery({
queryKey,
queryFn,
variables,
config
})
paginated: true; | ||
getCanFetchMore: (lastPage: TResult, allPages: TResult[]) => boolean; | ||
export interface InfiniteQueryOptions<TResult, FetchMoreVariable> extends QueryOptions<TResult> { | ||
getFetchMore?: ((lastGroup: TResult, allGroups: TResult[]) => FetchMoreVariable) | boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be?
getFetchMore?: ((lastGroup: TResult, allGroups: TResult[]) => FetchMoreVariable) | boolean; | |
getFetchMore?: (lastGroup: TResult, allGroups: TResult[]) => FetchMoreVariable | boolean; |
From what I understand in the doc and the code it's the result of the function that is either a FetchMoreVariable
or a boolean
.
Maybe this could be the right typing:
getFetchMore?: ((lastGroup: TResult, allGroups: TResult[]) => FetchMoreVariable) | boolean; | |
getFetchMore?: (lastGroup: TResult, allGroups: TResult[]) => FetchMoreVariable | false; |
At this point I think it would be better to get something merged, and then tweak it later. |
IMHO an incomplete initial implementation is fine, but it shouldn't flag errors on correct code, which these types currently do. |
Just curious, I know the rbuckton approval appears to be blocking this - but he hasn’t been around to confirm the changes. What about creating a new PR and getting that re-approved if that’s the blocker? (I’m not debating anyone else’s more active concerns, but in these times you never know why someone’s away) @lukaszfiszer thanks for the work so far! |
queryFn: QueryFunction<TResult, TVariables>, | ||
options?: QueryOptions<TResult> | ||
): QueryResult<TResult, TVariables>; | ||
export function useQuery<TResult, TQueryKey extends QueryKey>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yo Łukasz,
What do you think about this:
export function useQuery<TResult, TQueryKey extends QueryKey>(
queryKey: TQueryKey | (() => TQueryKey),
queryFn: QueryFunction<TResult, TVariables>, queryFn: (key: string, ...args: QueryKeyVariablesArgs<TQueryKey>) => Promise<TResult>,
options?: QueryOptions<TResult> options?: QueryOptions<TResult>,
): QueryResult<TResult, TVariables>; ): QueryState<TResult>;
export function useQuery<TResult, TQueryKey extends QueryKey, TQueryVariables extends any[]>(
queryKey: TQueryKey | (() => TQueryKey),
queryVariables: TQueryVariables,
queryFn: QueryFunction<TResult, TVariables>, queryFn: (key: string, ...args: Concat<QueryKeyVariablesArgs<TQueryKey>, TQueryVariables>) => Promise<TResult>,
options?: QueryOptions<TResult> options?: QueryOptions<TResult>,
): QueryResult<TResult, TVariables>; ): QueryState<TResult>;
?
This is simple solution for optional queryVariables, it's not perfect, but for first version of types for React-Query should be fine.
Concat<T, R>
is implementation from typescript-tuple package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mistakenly have been working on the same problem wich resulted in my PR #43438
I think I made type inference for functions, keys, and variables to work quite well. So I checked the tests I used with this PR and found a few issues that you might want to add to your cases as well.
const queryFn = (name: string, params: { bar: string }) => Promise.resolve(10);
declare const condition: boolean;
// Query with falsey query key
useQuery(condition && ['foo', { bar: 'baz' }], queryFn);
// ~~~~~~~
// Argument of type '(name: string, params: { bar: string; }) => Promise<number>' is not assignable to parameter of type '(key: string, ...args: [] | [{ bar: string; }]) => Promise<number>'.
// Types of parameters 'params' and 'args' are incompatible.
// Type '[] | [{ bar: string; }]' is not assignable to type '[{ bar: string; }]'.
// Property '0' is missing in type '[]' but required in type '[{ bar: string; }]'.
// This is just missing object API (when all the parameters passed as an object)
useQuery({
queryKey: condition && ['foo', { bar: 'baz' }],
queryFn,
});
//~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// Expected 2-3 arguments, but got 1.
// An argument for 'queryFn' was not provided.
const query2 = useQuery(
['key', { a: 1 }],
[{ b: true }, { c: 'c' }],
//~~~~~~~~~~~~~~~~~~~~~~~
// Argument of type '({ b: boolean; } | { c: string; })[]' is not assignable to parameter of type '(key: string, args_1: { a: number; }) => Promise<unknown>'.
// Type '({ b: boolean; } | { c: string; })[]' provides no match for the signature '(key: string, args_1: { a: number; }): Promise<unknown>'.
//
(
key1,
//~~ Parameter 'key1' implicitly has an 'any' type.
key2,
//~~ Parameter 'key2' implicitly has an 'any' type.
var1,
//~~ Parameter 'var1' implicitly has an 'any' type.
var2,
//~~ Parameter 'var2' implicitly has an 'any' type.
) => Promise.resolve(key1 === 'key' && key2.a === 1 && var1.b === true && var2.c === 'c'),
);
query2.data; // $ExpectType undefined | boolean
// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// Expected: undefined | boolean; got: unknown
- fix a few issues
Please fill in this template.
npm test
.)npm run lint package-name
(ortsc
if notslint.json
is present).If changing an existing definition:
tslint.json
containing{ "extends": "dtslint/dt.json" }
. If for reason the any rule need to be disabled, disable it for that line using// tslint:disable-next-line [ruleName]
and not for whole package so that the need for disabling can be reviewed.