cornerstonejs / dicomParser

JavaScript parser for DICOM Part 10 data
MIT License
712 stars 228 forks source link

Improve type declaration #258

Open Ragnar-Oock opened 1 year ago

Ragnar-Oock commented 1 year ago

This PR is a follow-up to #257

Changes done in this PR :

Notes :

Ragnar-Oock commented 1 year ago

Come to think about it, the introduction of the Tag type might be a breaking change. Maybe should remove that.

yagni commented 1 year ago

@Ragnar-Oock Can I get push rights to your branch so we can collaborate on it?

yagni commented 1 year ago

Come to think about it, the introduction of the Tag type might be a breaking change. Maybe should remove that.

Can you give me an example of where it would break something?

Ragnar-Oock commented 1 year ago

If someone does something like :

const TAG_I_AM_INTERESTED_IN = 'x00200000'; //random tag, the actual value does not matter

const element = dataset.elements[TAG_I_AM_INTERESTED_IN];

They would get an error from TS saying that string can't be used to index type {[x: `x${string}`] : Element}. And it doesn't add much anyway, so if you want to keep it maybe add it to the list of stuff to do in v2. Plus, the fact we can't limit the length of the string or the chars inside to be an actual 4 bytes hexadecimal value makes it a bit pointless.

A typescript playground demonstrating the problem

Ragnar-Oock commented 1 year ago

I thought about it a bit more.

While having the Tag type as it is now is a breaking change, it could be mitigated by changing it from :

type Tag = `x${string}`;

to

type Tag = `x${string}` | string;

That way it's possible to easily distinguish in the API what expects/returns a tag and what expects/returns just a string. And it's possible to have a doc string on it directly for intellisence to show to people that are not savvy of how the lib or the DICOM spec function (newcomers to a project using the lib mostly)

Ragnar-Oock commented 1 year ago

I'm not sure why the tests are failing, it says it can't install the ChromeDriver. Could someone look into that?

yagni commented 1 year ago

Yeah, please rebase onto yesterday's master--Circle CI's config needed updating per some changes the Chrome team made that broke things.