Skip to content
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

(feat) Add common typings for patient and person #1001

Merged
merged 2 commits into from
May 20, 2024
Merged

(feat) Add common typings for patient and person #1001

merged 2 commits into from
May 20, 2024

Conversation

chibongho
Copy link
Contributor

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. Ensure your PR title includes a conventional commit label (such as feat, fix, or chore, among others). See existing PR titles for inspiration.

For changes to apps

If applicable

  • My work includes tests or is validated by existing tests.
  • I have updated the esm-framework mock to reflect any API changes I have made.

Summary

Screenshots

Related Issue

Other

Copy link
Contributor

github-actions bot commented May 17, 2024

Size Change: -383 kB (-10.02%) 👏

Total Size: 3.44 MB

Filename Size Change
packages/shell/esm-app-shell/dist/openmrs.70d599f20d12b4b7.js 0 B -383 kB (removed) 🏆
ℹ️ View Unchanged
Filename Size
packages/apps/esm-devtools-app/dist/344.js 27.3 kB
packages/apps/esm-devtools-app/dist/630.js 2.69 kB
packages/apps/esm-devtools-app/dist/667.js 6.96 kB
packages/apps/esm-devtools-app/dist/735.js 2.63 kB
packages/apps/esm-devtools-app/dist/788.js 42.9 kB
packages/apps/esm-devtools-app/dist/875.js 9.86 kB
packages/apps/esm-devtools-app/dist/884.js 15.2 kB
packages/apps/esm-devtools-app/dist/889.js 157 kB
packages/apps/esm-devtools-app/dist/988.js 328 B
packages/apps/esm-devtools-app/dist/main.js 3.14 kB
packages/apps/esm-devtools-app/dist/openmrs-esm-devtools-app.js 3.19 kB
packages/apps/esm-implementer-tools-app/dist/15.js 62.2 kB
packages/apps/esm-implementer-tools-app/dist/271.js 716 B
packages/apps/esm-implementer-tools-app/dist/319.js 637 B
packages/apps/esm-implementer-tools-app/dist/426.js 24.8 kB
packages/apps/esm-implementer-tools-app/dist/460.js 735 B
packages/apps/esm-implementer-tools-app/dist/482.js 15.2 kB
packages/apps/esm-implementer-tools-app/dist/528.js 133 kB
packages/apps/esm-implementer-tools-app/dist/56.js 3.07 kB
packages/apps/esm-implementer-tools-app/dist/560.js 13.9 kB
packages/apps/esm-implementer-tools-app/dist/574.js 560 B
packages/apps/esm-implementer-tools-app/dist/587.js 2.92 kB
packages/apps/esm-implementer-tools-app/dist/620.js 126 kB
packages/apps/esm-implementer-tools-app/dist/625.js 562 B
packages/apps/esm-implementer-tools-app/dist/644.js 717 B
packages/apps/esm-implementer-tools-app/dist/657.js 7.01 kB
packages/apps/esm-implementer-tools-app/dist/71.js 6.97 kB
packages/apps/esm-implementer-tools-app/dist/735.js 2.63 kB
packages/apps/esm-implementer-tools-app/dist/757.js 560 B
packages/apps/esm-implementer-tools-app/dist/788.js 42.9 kB
packages/apps/esm-implementer-tools-app/dist/791.js 284 B
packages/apps/esm-implementer-tools-app/dist/807.js 559 B
packages/apps/esm-implementer-tools-app/dist/833.js 681 B
packages/apps/esm-implementer-tools-app/dist/86.js 6.71 kB
packages/apps/esm-implementer-tools-app/dist/889.js 157 kB
packages/apps/esm-implementer-tools-app/dist/main.js 79 kB
packages/apps/esm-implementer-tools-app/dist/openmrs-esm-implementer-tools-app.js 3.31 kB
packages/apps/esm-login-app/dist/111.js 1.22 kB
packages/apps/esm-login-app/dist/126.js 2.5 kB
packages/apps/esm-login-app/dist/173.js 1.22 kB
packages/apps/esm-login-app/dist/224.js 256 B
packages/apps/esm-login-app/dist/236.js 272 B
packages/apps/esm-login-app/dist/240.js 364 B
packages/apps/esm-login-app/dist/271.js 718 B
packages/apps/esm-login-app/dist/272.js 264 B
packages/apps/esm-login-app/dist/319.js 679 B
packages/apps/esm-login-app/dist/336.js 234 B
packages/apps/esm-login-app/dist/363.js 30.5 kB
packages/apps/esm-login-app/dist/460.js 737 B
packages/apps/esm-login-app/dist/539.js 298 B
packages/apps/esm-login-app/dist/56.js 3.06 kB
packages/apps/esm-login-app/dist/574.js 577 B
packages/apps/esm-login-app/dist/625.js 579 B
packages/apps/esm-login-app/dist/627.js 257 B
packages/apps/esm-login-app/dist/63.js 16.5 kB
packages/apps/esm-login-app/dist/644.js 718 B
packages/apps/esm-login-app/dist/667.js 6.96 kB
packages/apps/esm-login-app/dist/673.js 284 B
packages/apps/esm-login-app/dist/735.js 2.63 kB
packages/apps/esm-login-app/dist/757.js 660 B
packages/apps/esm-login-app/dist/788.js 42.9 kB
packages/apps/esm-login-app/dist/807.js 897 B
packages/apps/esm-login-app/dist/833.js 684 B
packages/apps/esm-login-app/dist/836.js 22.5 kB
packages/apps/esm-login-app/dist/884.js 15.2 kB
packages/apps/esm-login-app/dist/889.js 157 kB
packages/apps/esm-login-app/dist/main.js 57 kB
packages/apps/esm-login-app/dist/openmrs-esm-login-app.js 3.37 kB
packages/apps/esm-offline-tools-app/dist/271.js 1.18 kB
packages/apps/esm-offline-tools-app/dist/319.js 1.13 kB
packages/apps/esm-offline-tools-app/dist/460.js 1.29 kB
packages/apps/esm-offline-tools-app/dist/56.js 3.07 kB
packages/apps/esm-offline-tools-app/dist/574.js 1.04 kB
packages/apps/esm-offline-tools-app/dist/59.js 56.5 kB
packages/apps/esm-offline-tools-app/dist/625.js 1.04 kB
packages/apps/esm-offline-tools-app/dist/63.js 16.5 kB
packages/apps/esm-offline-tools-app/dist/644.js 1.18 kB
packages/apps/esm-offline-tools-app/dist/667.js 6.96 kB
packages/apps/esm-offline-tools-app/dist/735.js 2.63 kB
packages/apps/esm-offline-tools-app/dist/757.js 1.2 kB
packages/apps/esm-offline-tools-app/dist/788.js 42.9 kB
packages/apps/esm-offline-tools-app/dist/807.js 1.11 kB
packages/apps/esm-offline-tools-app/dist/833.js 1.21 kB
packages/apps/esm-offline-tools-app/dist/884.js 15.2 kB
packages/apps/esm-offline-tools-app/dist/889.js 157 kB
packages/apps/esm-offline-tools-app/dist/922.js 90.9 kB
packages/apps/esm-offline-tools-app/dist/main.js 147 kB
packages/apps/esm-offline-tools-app/dist/openmrs-esm-offline-tools-app.js 3.29 kB
packages/apps/esm-primary-navigation-app/dist/271.js 267 B
packages/apps/esm-primary-navigation-app/dist/319.js 237 B
packages/apps/esm-primary-navigation-app/dist/460.js 264 B
packages/apps/esm-primary-navigation-app/dist/553.js 23.3 kB
packages/apps/esm-primary-navigation-app/dist/574.js 230 B
packages/apps/esm-primary-navigation-app/dist/625.js 232 B
packages/apps/esm-primary-navigation-app/dist/63.js 16.5 kB
packages/apps/esm-primary-navigation-app/dist/644.js 267 B
packages/apps/esm-primary-navigation-app/dist/667.js 6.97 kB
packages/apps/esm-primary-navigation-app/dist/735.js 2.64 kB
packages/apps/esm-primary-navigation-app/dist/757.js 238 B
packages/apps/esm-primary-navigation-app/dist/762.js 7.61 kB
packages/apps/esm-primary-navigation-app/dist/788.js 42.9 kB
packages/apps/esm-primary-navigation-app/dist/807.js 290 B
packages/apps/esm-primary-navigation-app/dist/833.js 257 B
packages/apps/esm-primary-navigation-app/dist/884.js 15.2 kB
packages/apps/esm-primary-navigation-app/dist/889.js 157 kB
packages/apps/esm-primary-navigation-app/dist/958.js 22.7 kB
packages/apps/esm-primary-navigation-app/dist/main.js 47.6 kB
packages/apps/esm-primary-navigation-app/dist/openmrs-esm-primary-navigation-app.js 3.23 kB
packages/framework/esm-api/dist/openmrs-esm-api.js 16.3 kB
packages/framework/esm-config/dist/openmrs-esm-module-config.js 8.02 kB
packages/framework/esm-context/dist/openmrs-esm-context.js 1.1 kB
packages/framework/esm-dynamic-loading/dist/openmrs-esm-dynamic-loading.js 2.75 kB
packages/framework/esm-error-handling/dist/openmrs-esm-error-handling.js 889 B
packages/framework/esm-extensions/dist/openmrs-esm-extensions.js 8.14 kB
packages/framework/esm-feature-flags/dist/openmrs-esm-feature-flags.js 1.67 kB
packages/framework/esm-framework/dist/126.openmrs-esm-framework.js 2.47 kB
packages/framework/esm-framework/dist/278.openmrs-esm-framework.js 14.5 kB
packages/framework/esm-framework/dist/530.openmrs-esm-framework.js 2.92 kB
packages/framework/esm-framework/dist/619.openmrs-esm-framework.js 6.49 kB
packages/framework/esm-framework/dist/645.openmrs-esm-framework.js 9.31 kB
packages/framework/esm-framework/dist/680.openmrs-esm-framework.js 6.13 kB
packages/framework/esm-framework/dist/735.openmrs-esm-framework.js 2.66 kB
packages/framework/esm-framework/dist/788.openmrs-esm-framework.js 42.9 kB
packages/framework/esm-framework/dist/openmrs-esm-framework.js 477 kB
packages/framework/esm-globals/dist/openmrs-esm-globals.js 796 B
packages/framework/esm-navigation/dist/openmrs-esm-navigation.js 9.35 kB
packages/framework/esm-offline/dist/openmrs-esm-offline.js 34.4 kB
packages/framework/esm-react-utils/dist/openmrs-esm-react-utils.js 15.6 kB
packages/framework/esm-routes/dist/openmrs-esm-utils.js 1.46 kB
packages/framework/esm-state/dist/openmrs-esm-state.js 921 B
packages/framework/esm-styleguide/dist/openmrs-esm-styleguide.js 53.8 kB
packages/framework/esm-translations/dist/openmrs-esm-core-translations.js 1.59 kB
packages/framework/esm-utils/dist/openmrs-esm-utils.js 11.2 kB
packages/shell/esm-app-shell/dist/11c63b65f96a8718.js 499 B
packages/shell/esm-app-shell/dist/1e0131662341578e.js 645 B
packages/shell/esm-app-shell/dist/2916d0aa7a9d5dc8.js 544 B
packages/shell/esm-app-shell/dist/4a3e954c45d63305.js 645 B
packages/shell/esm-app-shell/dist/555334eb840d9a34.js 6.91 kB
packages/shell/esm-app-shell/dist/56c2295bc732ae32.js 722 B
packages/shell/esm-app-shell/dist/651172ae1548469c.js 499 B
packages/shell/esm-app-shell/dist/7ba811602c7a0036.js 1.58 kB
packages/shell/esm-app-shell/dist/98343ad4bb547c48.js 499 B
packages/shell/esm-app-shell/dist/b5151d35f680b40a.js 3.82 kB
packages/shell/esm-app-shell/dist/ba933133ad512cac.js 499 B
packages/shell/esm-app-shell/dist/fa89289c9d9645c9.js 519 B
packages/shell/esm-app-shell/dist/openmrs.bafd5a6754f571eb.js 383 kB
packages/shell/esm-app-shell/dist/service-worker.js 45.9 kB
packages/tooling/openmrs/dist/cli.js 2.88 kB
packages/tooling/openmrs/dist/commands/assemble.js 2.82 kB
packages/tooling/openmrs/dist/commands/build.js 1.34 kB
packages/tooling/openmrs/dist/commands/debug.js 545 B
packages/tooling/openmrs/dist/commands/develop.js 2.59 kB
packages/tooling/openmrs/dist/commands/index.js 438 B
packages/tooling/openmrs/dist/commands/start.js 851 B
packages/tooling/openmrs/dist/index.js 517 B
packages/tooling/openmrs/dist/runner.js 637 B
packages/tooling/openmrs/dist/utils/config.js 728 B
packages/tooling/openmrs/dist/utils/debugger.js 576 B
packages/tooling/openmrs/dist/utils/dependencies.js 648 B
packages/tooling/openmrs/dist/utils/helpers.js 395 B
packages/tooling/openmrs/dist/utils/importmap.js 3.07 kB
packages/tooling/openmrs/dist/utils/index.js 444 B
packages/tooling/openmrs/dist/utils/logger.js 368 B
packages/tooling/openmrs/dist/utils/npmConfig.js 830 B
packages/tooling/openmrs/dist/utils/untar.js 722 B
packages/tooling/openmrs/dist/utils/variables.js 192 B
packages/tooling/openmrs/dist/utils/webpack.js 278 B
packages/tooling/webpack-config/dist/index.js 3.61 kB

