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

Structurally refactor TypeScript output similar to C# output for direct implementation #72

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

Comments

@preston
Copy link

preston commented Jul 23, 2021

(Let me know if you'd prefer I split this issue up more!)

We've been doing some prototypes based on the TypeScript R4 .ts file, and have run into a few challenges I think could benefit from some general refactoring.

First is the lack of a .d.ts file. We tried simply renaming the .ts, but it doesn't work since .d.ts declarations can't have initializers. (Might be other issues.) Second is general performance. The file is > 400,000 lines long, causing editors to choke. Even when cherrypicking specific imports, VS Code, at least, seems to not handle the single file approach well. Third is the general approach to generics covered in #70 that makes it very challenging to implement concrete class definitions for FHIR profiles imposing type and cardinality constraints.

If the direction is to also provide a set of class definitions like the C# version -- which we would love and appreciate! -- I would suggest breaking up the output file substantially, implement some uses of generics (at least for frequently used types), and to add a corresponding concrete class hierarchy with essentially the same deep copy and serialization-type functions as C# outputs.

All this would really make it directly usable for SMART and general FHIR developers. At the moment we can't really base our class definitions off of it because we're trying to enforce profile constraints at a more granular, static level.

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

Synchronizing up with the Zulip discussion (around here).

People are generally supportive of the idea of splitting the definitions, but we need to ensure that the usability story doesn't change (e.g., a single include to get the FHIR namespace).

For creating the .d.ts file, @bkaney has been doing the transformation from this project's output to what gets checked into DefinitelyTyped (I believe via this project: fhir-dts-generator). It should be possible to change the output of the code generator to export what is needed directly, but there haven't been requests or PRs for doing so.

I am happy to support the creation of a more generic SDK for TS/JS (as exist in other languages), but I do not have the time to spearhead such an effort right now.

@jdjkelly
Copy link
Contributor

A few things strike me:

  1. Yeah, the fact that the project is separate from the DefinitelyTyped package is somewhat confusing but looks like there's a not trivial amount here: https://github.com/Vermonster/fhir-dts-generator/blob/main/gulpfile.ts - greater alignment makes sense tho for the sake of that project too right

  2. On the TS side, this project is right now a good schelling point around this:

breaking up the output file substantially, implement some uses of generics (at least for frequently used types)


Curious to hear more about these cases tho - @preston maybe we can sync up?

At the moment we can't really base our class definitions off of it because we're trying to enforce profile constraints at a more granular, static level.

makes it very challenging to implement concrete class definitions for FHIR profiles imposing type and cardinality constraints.

@bkaney
Copy link
Contributor

bkaney commented Jul 30, 2021

As to the output, I assume the output would be for fhir-codegen (and not DT)? Or is the ask to change the output for DT as well?

  1. Yeah, the fact that the project is separate from the DefinitelyTyped package is somewhat confusing but looks like there's a not trivial amount here: https://github.com/Vermonster/fhir-dts-generator/blob/main/gulpfile.ts - greater alignment makes sense tho for the sake of that project too right

Yea, to build for DT, I needed to structure things to be compatible with the conventions at that project, and create tests (which I download the FHIR spec and use the examples). I didn't want to do it by hand and wanted it to be repeatable, hence that project.

@preston
Copy link
Author

preston commented Aug 6, 2021

@jdjkelly Yes, let's chat. I have a call with a few of the MS folks this afternoon as well to share context.

@GinoCanessa
Copy link
Contributor

I've been working through some of this (in the context of updates to the subscriptions reference implementation). Moving the main output to .d.ts feels good, but we lose a lot as well. For example, since we cannot create instances of Coding, objects, we cannot create typed ValueSets for use. The same holds true for enums - in a .d.ts, they have to be declared const, which changes what you can use them for (e.g., iterating over at runtime).

I am leaning towards separating those out at least, so we have the option of creating the .d.ts for the interfaces/types and .ts files for all the 'extras'. The evaluation of this would also change if the goal is a full SDK/library, as we would probably need to create classes instead of just types anyway.

I also spent some time investigating splitting up the file structure... and the story for working through the dependency graph isn't great. I needed to move on, so I haven't come to a resolution on that topic yet.

For visibility, the branch I've been tinkering in is typescript.

@preston
Copy link
Author

preston commented Aug 6, 2021

Thanks @GinoCanessa , we're much looking forward to talking through the development use cases this afternoon.

Is the ultimate objective on whether this will be a a full SDK up in the air? I see arguments both ways, though if it remains just interfaces it would be nice to avoid enforcing any particular class hierarchy since the developer will probably have to extend other base types for serialization etc. This is one of our issues.. if we require a separate library for all the OAuth and FHIR REST calls, we still need the information model classes to click into it without a bunch of annoying model conversions between the canonical types and those the FHIR client understands.

@GinoCanessa
Copy link
Contributor

Original request has been completed, as well as the start of the TypeScript SDK project (fhir-typescript).

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

4 participants