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

Build typescript .d.ts files #61

Merged
merged 2 commits into from
Dec 12, 2022
Merged

Build typescript .d.ts files #61

merged 2 commits into from
Dec 12, 2022

Conversation

PaulHax
Copy link
Contributor

@PaulHax PaulHax commented Nov 15, 2022

Adds a tsc step in the npm run build script.

api.js changes:

  • make most options to api.DICOMwebClient optional.
  • add progressCallback arg to retrieveSeries

api.js changes:
- make most options to api.DICOMwebClient optional.
- add progressCallback arg to retrieveSeries
@pieper
Copy link
Contributor

pieper commented Nov 17, 2022

Should these be two different topics?

@hackermd
Copy link
Member

@PaulHax thanks for your PR. Could you kindly provide some context for these changes and elaboration on why you think they would be needed?

@PaulHax
Copy link
Contributor Author

PaulHax commented Nov 17, 2022

Hi @hackermd

.d.ts files make this library easier to use with Typescript projects and enable auto-completion in some IDEs.

The Typescript build tool can use the extensive JS Docs comments to generate types. The couple changes the JS Docs I made bring the generated TS types closer to the JavaScript behavior.

@hackermd
Copy link
Member

Thanks @PaulHax, I see how that could be helpful. This appears to be related to #42. I wonder whether we should rather focus on getting the full types into the library?

@hackermd hackermd added the enhancement New feature or request label Nov 18, 2022
@hackermd hackermd self-assigned this Nov 18, 2022
@PaulHax
Copy link
Contributor Author

PaulHax commented Nov 18, 2022

Ah, some nice work in Slim with the essential Dataset, Study, MetadataElement, etc types. Copying the Slim types in and closing this PR sounds good.

@hackermd
Copy link
Member

Could you help us get #42 over the finish line? It appears you have experience in configuration a JavaScript library for TypeScript and I am wondering whether we may be able to reuse (parts of) the configuration file that's included in this PR.

@hackermd
Copy link
Member

I would also like to include your improvements/fixes of the documentation.

@PaulHax
Copy link
Contributor Author

PaulHax commented Nov 21, 2022

I would like to help with this.

Would be nice to keep using the JS Docs for the source maps, doc helper pop-ups, and future maintenance. I pushed up another commit that mixes in Slim's Metadata and and MetadataElement interfaces. It add some Typescript oriented lines to api.js

/**
 * @typedef { import("../types/types").InstanceMetadata } InstanceMetadata
 */

But the docs would get better with TS folks forced to maintain them?

@PaulHax
Copy link
Contributor Author

PaulHax commented Dec 5, 2022

Hi @hackermd

Have any thoughts on keeping the JS Docs the source of type truth?

@hackermd
Copy link
Member

hackermd commented Dec 5, 2022

Have any thoughts on keeping the JS Docs the source of type truth?

I have limited experience with mixing JavaScript and TypeScript. Based on my understanding, there are two options:

  1. Declare the types in a separate .d.ts file and omit them in the JSDoc docstrings
  2. Define the types in the JSDoc docstrings

The first option has the advantage that the type declarations can be specified in the package.json file via the types parameter: https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html

In addition, I tend argue that it would make the transition to TypeScript (.js -> .ts) easier, because we wouldn't want the type definitions in the JSDoc docstrings anymore once type hints are used throughout the code, would we?

What would be the downsides of that approach?

@PaulHax
Copy link
Contributor Author

PaulHax commented Dec 5, 2022

This PR adds a type field to the pacakge.json file pointing to the tsc built .d.ts file. It also mixes in Slim's Metadata type via a comment in api.js

@typedef { import("../types/types").InstanceMetadata } InstanceMetadata

Keeping types in the JSDocs makes the docs appear on hover and "go to definition" go to the JavaScript.
image

@hackermd hackermd linked an issue Dec 12, 2022 that may be closed by this pull request
@hackermd
Copy link
Member

Thanks @PaulHax! Looks mostly good to me. Unfortunately, our pipelines are currently broken (#65), so we'll have to ignore the failures.

Moving forward it would be great to make the documentation available online. Do you know how to generate the HTML files with JSDoc together with the typescript type declaration files?

@PaulHax
Copy link
Contributor Author

PaulHax commented Dec 12, 2022

Great 👍

Do you know how to generate the HTML files with JSDoc

That is something I have not done before.

@hackermd
Copy link
Member

Do you know how to generate the HTML files with JSDoc

That is something I have not done before.

Okay, we can figure that out later.

@hackermd hackermd merged commit 53d97ba into dcmjs-org:master Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typescript definitions
3 participants