compressed-size-action

Copy link
Member

@mseaton mseaton left a comment

Choose a reason for hiding this comment

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

Generally LGTM. Are these mostly copied out of the work we did in service queues? I'd want to validate that these interfaces exactly match the REST responses - can you confirm?

attributeType: OpenmrsResource;
display: string;
uuid: string;
value: string | number;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is necessarily accurate. Can you check on this typing for value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right. This should be a string.

@chibongho
Copy link
Contributor Author

Generally LGTM. Are these mostly copied out of the work we did in service queues? I'd want to validate that these interfaces exactly match the REST responses - can you confirm?

Yeah, most of these were from when we were making these types together in the service queues module months ago.

@ibacher
Copy link
Member

ibacher commented May 17, 2024

One issue here is how we handle the difference between different representations. I'm not even necessarily thinking about custom representations, but, e.g., the default REF format for a person (which is returned in some circumstances):

interface Person {
  uuid: string;
  display: string;
  links: Array<{ rel: string; uri: string; resourceAlias?: string; }>;
}

Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

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

Couple of quick thoughts here to make the types a little more reflective of the API.

Comment on lines 19 to 20
deathDate: string;
causeOfDeath: Concept;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
deathDate: string;
causeOfDeath: Concept;
deathDate: string | null;
causeOfDeath: Concept | null;

