search

PantelisGeorgiadis / dcmjs-dimse

DICOM DIMSE implementation for Node.js using the dcmjs library
MIT License
68 stars 13 forks source link

TypeScript typing updates #35

Closed richard-viney closed 1 year ago

richard-viney commented 1 year ago

Have run into a few issues with the provided typings:

  1. The toFile() function's callback argument should have an | null on its error parameter.
  2. The key, cert and ca members of the securityOptions parameter are too narrow, the full types supported by Node.js are here: https://nodejs.org/api/tls.html#tlscreatesecurecontextoptions. The main thing is that string and string arrays are allowed but aren't in the current typings. This change applies to all three of the places that securityOptions are specified. It may be possible to pull the types straight from Node.js, i.e. no need to duplicate them.
  3. If we could export a SecurityOptions type that would be a nice to have, and it would avoid the current duplication too.
  4. Exporting the Request and Response from the library, along with the types. would be useful as it would allow use of those types in user code.
PantelisGeorgiadis commented 1 year ago

Hi Richard! Thanks for reporting these issues! Will try to fix them ASAP.

PantelisGeorgiadis commented 1 year ago

Hello @richard-viney! Version 0.1.15 was just released, and it includes fixes for the typing issue you reported. More specifically: 1. The toFile() function's callback argument now has an | undefined type. A custom callback was implemented to abstract Node.js’s one, that uses undefined (instead of null), so the signature can be similar with the Dataset.fromFile function. 2-3. The key, cert and ca members of the securityOptions parameter have been updated to include string | Array<string> | Buffer | Array<Buffer> types. 4. Request and Response types are now exported.

richard-viney commented 1 year ago

Great, thanks! It's working well so far. One minor thing that would be handy is to expose Network.socket in the types. We use it to get information on the TLS certificate as well as monitor the amount of data transferred. Not sure if it's considered public API though?

Also FYI the change to use undefined instead of null in the callback for toFile() was technically a breaking change, so it could warrant a v0.2 instead of a v0.1.15, though I'm not sure how strictly semantic versioning is being adhered to on this project so obviously it's entirely your call.

PantelisGeorgiadis commented 1 year ago

Hi Richard! Glad to hear that the changes are working well for you! Regarding exposing the socket class, are you referring to SCP or SCU operations? For SCP, it is already exposed and it is passed in the constructor of the respected class. If you are referring to SCU, how you have in mind to export just the certificate information? Have you checked the Statistics class? Currently, it allows you to measure the total bytes sent/received fore both SCU/SCP operations.

The callback of toFile() prior to this version, had just the Error parameter. It was missing the null. So in this version, we went from just Error to Error | undefined (not from Error | null to Error | undefined). Is this a breaking change? If yes, I realize that it is good to have a typescript expert around!

richard-viney commented 1 year ago

I was referring to SCP, and yep it is passed in the constructor so can be intercepted there, but the Network.socket value isn't exposed in the types currently, i.e. this.socket will give the socket at runtime, but will fail at compile time. However, there is a workaround where you re-define the socket value in the subclass so the type system knows about it.

Will check out the Statistics class, sounds useful! @ericlin-nz

Regarding toFile()'s callback type, that's a good point that the null was actually missing, but yes it is technically still a breaking change because code that was previously type-correct now might not be. There are differing opinions on how breaking changes to typings should affect semantic versioning, it's a whole thing in itself. In practice here though, using this callback was broken without the null on it, so I suspect anyone using these types would have already added it themselves, so correcting the type is fine. Changing it to use undefined is definitely a non-backwards-compatible change to the JavaScript API though, independent of these types.

PantelisGeorgiadis commented 1 year ago

Thanks Richard! What I understand is that I need to be more careful with type definitions! Let's hear the community's feedback and if we need to do a respected change, we can advance to v0.2.X.