-
Notifications
You must be signed in to change notification settings - Fork 13
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
fix(types): improve useDataQuery types #1347
base: master
Are you sure you want to change the base?
Conversation
services/data/src/types.ts
Outdated
@@ -40,19 +40,22 @@ export interface ExecuteHookResult<ReturnType> { | |||
data?: ReturnType | |||
} | |||
|
|||
export interface QueryState { | |||
export interface QueryState<TQueryResult> { |
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.
Would prefer to rename this to QueryResultData
Could you add some docs with examples? |
fbcbaa9
to
f8bb081
Compare
@@ -19,7 +18,11 @@ export interface ResolvedResourceQuery extends ResourceQuery { | |||
} | |||
|
|||
export type Query = Record<string, ResourceQuery> | |||
export type QueryResult = JsonMap | |||
export type QueryResult = any |
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.
We could potentially use unknown
here instead, but then we are enforcing consumers to type their data. Which could be a breaking change. But because JsonMap
-type don't really work anyway (and you had to do the same thing as in screenshot below), I'm all for using unknown
if we want to enforce typing the result. any
could be "easier" to use, but of course then you lose the types...
If we used unknown
you would have to do something like this (which you have to do now anyway with JsonMap
:
docs/hooks/useDataQuery.md
Outdated
// keep previous data, so list does not disappear when refetching/fetching new page | ||
const prevData = React.useRef(data) | ||
const stableData = data || prevData.current | ||
React.useEffect(() => { | ||
if(data) { | ||
prevData.current = data | ||
} | ||
}, [data]) |
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.
Wasn't really sure if I should include this in the example. I do think we should either add an option to useQuery
for react-query
's keepPreviousData
. Or preferably just have an reactQueryOptions
that we can pass directly... But I do understand the argument if we want to keep the APIs separate, to not rely on react-query
if we were to replace it...
However I do think an example like this would be bad without the functionality of "keeping" old data when using pagination - especially since all lists would flicker/be hidden regardless of what "pager" updated, because data
will be undefined when fetching... And since it's out of scope for this PR to have an option like that, I opted to include this pretty simple workaround in the example. And we can remove it/update it we decide to implement an option for this.
Hello @Birkbjo I have improvement suggestions if you're still working on this pull request. I think the generic type passed on the |
Key features
query
-keys are typed on thedata
object when not passing in a generic type touseDataQuery
.QueryResult
to beany
instead ofJsonMap
, due to problems withJsonMap
. Please refer to this comment for more context.engine.query
, so it follows the same types asuseDataQuery
.useDataQuery
, because when I first worked with this I was very confused by theQueryRenderInput
-name of the result ofuseDataQuery
.Pager
type and some related utility types to make it easy to create aResult
-type that is paged.Description
Builds on @eirikhaugstulen improvement of adding a generic type for
useDataQuery
.Checklist
Screenshots
Type-completion on query-keys when not passing in a generic type