names: Array<PersonName>;
addresses: Array<PersonAddress>;
attributes: Array<PersonAttribute>;
birthtime: string;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
birthtime: string;
birthtime: string | null;

birthtime: string;
deathdateEstimated: boolean;
causeOfDeathNonCoded: string;
links: Array<any>;
Copy link
Member

Choose a reason for hiding this comment

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

Links are always in this format:

export interface Link {
  rel: string;
  uri: string;
  resourceAlias?: string;
}
Suggested change
links: Array<any>;
links: Array<Link>;

dead: boolean;
deathDate: string;
causeOfDeath: Concept;
preferredName: PersonName;
Copy link
Member

Choose a reason for hiding this comment

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

Technically, this will be:

Suggested change
preferredName: PersonName;
preferredName: Pick<PersonName, 'uuid' | 'display' | 'links'>;

Unless v=full is supplied.

attributes: Array<PersonAttribute>;
birthtime: string;
deathdateEstimated: boolean;
causeOfDeathNonCoded: string;
Copy link
Member

Choose a reason for hiding this comment

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

This one is also nullable.

@ibacher ibacher changed the title (chore) - add common typings for patient and user (feat) - add common typings for patient and user May 17, 2024
@chibongho
Copy link
Contributor Author

Generally LGTM. Are these mostly copied out of the work we did in service queues? I'd want to validate that these interfaces exactly match the REST responses - can you confirm?

@mseaton these interfaces are definitely incomplete (for example, Patient has a voided field with v=full rep). They are probably good enough for most use cases, but I can add those missing fields.

One issue here is how we handle the difference between different representations. I'm not even necessarily thinking about custom representations, but, e.g., the default REF format for a person (which is returned in some circumstances):

@ibacher Thanks for the suggestions. Unless a field's type can change based on the representation, this should be ok right? I think it'll still helpful to see what fields are available for each type while having the expectation that it's up to the implementor to verify that the fields they need are indeed there.

@ibacher
Copy link
Member

ibacher commented May 17, 2024

Thanks for the suggestions. Unless a field's type can change based on the representation, this should be ok right?

I'm not really too concerned about that (it's fully possible with custom and nested objects). The real thing is I'm wondering if the types themselves should force the implementer to verify that the fields are there, i.e., something like:

interface Person extends Pick<FullPerson, 'uuid' | 'display'>, Partial<Omit<FullPerson, 'uuid' | 'display'>> {}

Because that would kind of force using something like:

const patientName: string = patient.person.preferredName ?? patient.person.name?[0] ?? 'UNKNOWN';

@chibongho
Copy link
Contributor Author

Thanks for the suggestions. Unless a field's type can change based on the representation, this should be ok right?

I'm not really too concerned about that (it's fully possible with custom and nested objects). The real thing is I'm wondering if the types themselves should force the implementer to verify that the fields are there

