Explore elm types #267
Explore elm types #267
Conversation
There was a problem hiding this comment.
Thanks for the pull request, @csenn! Happy to see ELM TypeScript types being explored and implemented. It's something that's been on my radar for a long time, but I've never put the effort in, so I'm very glad that you have :).
Also, thanks for adding some better non-ELM-related types to the repo! This was our vision, to start with a bunch of anys or non-specific types for the initial conversion, and slowly improve it as we go.
I've left a bunch of inline comments. Some are just for discussion, and some have direct action. Most probably require @cmoesel's thoughts too.
| @@ -381,7 +382,7 @@ export class Power extends Expression { | |||
| } | |||
|
|
|||
| export class MinValue extends Expression { | |||
| static readonly MIN_VALUES = { | |||
| static MIN_VALUES = { | |||
There was a problem hiding this comment.
Is there a reason to take out the readonly qualifier here? The value of MIN_VALUES never gets re-assigned. I saw that MAX_VALUES below still has readonly, so figured I'd ask about the change
There was a problem hiding this comment.
@csenn - I'm taking a look at this PR again, as it would be great to get it resolved. I noticed that you said you added the readonly back on line 385, but when I look at the current code on this branch, I don't see it. Perhaps you have not pushed it back up?
| super(json); | ||
| this.valueType = json.valueType; | ||
| if (json.valueType in MinValue.MIN_VALUES) { | ||
| this.valueType = json.valueType as keyof typeof MinValue.MIN_VALUES; |
There was a problem hiding this comment.
Small (opinionated) comment. Now that we use keyof typeof MinValue.MIN_VALUES in more than one place, maybe we can bring it out to its own type outside of the class? E.g.:
type MinValueType = keyof typeof MinValue.MIN_VALUES;Then we can just re-use this type where applicable. Same applies to max values below
There was a problem hiding this comment.
It also does make sense to pull MIN_VALUE out, and maybe even use an ENUM instead of a dict. But it was a little weird to even have to use as keyof typeof MinValue.MIN_VALUES... I thought Typescript would be able to infer that since json.valueType in MinValue.MIN_VALUES must be true
| if (json.valueType in MinValue.MIN_VALUES) { | ||
| this.valueType = json.valueType as keyof typeof MinValue.MIN_VALUES; | ||
| } else { | ||
| throw new Error(`Not expecting MIN_VALUE: ${json.valueType})`); |
There was a problem hiding this comment.
This error check breaks the arithmetic tests, but for a very logical reason lol. There is a test case that asserts that any incorrect MinValue provided should throw an error during execution. The generated ELM for this test case is here.
Since the constructor now throws an error if provided a valueType that we don't recognize.
I'm going to defer to @cmoesel here on how exactly to proceed. I think I might lean towards not throwing an error in a constructor, but I see the merit in the code that you've added.
There was a problem hiding this comment.
Yeah def agree with you, both ways have merit. Here is a StackOverflow discussing. I semi-lean towards validation in the constructor as long as that runs over the whole ELM tree as soon as the ELM wrapper is instantiated, just so that it doesn't become possible for execution to 'skip' certain parts of the ELM tree depending on the input patient data (only exec MinValue node if Patient has Condition Y) which could then allow bugs to escape tests more easily. Not sure if that's applicable though?
Either way, MIN_VALUES could be an ENUM but SHOULD NOT be added to the ELM generated from the XML because the mappings to MathUtil.MIN_INT_VALUE seem to be numbers that are only specific to the JS implementation.
There was a problem hiding this comment.
I can see both arguments -- including the desire to fail immediately upon loading the ELM (rather than on execution, just in case execution never occurs). That said the spec does say this:
For any other type, attempting to invoke minimum results in an error.
One could argue that "attempting to invoke" implies an error on execution, not on loading. (As far as I can tell, this is how the Java cql-engine implementation behaves as well).
| if (json.valueType in MaxValue.MAX_VALUES) { | ||
| this.valueType = json.valueType as keyof typeof MaxValue.MAX_VALUES; | ||
| } else { | ||
| throw new Error(`Not expecting Max_VALUE: ${json.valueType})`); |
There was a problem hiding this comment.
Same deal here as my comment about errors for MinValue.
|
|
||
| export class ValueSetDef extends Expression { | ||
| name: string; | ||
| id: string; | ||
| version?: string; | ||
|
|
||
| constructor(json: any) { | ||
| constructor(json: ELM.ValueSetDef) { | ||
| // @ts-ignore |
There was a problem hiding this comment.
Looks like this ts-ignore is because of issues with the ELM types? Is this a known issue? If so, we should maybe add a comment explaining the presence of this, and what the discrepancy of the ELM types are. To make sure eslint doesn't fail, you should also disable the lint rule above this line:
| // @ts-ignore | |
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | |
| // @ts-ignore |
There was a problem hiding this comment.
So this is a tricky one. @cmoesel also including you here. The issue is that the Def classes ValueSetDef, ConceptDef, CodeSystemDef, etc. DO NOT inherit from Expression in the XML schema data. Yet the class in the TypeScript does.
<xs:complexType name="ValueSetDef">
<xs:complexContent>
<xs:extension base="Element">
<xs:sequence>
<xs:element name="codeSystem" type="CodeSystemRef" minOccurs="0" maxOccurs="unbounded">
</xs:element>
</xs:sequence>
<xs:attribute name="name" type="xs:string"/>
<xs:attribute name="id" type="xs:string" use="required">
</xs:attribute>
<xs:attribute name="version" type="xs:string" use="optional">
</xs:attribute>
<xs:attribute name="accessLevel" type="AccessModifier" use="optional" default="Public"/>
</xs:extension>
</xs:complexContent>
</xs:complexType>
export class ValueSetDef extends Expression {
name: string;
id: string;
version?: string;
constructor(json: ELM.ValueSetDef) {
// @ts-ignore
super(json);
this.name = json.name || '';
this.id = json.id;
this.version = json.version;
}
etc...
}
Notice the base='Element'. In most other "expressions", the XML looks has base='Expression':
<xs:complexType name="MinValue">
<xs:complexContent>
<xs:extension base="Expression">
<xs:attribute name="valueType" type="xs:QName" use="required"/>
</xs:extension>
</xs:complexContent>
</xs:complexType>
To further complicate, the generated ELM for the Def expressions do not carry a type property that allows the differentiation of an expression type.
// No Type
interface ValueSetDef extends AbstractElement {
name?: string;
id: string;
version?: string;
accessLevel?: AccessModifier;
codeSystem?: CodeSystemRef[];
}
// Has Type
interface MinValue extends AbstractExpression {
type: "MinValue";
valueType: string;
}
Which is necessary to differentiate the different types of Expressions currently being evaluated using
type Expression =
| OperatorExpression
| ExpressionRef
| FunctionRef
| ParameterRef
| + a bunch more
So to summarize, the problem could possibly be solved by simply adding 'base=Expression' to the def in the XML, but
- Not sure the Core Java Code wants us to keep changing the XML spec for this library (maybe through an MR)? There would then likely be changes to the generated ELM (adding of the type key for expression differentiation) which could potentially break things. Also that may not make logical sense anyways, because a
ValueSetDefcan not exist anywhere in the Expression tree, only at the root ELM.Library layer. - Could change this code a bit on the JS side, ensuring that these
Defclasses do not inherit from Expression. But of course we would need to think through that a bit more than just adding// @ts-ignoreas I did here.
There was a problem hiding this comment.
I'm not sure if I followed this conversation correctly, but...
I don't think we want to force a change on the ELM. The ELM is part of the formal spec, so that would have to be voted on by HL7 (if considered a "technical correction") or balloted (if not considered a "technical correction"). I'm not sure we have a strong enough justification for that.
I think a long term solution could be to rely a bit more on interfaces than parent classes. Perhaps rather than extending Expression, this class should implement Executable (an interface w/ an exec() function). And then wherever exec gets called, modify that code to expect an Executable rather than an Expression. As @csenn suggested, however, I think we'd need to think that through a bit more. So for now, I think I'd stick w/ the ts-ignore and maybe add a TODO to consider refactoring.
There was a problem hiding this comment.
Hey @cmoesel, interfaces would definitely be an interesting approach to explore. It makes sense that switching to TypeScript might lean the library towards certain design patterns that might have been handled a little differently in vanilla JavaScript.
Sorry I haven't made any more progress on this MR, I've actually been working on another related project that you may find interesting though! It's a browser based CQL IDE and publisher tool that provides support for real-time validation of CQL, CQL statement type checking, dependency validation and resolution, and other tooling that supports writing CQL. It uses cql-exec under the hood for CQL execution.
If you're interested, I'd love to share the progress and maybe get some feedback from you and @mgramigna in a week or two and see what you guys think about the approach?
P.S. - It also may be worth looking into the type support for the cql execution results. One thing I'm seeing with fhir-exec is the results are not in FHIR format. For instance, a codeable_concept.code has a .value property, which in FHIR is just a string. It can be a little confusing on what exactly the output you're dealing with is.
There was a problem hiding this comment.
Hi @csenn -- yes, your side-project does sound interesting! Will it be available via an open source license? If so, cool, I'd love to see it. If it won't be available via open source, I need to determine if it's appropriate for us to engage at that level (just because of our position in this space and engagement w/ other companies).
As for the results, it might be hard to type those since CQL allows you to return arbitrary Tuples. I suppose we could type the FHIR result types and authors can cast to them if they're sure of what it is. The reason for the variation from FHIR is that the result types align with the CQL FHIR model info (which expresses FHIR primitives as complex types with a value) rather than the FHIR JSON serialization. I've thought in the past that it might be nice to add a toFHIRJSON function on the types to allow for conversion.
There was a problem hiding this comment.
Ah interesting, that makes sense that it's the model info. A toFHIRJSON could likely be useful though... Maybe we can discuss how that could be best achieved and I can help look into it.
I'm actually not 100% sure where the project I'm working on is going, more just trying to figure what makes sense right now. It will certainly be free, but probably a combination of open and closed source since there will need to be login, data persistence, CI pipeline, etc. If you do have a chance to take a quick gander once it's ready, you'll probably pretty immediately see why the output type matters. Since data IN is FHIR, it's a little hard to intuit that OUT comes a different data structure especially for common scenarios such as filtering Conditions by a ValueSet. But yeah like you mentioned, when shaping and returning Tuples you're actually not working with FHIR anymore, so it's tricky... I suppose the same also holds when returning primitives such as booleans or numbers.
There was a problem hiding this comment.
Hey @cmoesel! Here is an initial release of the project I was telling you about: https://www.cqlab.io/. It uses cql-exec quite a bit, and in the learn docs (https://www.cqlab.io/learn) you may see some examples inspired by the test cases used in cql-exec (that's actually what I used to learn CQL). There's also a tutorial learning guide in the docs that teaches you how to use the editors.
This is a preview-release (servers cold start after inactivity so some requests are slow on warm up) and with your experience I'd be ultra-grateful for any feedback, positive or negative. I mainly focused on FHIR4.1 here, but would be interested in your thoughts on other data models. This is probably not the right medium for further discussion, but I saw we connected on LinkedIn so feel free to message me there if you have any thoughts :)
There was a problem hiding this comment.
On re-review (in an effort to button this PR up), I think our guidance is to update this code to doc why we have to @ts-ignore it and to add the eslint rule as Matt suggested. This is a potential point for improvement in the future, but let's just leave it at that for now.
| this.systemName = json.system.name; | ||
| this.version = json.version; | ||
| this.systemName = json.system?.name || ''; | ||
| // @ts-ignore |
There was a problem hiding this comment.
Or maybe we should just tell eslint to allow @ts-ignore? If we know we need to use it in a bunch of places...
| super(json); | ||
| this.name = json.name; | ||
| this.name = json.name || ''; |
There was a problem hiding this comment.
ugh you know the drill at this point. My fault again, property can be optional. It's almost like I should've read the dang manual when doing this conversion. Sorry
There was a problem hiding this comment.
I personally don't hate defaulting to empty strings because usually you just do a check like
if (json.name) {
// Do Something
}
and null or empty string behave the same way. It also makes the code and types simpler sometimes without having to always check for null in cases where you want to do something like
json.name.toLowerCase()
But the broader discussion goes back to whether there are cases where we'd actually want to update the XML spec itself and change certain properties to required or not. There are pros and cons to having the single source of truth for the ELM spec. But it could get messy if we were to say manually start editing the generated ELM Typescript Types, because then we would not be able to generate it again later when the XML schema changes.
There was a problem hiding this comment.
At this point, unless there is a strong reason for it, I'd shy away from introducing changes to the ELM specification. Those changes are expensive given the governance process on CQL. In cases where something isn't clearly wrong, it may be better just to accommodate for it.
| const codes = this.codes.map((code: any) => this.toCode(ctx, code)); | ||
| return new dt.Concept(codes, this.display); | ||
| } | ||
| } | ||
|
|
||
| export class CalculateAge extends Expression { | ||
| precision: string; | ||
| precision: ELM.DateTimePrecision; |
There was a problem hiding this comment.
Oooo love that we get the specific literals now rather than having to use general string. Yay!
There was a problem hiding this comment.
Yeah this is nice. In theory we could also make this an ENUM instead of
type DateTimePrecision =
| 'Year'
| 'Month'
| 'Week'
| 'Day'
| 'Hour'
| 'Minute'
| 'Second'
| 'Millisecond';
but both probably are fine.
| if (!json.precision) { | ||
| throw new Error('Precision is required'); | ||
| } |
There was a problem hiding this comment.
Hmm.. I wonder how to handle this case. It's marked as optional in the type defs, but we throw an error when it isn't provided. Based on the logical specification, looks like precision should be 0..1. Ugh. This might snowball into types being sad in other functions, and I've now realized is yet again my fault. Let's discuss with @cmoesel what to do with this one. Upon further inspection, I think this error can be removed, precision can be marked as optional, and we can just make sure the durationBetween function call is happy when precision is undefined?
And fine I'll come clean now. Yes, I just very recently realized the utility of the logical specification...
There was a problem hiding this comment.
OK. So here's the thing:
- The logical spec (and XSD) do allow precision to be missing.
- Neither say how you should calculate the age when it's missing. Which is quite unfortunate.
- BUT... I don't think there is any way in the CQL language to create this type without a precision, because the language exposes it via specific unit-based functions (
CalculateAgeInYears,CalculateAgeInMonths, etc); there is noCalculateAgewithout a built-in precision unit. - So in some ways, this conversation is moot, because, in theory, precision will never actually be undefined.
What does that mean for us here? I guess I don't know -- but probably it means we should just do whatever is easiest. ;-)
There was a problem hiding this comment.
I think this goes back to the previous comment about possibly updating the XML spec to ensure that precision is always there if it should always be there... Interested to hear both of your thoughts on this one. At worse we could always just create some special ELM types locally that act as overrides, something like interface CqlExecCalculateAge extends ELM.Expression but that will get annoying in more than just a few case
There was a problem hiding this comment.
I think it makes sense to file an issue against CQL asking that this property either be made required, or that the behavior is defined for when a precision is not provided.
| if (!json.precision) { | ||
| throw new Error('Precision is required'); | ||
| } |
There was a problem hiding this comment.
Same as above comment on precision optional
|
Sorry I let this linger for so long... I've responded in some of the conversations with my thoughts. Interested to hear yours, |
|
@csenn - I've given this another round of review. I think there are a few small tweaks that would be good to make, but overall everything looks to be in good shape. The question on whether or not to throw in the constructor is still unresolved. I think it's probably OK to keep it as you have it in this PR, but I've also asked a colleague for additional input before making the call. So... I think this branch needs to be brought up to date w/ master and a few very minor things (mainly doc/ts-ignore) need to be tweaked. I realize that this PR has been open for some time now, so if you (@csenn) don't have time to revisit it, I could potentially make the changes myself if that is OK w/ you. Let me know how you want to proceed. |
|
Agreed with @cmoesel on the constructor throwing. I think it's okay as is. One thing I wanted to ask about was the intention of these ELM types going forward. I think we're happy to include the |
|
Hey @cmoesel and @mgramigna, sorry this fell on the wayside. I used the ELM types generated in my own projects, and it's been very helpful for my needs, but the question is are the types good enough for DefinitleyTyped. Here is the repo I used to generate the types: https://github.com/cqlabio/cql-elm-types. It converts the XSD into Typescript types. Unfortunately, initially i tried to keep it simple and just use libraries that converted XSD into json_schema and json_schema into typescript types, however the types did not look good. So I ended up just writing some code that converts it manually, but it's not super well tested. So either A) we can try and validate and possibly improve what I built or B) we can just create a new de-facto repo with the index.d.ts file and basically hand code changes for now on. What do you guys think? Does ELM change enough that automated would be better? Is that XSD reliable enough or does hand crafting have some advantages (such as marking something required that is missing in the XSD)? In terms of this PR, it would probably be better to just restart it since it only took 0.5 hours - 1 hour if I remember right. Is there a plan for any large architecture changes to what's currently on master/main? |
|
@csenn -- I think it makes sense to target DefinitelyTyped. I wouldn't worry too much about some types not being 100% perfect due to idiosyncrasies, etc; it's easy enough to fix them if we find anything like that. As for automating type generation vs hand-maintaining from here forward... I feel like ELM is pretty stable at this point. If getting the automation solid requires a lot of hacks/workarounds or is just generally difficult, I feel like hand-maintaining the types should be quite feasible. It also allows for some one-offs and judgement calls that might not be possible in automation. All that said, before we decide how to move forward, I think it would be good to bring @brynrhodes into this conversation. Have you talked to him about this at all? @bkaney is also a big typescript fan who does some work in CQL, so he might have some feedback and tips as well. |
This Pull Request starts the addition of ELM Typescript Types that can be used to make the ELM traversal logic a little more type safe.
Pull requests into cql-execution require the following.
Submitter and reviewer should ✔ when done.
For items that are not-applicable, mark "N/A" and ✔.
CDS Connect and Bonnie are the main users of this repository.
It is strongly recommended to include a person from each of those projects as a reviewer.
Submitter:
npm run test:plusto run tests, lint, and prettier)cql4browsers.jsbuilt withnpm run build:browserifyif source changed.Reviewer:
Name: