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

Make "Reference" type generic? #70

Open
preston opened this issue Jul 21, 2021 · 6 comments
Open

Make "Reference" type generic? #70

preston opened this issue Jul 21, 2021 · 6 comments
Labels
enhancement New feature or request typescript Specific to export of Language TypeScript

Comments

@preston
Copy link

preston commented Jul 21, 2021

Would it be possible to make the TypeScript version of Reference generic? It is very difficult to enforce constraints without this, and TypeScript supports complex "A | B | C | D"-style generics.

We are trying to strictly enforce all manner of constraints in our information models for both IG publishing and software development purposes, and such refinements would be very welcome!

@GinoCanessa GinoCanessa added enhancement New feature or request typescript Specific to export of Language TypeScript labels Jul 21, 2021
@GinoCanessa
Copy link
Contributor

I like the idea, but I am struggling with what the output should look like.

In Bundle, we use the generic to apply type information to Bundle.entry.resource, which actually contains a FHIR object.

However, the datatype Reference doesn't contain an object - it just has an optional type element (which points to the URI of the reference type).

Is the goal that Reference<Patient> forces the literal Patient into Reference.type?

Thoughts: @bkaney @jdjkelly @ir4y @jmandel ?

@preston
Copy link
Author

preston commented Jul 22, 2021

Yup, exactly. Reference itself would be a generic type. Something along the lines of..

export interface Reference<T extends FhirResource> extends Element {
...
_type?: T;
...
}

..with the general pattern repeated for other resource types to match more closely with the FHIR specification and be more directly implementable without casting madness.

@GinoCanessa
Copy link
Contributor

Updating here for visibility. @jmandel suggested the following:

interface Reference<T extends Resource> {
 ...
  type?: `${T["resourceType"]}`,
 ...
}

Which, I believe is in line with what we are looking for (e.g., the type field is actually a string value). I haven't tested it yet, but it looks like a step in the right direction. I believe it will need a little more logic around undefined though.

@jdjkelly
Copy link
Contributor

jdjkelly commented Jul 29, 2021

From the string PR, this is a bit stricter (edit: tho I'm not sure is valid TS lol)

type?: `${T["resourceType"]}` as FhirResourceType

Although I suppose if you have T that is accessing ["resourceType"] you're probably okay already. Or did you mean something else by the undefined case @GinoCanessa ?

@ir4y
Copy link

ir4y commented Sep 1, 2021

It would be nice to check Reference.reference.

interface Reference<T extends Resource> {
 ...
  reference?: `${T["resourceType"]}/\s+`,
 ...
}

Now it is not possible because microsoft/TypeScript#41160 has to be approved first.
Also, we need to handle local references somehow.

Regarding defining the Reference.type personally, I like the @jmandel idea.

interface Reference<T extends Resource> {
 ...
  type?: `${T["resourceType"]}`,
 ...
}

However, based on type definition https://www.hl7.org/fhir/references-definitions.html#Reference.type the valueset binding for Reference.type is extensible.
Accordingly this the most close definition to FHIR is

interface Reference<ResourceType extends (FhirResourceType | string) = FhirResourceType> {
...
type: ResourceType;
...
};

let r1:Reference<"Patient">;
let r2:Reference<"AnotherValue">;

I don't this. | string usage makes Reference.type weak and it will no catch typos.
However, it is how FHIR defines Reference.type and we have to support all FHIR valid cases.

I would like to hear your thoughts.

@GinoCanessa @jdjkelly @bkaney @preston

@bkaney
Copy link
Contributor

bkaney commented Sep 13, 2021

Yes, this would be great @ir4y! I am looking forward to microsoft/TypeScript#41160, but wonder how we could do this to be somewhat backward compatiable?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request typescript Specific to export of Language TypeScript
Projects
None yet
Development

No branches or pull requests

5 participants