I don't like having that forcing everywhere, but I think we can try it and remove it later if we want (easier to go from restrictive to loose, instead of vice versa).

So how about something like this pattern? I think having every attribute being ? is equivalent to your example.

// this already exists:
export interface OpenmrsResource {
  uuid: string;
  display?: string;
  [anythingElse: string]: any;
}

// we add this as the base type for almost everything
export interface OpenmrsResourceStrict {
  uuid: string;
  display?: string;
  links?: Array<Link>;
}

interface Person extends OpenmrsResourceStrict  {
  // everything with a '?'
  givenName?: string;
  familyName?: string;
  ...
}

@ibacher
Copy link
Member

ibacher commented May 17, 2024

I don't like having that forcing everywhere

I'm not hugely enamored with my own proposal, but I think that having the type system setup like that will help some of our community developers do the right thing.

And, yeah, I'm happy with the pattern you suggest.

@chibongho chibongho changed the title (feat) - add common typings for patient and user (feat) - add common typings for patient and person May 17, 2024
@chibongho chibongho requested review from mseaton and ibacher May 17, 2024 21:09
@@ -0,0 +1,5 @@
import { type OpenmrsResource } from './openmrs-resource';

export interface Concept extends OpenmrsResource {
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be OpenmrsResourceStrict too? Should it have more properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not every type needed by the Patient type (directly or transitively) is completely typed yet. Specifically this Concept type and the User type below are just using OpenmrsResource as a flexible placeholder for now, while still being more correct than any. A completely typed type should extend OpenmrsResourceStrict instead. This convention should allow us to add more typings piecemeal.

@ibacher ibacher changed the title (feat) - add common typings for patient and person (feat) add common typings for patient and person May 20, 2024
@ibacher ibacher changed the title (feat) add common typings for patient and person (feat) Add common typings for patient and person May 20, 2024
@ibacher
Copy link
Member

ibacher commented May 20, 2024

Thanks @chibongho! I think this actually helps solve a long-standing problem we've had with types returned from APIs.

@ibacher ibacher merged commit 7feaf35 into main May 20, 2024
10 checks passed
@ibacher ibacher deleted the typings branch May 20, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants