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

Initial typescript setup #260

Merged
merged 42 commits into from Feb 21, 2022

Conversation

mgramigna
Copy link
Contributor

@mgramigna mgramigna commented Jan 24, 2022

Fixes #236

Summary/Motivation

This PR does the initial conversion of the library to TypeScript, without too many strict types and no runtime changes. This will significantly lower the burden of TypeScript developers wishing to use cql-execution, as they no longer have to define custom .d.ts files to make the TypeScript compiler happy. All types are now exported from cql-execution itself when publishing.

Basically all of these changes will not be seen by any non-TypeScript projects consuming the library, as most of the changes are development-focused, with a few new dependencies, and using tsc over babel

Code Changes

Core Development Changes

  • Added files and dependencies necessary for TypeScript compilation with basic configuration options
    • tsconfig.test.json is an extension of tsconfig.json that allows the test to disable certain TypeScript rules on a case-by-case basis
  • Replaced babel with tsc and updated necessary build scripts
    • Remove babel.config.json
  • Added TypeScript eslint plugin and use recommended rules
  • Added .mocharc.json to support TypeScript tests

Source Code Changes

  • Renamed all core files to .ts
  • Update all module.exports to use export or export default where appropriate
  • Added type declarations to all files, mostly using any for now, aside from existing classes/simple data types
    • Stricter typing will be added incrementally
    • Added type casting where necessary to avoid changing any source code (type guards could be a nice improvement here down the line)
  • Reworked some JavaScript to adhere to TypeScript practices
    • Moved any static variables of classes to inside the class declaration
    • Fix some simple assignments of class variables to handle the proper types
  • Added .d.ts file for ucum-lhc library with basic type definitions needed for cql-execution
  • Update all tests to be TypeScript
    • Any auto-generated .js files from the test case generator remained untouched and as .js
    • Fixed issue in convert-test where improper bracket matching led to an improperly labeled test
  • Removed some unnecessary assertions from tests
    • Certain tests were asserting things that the TypeScript compiler deemed impossible: datetime-test.ts was asserting that properties on a Date class should not exist where TypeScript would have never let them exist in the first place (co-signed change by @cmoesel )

Misc Changes

  • Added TypeScript example code to examples directory (I am not attached to this, will be happy to remove)
  • Added myself to the contributors list in package.json. I thought this PR warranted it ;)

Testing Guidance

The functionality of the library should be unchanged. Run all tests and usual checks

  • To see how the types behave within a TypeScript project, link this library and import the exported classes with import * as cql from 'cql-execution'. You should be able to inspect the .d.ts for what cql is, and easily write code that deals with any of the normally-exported classes from cql-execution
  • To test the built version of the code, use the example in examples/node, or use npm link in an existing JavaScript project
  • To test the browserified code, open examples/browsers/cql4browsers.html and ensure the results for any ELM are the same as the current master branch

Open Questions

  • The main concern I have with this conversion is whether or not I properly marked optional parameters as optional, specifically ones that are exported by the library as part of the core API. The unit tests and examples covered most of the cases I found, but I still think a thorough review should be done on each exported class to make sure the attributes are declared properly
  • I left a few @ts-ignore comments in here to revisit later. Some of them may be trivial to fix if the reviewer has any suggestions. Some of them might be a bit more involved though. I'm happy to expand on any of the decisions made there
  • Should I update the README to describe this as a TypeScript library now? I can at the very least include examples in the README for what developing with cql-execution in TypeScript looks like
  • Since this doesn't break the API at all, is this change appropriate for only a minor version bump? I didn't change the version in package.json yet. Will probably do so when the PR is in a stabler state

Advice

Have fun!!!!

--

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:

  • This pull request describes why these changes were made
  • Code diff has been done and been reviewed (it does not contain: additional white space, not applicable code changes, debug statements, etc.)
  • Tests are included and test edge cases
  • Tests have been run locally and pass
  • Code coverage has not gone down and all code touched or added is covered.
  • Code passes lint and prettier (hint: use yarn run test:plus to run tests, lint, and prettier)
  • [ N/A] All dependent libraries are appropriately updated or have a corresponding PR related to this change
  • cql4browsers.js built with yarn run build:browserify if source changed.

Reviewer:

Name: Chris Hossenlopp (@hossenlopp)

  • Code is maintainable and reusable, reuses existing code and infrastructure where appropriate, and accomplishes the task’s purpose
  • The tests appropriately test the new code, including edge cases
  • You have tried to break the code

package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@hossenlopp hossenlopp left a comment

Choose a reason for hiding this comment

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

A few comments and questions. Also currently breaks in fqm-execution, but we are actively resolving that issue on slack.

src/datatypes/clinical.ts Outdated Show resolved Hide resolved
src/datatypes/clinical.ts Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
src/elm/expressions.ts Show resolved Hide resolved
.github/workflows/ci-workflow.yml Show resolved Hide resolved
src/runtime/executor.ts Outdated Show resolved Hide resolved
test/datatypes/datetime-test.ts Show resolved Hide resolved
test/elm/string/string-test.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
mgramigna added 2 commits February 3, 2022 10:46
* Fix args that should have been optional
* Add stricter typing of execution parameters
* Switch to prepare over prepublish
* Export types directory from cql.ts
@codecov-commenter
Copy link

codecov-commenter commented Feb 3, 2022

Codecov Report

