fix(types): improve useDataQuery types #1347
Conversation
Birkbjo
changed the title
| } | ||
|
|
||
| export interface QueryState { | ||
| export interface QueryState<TQueryResult> { |
There was a problem hiding this comment.
Would prefer to rename this to QueryResultData
kabaros
changed the base branch from
master
to
eh/fix/AddGenericTypesToUseDataQuery
|
Could you add some docs with examples? |
Birkbjo
changed the title
kabaros
force-pushed
the
eh/fix/AddGenericTypesToUseDataQuery
branch
from
fbcbaa9 to
f8bb081
Compare
|
|
||
| export type Query = Record<string, ResourceQuery> | ||
| export type QueryResult = JsonMap | ||
| export type QueryResult = any |
There was a problem hiding this comment.
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.
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 thedataobject when not passing in a generic type touseDataQuery.QueryResultto beanyinstead 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.Pagertype 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