cornerstonejs / dicomParser

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

why dicomParser does not have pako as dependency #270

Open sedghi opened 2 months ago

sedghi commented 2 months ago

I'm trying to build an ES module for dicom-image-loader and encountering several issues. It seems like dicom-parser attempts to check if pako is defined or not, and then decides whether to use it. My question is, why not simply depend on pako directly? Currently, dicom-parser has zero dependencies and only dev dependencies.

@yagni

yagni commented 2 months ago

This is a little before my time (way back in 2015), but my guess would be so that users who don't need inflation don't have to take the hit of the extra bytes from bundling pako. It's also not needed for running in node.js, which has the native zlib library, and there's no separate build pipeline at the moment for browser vs. node. @chafey would know for sure

chafey commented 2 months ago

@yagni is correct, paki is optional to avoid adding a dependency. Dependency management was much more difficult back then

On Thu, Jun 13, 2024 at 5:25 PM yagni @.***> wrote:

This is a little before my time (way back in 2015), but my guess would be so that users who don't need inflation don't have to take the hit of the extra bytes from bundling pako. It's also not needed for running in node.js, which has the native zlib library, and there's no separate build pipeline at the moment for browser vs. node. @chafey https://github.com/chafey would know for sure

— Reply to this email directly, view it on GitHub https://github.com/cornerstonejs/dicomParser/issues/270#issuecomment-2166893126, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJVXWVEUDTW5C5UWQHROXLZHIL6FAVCNFSM6AAAAABJJGKBZ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRWHA4TGMJSGY . You are receiving this because you were mentioned.Message ID: @.***>

sedghi commented 2 months ago

I understand, but I'm encountering issues with ESM for the DICOM image loader, and assuming a library is available on the window object is an anti-pattern, in my opinion. At the very least, it should be injected during initialization or a similar process.

chafey commented 2 months ago

Adding support for injecting it via an init function makes sense. Just don't make it mandatory as dicomParser is widely used as it is today

On Fri, Jun 14, 2024 at 8:00 AM Alireza @.***> wrote:

I understand, but I'm encountering issues with ESM for the DICOM image loader, and assuming a library is available on the window object is an anti-pattern, in my opinion. At the very least, it should be injected during initialization or a similar process.

— Reply to this email directly, view it on GitHub https://github.com/cornerstonejs/dicomParser/issues/270#issuecomment-2167987909, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJVXWRSI5IEE67KKW2MT6DZHLSQNAVCNFSM6AAAAABJJGKBZ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRXHE4DOOJQHE . You are receiving this because you were mentioned.Message ID: @.***>