Upgrade dcmjs dependency to 0.24.6

richard-viney commented 2 years ago

Hey Pantelis,

Similar to #19, could we upgrade dcmjs to "^0.24.6" to get the latest fixes in a dcmjs-dimse release?

Also, this is probably a separate issue, but is it intentional that the release bundles for this library actually include a copy of the dcmjs library? This has the effect of users not being able to use dcmjs patch fixes without a release of this library, and also means that things like Yarn's selective dependency resolution feature can't be used. Can open a separate issue for this if you'd like?

Thanks!

PantelisGeorgiadis commented 2 years ago

Hi Richard!

Starting with your question, whether including a copy of dcmjs in the bundle is intentional, the answer is no. Seems to be the default webpack behavior. However, it is in my TODO list to research on how this can be avoided (declare dcmjs as “external” in the webpack config?). Let’s open another issue for that!

I tried to upgrade to dcmjs v0.24.6 and noticed a warning and a console message while running the tests.

Console message says “DicomMessage.read to be deprecated after dcmjs 0.24.x”. dcmjs-dimse is using DicomMessage.read to parse datasets without file meta information (actually, everything that comes from the network). Unfortunately, the message doesn’t propose a new method to use for the same purpose. Is this functionality going to be completely removed (@pieper)? A strange observation is that although the library contains a logging mechanism, this message is getting written directly to console (console.warn). I believe we should make this clear before updating.
Warning (more like an error?) says “WARN: TypeError: coding.replaceAll is not a function”. replaceAll was introduced in Node.JS v15 and I’m using v14.16.1. I can surely upgrade my Node.JS but I’m not sure that it was in the dcmjs authors’ intention to break v14 compatibility for such a simple function. Should we wait for their hotfix (@pieper, @florianlink)?

Thoughts?

pieper commented 2 years ago

Thanks for raising these issues.

The discussion about the read API is here: https://github.com/dcmjs-org/dcmjs/pull/283. I guess we were wrong in thinking it was only an internal method? @Ouwen

Regarding replaceAll I wasn't aware of that and probably yes, we can make it compatible with nodejs 14, which I understand is still in maintenance mode (https://nodejs.org/en/about/releases/).

I hope these are easy fixes.

florianlink commented 2 years ago

You can replace .replaceAll with .replace(/regexp/g, “replacement”), note the g on the regexp.

On Sun 24. Jul 2022 at 18:36, Steve Pieper @.***> wrote:

Thanks for raising these issues.

The discussion about the read API is here: dcmjs-org/dcmjs#283 https://github.com/dcmjs-org/dcmjs/pull/283. I guess we were wrong in thinking it was only an internal method? @Ouwen https://github.com/Ouwen

Regarding replaceAll I wasn't aware of that and probably yes, we can make it compatible with nodejs 14, which I understand is still in maintenance mode (https://nodejs.org/en/about/releases/).

I hope these are easy fixes.

— Reply to this email directly, view it on GitHub https://github.com/PantelisGeorgiadis/dcmjs-dimse/issues/24#issuecomment-1193353097, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABKHPHCRO2PZCDZG73WRBJDVVVWIFANCNFSM54JBCCAQ . You are receiving this because you were mentioned.Message ID: @.***>

florianlink commented 2 years ago

If the tests still work, your fix will be correct 😀

On Sun 24. Jul 2022 at 19:55, Florian Link @.***> wrote:

You can replace .replaceAll with .replace(/regexp/g, “replacement”), note the g on the regexp.

On Sun 24. Jul 2022 at 18:36, Steve Pieper @.***> wrote:

Thanks for raising these issues.

The discussion about the read API is here: dcmjs-org/dcmjs#283 https://github.com/dcmjs-org/dcmjs/pull/283. I guess we were wrong in thinking it was only an internal method? @Ouwen https://github.com/Ouwen

Regarding replaceAll I wasn't aware of that and probably yes, we can make it compatible with nodejs 14, which I understand is still in maintenance mode (https://nodejs.org/en/about/releases/).

I hope these are easy fixes.

— Reply to this email directly, view it on GitHub https://github.com/PantelisGeorgiadis/dcmjs-dimse/issues/24#issuecomment-1193353097, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABKHPHCRO2PZCDZG73WRBJDVVVWIFANCNFSM54JBCCAQ . You are receiving this because you were mentioned.Message ID: @.***>

Ouwen commented 2 years ago

@pieper, yes good that you had the foresight to add the warn. I think it is worth a discussion if read should be internal or external. @PantelisGeorgiadis, if the function turns internal libraries would need to call DicomMessage._read instead of DicomMessage.read

richard-viney commented 2 years ago

I've opened a PR for avoiding replaceAll:

https://github.com/dcmjs-org/dcmjs/pull/296

richard-viney commented 2 years ago

For the change to the DicomMessage.readTag() signature, I think it's useful for those functions to be public (with the revised signatures). This breaking public API change could be part of dcmjs 0.25.x ?

PantelisGeorgiadis commented 2 years ago

Hi @richard-viney! I've updated to dcmjs 0.24.7 and replaced the call fromDicomMessage.read to DicomMessage._read. Additionally, I've added all dependencies to webpack's external list and the bundle size was decreased from 1.31 MB to 79.3 kB, while things seems to be still working!

richard-viney commented 2 years ago

Awesome. thank you, seems to be working well so far!

florianlink commented 2 years ago

Hi,

yeah, replacing replaceAll sounds good, can you provide a fix? I am on holidays right now, only got my mobile phone 😂

On Sun 24. Jul 2022 at 17:23, Pantelis Georgiadis @.***> wrote:

Hi Richard!

Starting with your question, whether including a copy of dcmjs in the bundle is intentional, the answer is no. Seems to be the default webpack behavior. However, it is in my TODO list to research on how this can be avoided (declare dcmjs as “external” in the webpack config?). Let’s open another issue for that!

I tried to upgrade to dcmjs v0.24.6 and noticed a warning and a console message while running the tests.

Console message says “DicomMessage.read to be deprecated after dcmjs 0.24.x”. dcmjs-dimse is using DicomMessage.read to parse datasets without file meta information (actually, everything that comes from the network). Unfortunately, the message doesn’t propose a new method to use for the same purpose. Is this functionality going to be completely removed ( @pieper https://github.com/pieper)? A strange observation is that although the library contains a logging mechanism, this message is getting written directly to console (console.warn). I believe we should make this clear before updating.

Warning (more like an error?) says “WARN: TypeError: coding.replaceAll is not a function”. replaceAll was introduced in Node.JS v15 and I’m using v14.16.1. I can surely upgrade my Node.JS but I’m not sure that it was in the dcmjs authors’ intention to break v14 compatibility for such a simple function. Should we wait for their hotfix @.*** https://github.com/pieper, @florianlink https://github.com/florianlink)?

Thoughts?

— Reply to this email directly, view it on GitHub https://github.com/PantelisGeorgiadis/dcmjs-dimse/issues/24#issuecomment-1193339089, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABKHPHCUVOP27Q3FI5JWTD3VVVNWXANCNFSM54JBCCAQ . You are receiving this because you were mentioned.Message ID: @.***>

PantelisGeorgiadis commented 2 years ago

The fix has already been applied! Enjoy your vacation @florianlink!

PantelisGeorgiadis / dcmjs-dimse

Upgrade dcmjs dependency to 0.24.6 #24