fix: [DHIS2] Type generic T = QueryResult to useDataQuery #1297

eirikhaugstulen · 2022-12-07T11:21:07Z

Hey all,

We have a use-case in capture that I think multiple projects need as well:
We need to be able to type the data-object returned from the useDataQuery-hook. This is because the JsonMap type that is currently returned would error on almost any nested object - without giving any type safety.

I have written a quick example in this PR, which I expect would be very wrong, but it gives you an idea of what would be a solution for us. To summarize, I've just added a generic T and mapped this to the QueryResult type. For the end user, typing is still optional.

This would allow us to type the data returned like this:

Open for any suggestions and discussions!

JoakimSM

Looks good to me @eirikhaugstulen! (At least as a temporary solution until we strongly type the query result)

amcgee

Thanks @eirikhaugstulen, this is super useful and looks good to me! My one concern is that, as @JoakimSM mentioned, this might be a temporary fix until we have strong types for query responses (generated from the new OpenAPI spec) but that could still be a little way off. If we do get to that point this will no longer be a generic, so it will be a breaking change for the app-runtime consumer. I think that's fine though (a codemod could automatically strip the generic type on upgrade) and worth it if we can get better type safety in the interim.

Let's let someone from @dhis2/team-platform comment here as well before we merge

amcgee · 2022-12-13T15:22:50Z

Should we also fix the JsonMap default type so that it supports nested objects...?

HendrikThePendric · 2022-12-13T16:24:15Z

Should we also fix the JsonMap default type so that it supports nested objects...?

TypeScript novice here.... I'm just wondering if we were to fix the JsonMap to support nested objects, would TQueryResult still be needed?

kabaros · 2022-12-13T16:46:19Z

Thanks @eirikhaugstulen, this is super useful and looks good to me! My one concern is that, as @JoakimSM mentioned, this might be a temporary fix until we have strong types for query responses (generated from the new OpenAPI spec) but that could still be a little way off. If we do get to that point this will no longer be a generic, so it will be a breaking change for the app-runtime consumer. I think that's fine though (a codemod could automatically strip the generic type on upgrade) and worth it if we can get better type safety in the interim.

for this point, I don't totally get why it will be a breaking change, can't this still be a generic when we have strong types? .. the client would still be using useDataQuery<ConcreteApiType>(.... .. the ConcreteApiType would replace an internal type in the consumer, but it will be backwards compatible with the old useDataQuery<InternalTemporaryApiType>

kabaros

LGTM .. just a suggestion + question to clarify things a bit for me

kabaros · 2022-12-13T16:40:29Z

services/data/src/types.ts

@@ -40,15 +40,16 @@ export interface ExecuteHookResult<ReturnType> {
    data?: ReturnType
 }

-export interface QueryState {
+export interface QueryState<TQueryResult> {


would there be a benefit to restrict TQueryResult a bit to extend an interface, i.e.

Suggested change

export interface QueryState<TQueryResult> {

interface IQueryResult {

id: string; // properties that are always in a response.

}

export interface QueryState<TQueryResult extends IQueryResponse> {

called: boolean

loading: boolean

fetching: boolean

error?: FetchError

data?: TQueryResult

}

the concrete API responses - when they're added - should then adhere to IQueryResult.

I agree that some extensibility can be done to this, and I thought about making something similar, although I was a bit unsure as to how me make this generic enough. What happens if the user is filtering out some of these with the filter-property for example?

eirikhaugstulen · 2022-12-16T14:52:04Z

Should we also fix the JsonMap default type so that it supports nested objects...?

TypeScript novice here.... I'm just wondering if we were to fix the JsonMap to support nested objects, would TQueryResult still be needed?

I think we should definitively do something about the JSONMap-type that we return. As it stands now, it is more annoying to the end user than helpful - at least that's my experience using it. I would argue that as a first iteration, we should type what we know about the return and anything other than that should be any or unknown. We know we return an object with strings as keys, and that's pretty much it. We should also encourage the user to pass in a TQueryResult to overwrite the any-type that we put on the data returned from the API.

Therefore, in my opinion, the data returned from the hook should be TQueryResult | Record<string, any>
Any insights? @kabaros @amcgee @HendrikThePendric

Edit:
Adding some images to show what the record would do

Current solution:

With Record<string, any>

Birkbjo · 2023-02-08T14:53:04Z

Btw. is it only me that think QueryRenderInput type name is a bit confusing as a return result of useQuery? I understand it's used internally as an input to the declarative DataQuery-component, but most will use useDataQuery, and in that context it's quite confusing. Could just have a type "alias" and name it UseDataQueryResult or something similiar?

kabaros · 2023-04-20T14:10:17Z

@eirikhaugstulen looking at this PR again - do you think we need to address any of the pending questions before merging? I think it's a big improvement as it is (we need it in a new app using TS) and we can address the rest later, if there is a need.

eirikhaugstulen · 2023-04-20T14:17:52Z

Hey again, @kabaros!

Personally, I would agree with you, free to merge from my side. I do know that @Birkbjo was planning on doing some minor changes though. What do you think, Birk?

kabaros · 2023-05-01T10:40:27Z

ok, if no one objects then I will merge this one (since it's needed and already had few approvals) then we can discuss the pending questions and enhancements from @Birkbjo PR afterwards.

## [3.9.2](v3.9.1...v3.9.2) (2023-05-02) ### Bug Fixes * [DHIS2] Type generic T = QueryResult to useDataQuery ([#1297](#1297)) ([7c5c083](7c5c083))

dhis2-bot · 2023-05-02T09:04:37Z

🎉 This PR is included in version 3.9.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

* fix: added generic T = QueryResult * fix: changed to correct import * fix: prettier * fix: changed name of T * fix: prettier

## [3.9.2](v3.9.1...v3.9.2) (2023-05-02) ### Bug Fixes * [DHIS2] Type generic T = QueryResult to useDataQuery ([#1297](#1297)) ([7c5c083](7c5c083))

# [3.10.0-alpha.3](v3.10.0-alpha.2...v3.10.0-alpha.3) (2023-08-22) ### Bug Fixes * **connection-status:** responsiveness to online events [LIBS-497] ([#1348](#1348)) ([91a3d4d](91a3d4d)) * **types:** add generic result type to oncomplete param ([#1350](#1350)) ([a069603](a069603)) * [DHIS2] Type generic T = QueryResult to useDataQuery ([#1297](#1297)) ([7c5c083](7c5c083)) * account for daylight savings time [LIBS-490] ([06eaa5d](06eaa5d)) * account for daylight savings time [LIBS-490] [#1345](#1345) ([fb00533](fb00533)) * add test for when time zones are the same [LIBS-490] ([7911f8b](7911f8b))

dhis2-bot · 2023-08-22T15:19:24Z

🎉 This PR is included in version 3.10.0-alpha.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

eirikhaugstulen marked this pull request as ready for review December 7, 2022 13:03

eirikhaugstulen requested review from amcgee, HendrikThePendric and JoakimSM December 7, 2022 13:03

JoakimSM approved these changes Dec 7, 2022

View reviewed changes

amcgee approved these changes Dec 13, 2022

View reviewed changes

kabaros approved these changes Dec 13, 2022

View reviewed changes

Birkbjo mentioned this pull request Apr 17, 2023

fix(types): improve useDataQuery types #1347

Open

2 tasks

eirikhaugstulen added 5 commits May 1, 2023 13:40

fix: added generic T = QueryResult

a3d1081

fix: changed to correct import

e3518fe

fix: prettier

3bf86d6

fix: changed name of T

a5d527d

fix: prettier

f8bb081

kabaros force-pushed the eh/fix/AddGenericTypesToUseDataQuery branch from fbcbaa9 to f8bb081 Compare May 1, 2023 10:40

kabaros merged commit 7c5c083 into master May 2, 2023

kabaros deleted the eh/fix/AddGenericTypesToUseDataQuery branch May 2, 2023 08:54

dhis2-bot added a commit that referenced this pull request May 2, 2023

chore(release): cut 3.9.2 [skip ci]

0098290

## [3.9.2](v3.9.1...v3.9.2) (2023-05-02) ### Bug Fixes * [DHIS2] Type generic T = QueryResult to useDataQuery ([#1297](#1297)) ([7c5c083](7c5c083))

dhis2-bot added the released label May 2, 2023

amcgee pushed a commit that referenced this pull request Aug 22, 2023

fix: [DHIS2] Type generic T = QueryResult to useDataQuery (#1297)

aeb6f3b

* fix: added generic T = QueryResult * fix: changed to correct import * fix: prettier * fix: changed name of T * fix: prettier

amcgee pushed a commit that referenced this pull request Aug 22, 2023

chore(release): cut 3.9.2 [skip ci]

614a521

## [3.9.2](v3.9.1...v3.9.2) (2023-05-02) ### Bug Fixes * [DHIS2] Type generic T = QueryResult to useDataQuery ([#1297](#1297)) ([7c5c083](7c5c083))

dhis2-bot added the released on @alpha label Aug 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: [DHIS2] Type generic T = QueryResult to useDataQuery #1297

fix: [DHIS2] Type generic T = QueryResult to useDataQuery #1297

eirikhaugstulen commented Dec 7, 2022

JoakimSM left a comment

amcgee left a comment

amcgee commented Dec 13, 2022

HendrikThePendric commented Dec 13, 2022

kabaros commented Dec 13, 2022

kabaros left a comment

kabaros Dec 13, 2022 •

edited

Loading

eirikhaugstulen Dec 13, 2022

eirikhaugstulen commented Dec 16, 2022 •

edited

Loading

Birkbjo commented Feb 8, 2023 •

edited

Loading

kabaros commented Apr 20, 2023

eirikhaugstulen commented Apr 20, 2023

kabaros commented May 1, 2023

dhis2-bot commented May 2, 2023

dhis2-bot commented Aug 22, 2023

-export interface QueryState<TQueryResult> {
+interface IQueryResult {
+  id: string; // properties that are always in a response.
+}
+export interface QueryState<TQueryResult extends IQueryResponse> {
+   called: boolean
+    loading: boolean
+    fetching: boolean
+    error?: FetchError
+    data?: TQueryResult
+}

fix: [DHIS2] Type generic T = QueryResult to useDataQuery #1297

fix: [DHIS2] Type generic T = QueryResult to useDataQuery #1297

Conversation

eirikhaugstulen commented Dec 7, 2022

JoakimSM left a comment

Choose a reason for hiding this comment

amcgee left a comment

Choose a reason for hiding this comment

amcgee commented Dec 13, 2022

HendrikThePendric commented Dec 13, 2022

kabaros commented Dec 13, 2022

kabaros left a comment

Choose a reason for hiding this comment

kabaros Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

eirikhaugstulen Dec 13, 2022

Choose a reason for hiding this comment

eirikhaugstulen commented Dec 16, 2022 • edited Loading

Birkbjo commented Feb 8, 2023 • edited Loading

kabaros commented Apr 20, 2023

eirikhaugstulen commented Apr 20, 2023

kabaros commented May 1, 2023

dhis2-bot commented May 2, 2023

dhis2-bot commented Aug 22, 2023

kabaros Dec 13, 2022 •

edited

Loading

eirikhaugstulen commented Dec 16, 2022 •

edited

Loading

Birkbjo commented Feb 8, 2023 •

edited

Loading