Merging #260 (544f7dc) into master (b0f5307) will decrease coverage by 4.88%.
The diff coverage is 95.51%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #260      +/-   ##
==========================================
- Coverage   92.43%   87.54%   -4.89%     
==========================================
  Files          49       50       +1     
  Lines        4082     5035     +953     
  Branches        0     1415    +1415     
==========================================
+ Hits         3773     4408     +635     
- Misses        309      310       +1     
- Partials        0      317     +317     
Impacted Files Coverage Δ
src/util/math.ts 65.38% <71.73%> (ø)
src/elm/comparison.ts 76.47% <76.47%> (ø)
src/elm/expression.ts 80.64% <84.61%> (ø)
src/datatypes/ratio.ts 86.36% <85.71%> (ø)
src/datatypes/datetime.ts 90.43% <89.80%> (ø)
src/cql-code-service.ts 86.95% <90.00%> (ø)
src/datatypes/clinical.ts 93.33% <90.90%> (ø)
src/util/util.ts 77.50% <92.30%> (ø)
src/cql-patient.ts 82.89% <94.11%> (ø)
src/elm/reusable.ts 74.19% <94.11%> (ø)
... and 79 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b0f5307...544f7dc. Read the comment docs.

Copy link
Contributor

@hossenlopp hossenlopp left a comment

Choose a reason for hiding this comment

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

This is the beginning of a bright future for cql-execution!

Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

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

Wowsers. That's a pretty significant PR. It is also a pretty awesome PR. I'm stoked to see TypeScript support coming into cql-execution. (I probably haven't used the word "stoked" for at least 15 years, but here we are. Huh.)

I've made a bunch of individual comments, some of which are actionable, some of which are not actionable, and some of which are actionable but also ignorable. I leave it as an exercise for the reader to figure out which is which. ;-)

One comment not in the list of individual comments: I noticed that .github/pull_request_template.md still references yarn. That file isn't in the PR, so this is the only place I can comment on it.

Thanks again for this excellent work. I'm sure it was very tedious and annoying at times. I hope you will receive your reward in the afterlife (if you believe in such a thing). Please feel free to reach out if you have any questions (about my review, that is; I'm afraid I have no further details about your reward in the afterlife).

examples/browser/README.md Show resolved Hide resolved
examples/node/exec-age.js Show resolved Hide resolved
examples/typescript/exec-age.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/util/math.ts Show resolved Hide resolved
src/util/units.ts Outdated Show resolved Hide resolved
test/cql-exports-test.ts Show resolved Hide resolved
test/datatypes/interval-test.ts Show resolved Hide resolved
test/elm/arithmetic/arithmetic-test.ts Show resolved Hide resolved
@mgramigna mgramigna changed the title Intial typescript setup Initial typescript setup Feb 15, 2022
mgramigna and others added 6 commits February 15, 2022 09:50
* Fix easy type updates
* Properly mark optional params

Co-authored-by: Chris Moesel <cmoesel@users.noreply.github.com>
* Update PR template to use npm
* Remove prepend flag from npm scripts
* Properly mark patient attribute as potentially undefined
* Rename types files
* Add strict typing to executor and context args
* Update example to use typescript import with comment
Copy link
Contributor

@hossenlopp hossenlopp left a comment

Choose a reason for hiding this comment

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

Another comment on TS interfaces.

src/types/cql-patient.interfaces.ts Show resolved Hide resolved
Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

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

This is looking pretty awesome. Thanks for addressing my comments so quickly! I've commented back on a few of the existing comment threads and just added a few more additional comments in this review.

Thanks again for all the hard work and attention to detail!

src/cql-code-service.ts Outdated Show resolved Hide resolved
src/runtime/context.ts Outdated Show resolved Hide resolved
src/types/cql-code-service.interfaces.ts Outdated Show resolved Hide resolved
src/types/cql-patient.interfaces.ts Show resolved Hide resolved
@mgramigna
Copy link
Contributor Author

@cmoesel I got a second wind and attempted a first pass at an abstract base class for Date and DateTime in a221bd6

I kept some TODOs in there, as I think we can take this further (e.g. improving how we access the static FIELDS attribute, potentially doing DateTime extends Date if possible, maybe moving out some of these helper functions to their own file)

src/types/cql-patient.interfaces.ts Show resolved Hide resolved
src/types/cql-patient.interfaces.ts Outdated Show resolved Hide resolved
src/datatypes/datetime.ts Show resolved Hide resolved
src/runtime/context.ts Outdated Show resolved Hide resolved
* Require _is and _typeHierarchy
* Add interfaces for TypeSpecifier structures
Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

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

This all looks really great, @mgramigna. Since a lot has changed since I first looked at this, I think I will need to run it through some regressions again w/ my other projects that use cql-execution.

cmoesel and others added 4 commits February 18, 2022 16:18
* Add information about TypeScript conversion
* Update examples to include TypeScript and proper JavaScript
* Update CI bade to GH Actions over travis
* Update command to generate cql4browsers
Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

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

@mgramigna - This is the start of something beautiful. Thank you so much for your very significant contribution. The future is bright (and typed)!

@cmoesel cmoesel merged commit 4e1c56a into cqframework:master Feb 21, 2022
@mgramigna mgramigna deleted the intial-typescript-setup branch February 21, 2022 18:36
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.

Port to TypeScript?
4 participants