dcmjs-org / dcmjs

Javascript implementation of DICOM manipulation
https://dcmjs.netlify.com/
MIT License
291 stars 110 forks source link

Crash due to no TextEncoder on nodejs #319

Open pieper opened 1 year ago

pieper commented 1 year ago

Following https://github.com/dcmjs-org/dcmjs/commit/bfd6693e7491b17a2e615393eb5b80af3a78de9b nodejs now crashes with undefined use of TextEncoder. According to this post it is fixed with the node util package like this:

const util = require('util');
this.encoder = new util.TextEncoder("utf-8");

And indeed this works when I hack it into the build. It needs to be integrated so that the code will work on both node and browser out of the box.

vjau commented 1 year ago

What version of Node do you need to support ? After some searching, it seems all version of Node after v12 support TextEncoder on the global object (as in the browser), no need to import/require util.

vjau commented 1 year ago

Current Node LTS is v16, and v14 will just get security fixes until april of 2023, so it's probably pretty safe not to support v10 and v11, imho.

pieper commented 1 year ago

I just installed the version that comes with apt-get on ubuntu 20.04, so yes, it's old. We haven't had a formal policy but if we do want to set a limit that's fine with me. It should be documented (in the readme.md for now) and we should have a startup check with a warning that the version is not supported and may not work.

vjau commented 1 year ago

I'm not familiar with this project architecture but it seems it has a lot of tools and entry points. Where would be a good place to put this check ? Or should it be in a dependency that should be imported and run by every tool ?

pieper commented 1 year ago

I'm not sure how this is handled by other packages in npm, but to me it would make sense that if you try to require the package into a version of node that we know is not compatible we should raise an error right away. Maybe the best would be to do some research into conventions other packages use to manage what node versions they will or won't run on.

I know the default node version is old, but 20.04 is supported until 2030 so I still think it would be nice to include the util.TextEncoder / util.TextDecoder workarounds if they can be cleanly integrated.

vjau commented 1 year ago

Ubuntu 20.04 includes in his repositories Node v10 which end of life (no more security fixes) was 2016-10-31, six years ago. Most npm packages don't support it anymore and there are multiple warning not to use it in production. I am also on 20.04 with node v14 running (EOL in six months) and i already have problems running some tools with it. I will have to update it by any means to v16 before EOL since there is no way i'm running a server tool that is not minimally maintained anymore.

vjau commented 1 year ago

I have found this package https://github.com/parshap/check-node-version that could be run with the test script ?

vjau commented 1 year ago

It seems there is also a way to enforce it in the package.json, but wouldn't it cause problem to run it on the browser ? (or would it be ignored by the bundler ?) https://stackoverflow.com/questions/29349684/how-can-i-specify-the-required-node-js-version-in-package-json

pieper commented 1 year ago

Thanks for looking into this. I agree running end-of-life software for anything serious is not good practice. It's also good though for us to support older versions if it's not a big effort, just because there are times when valid uses get locked in a version dilemma where one dependency requires an upgrade but another doesn't support the new version yet. I wouldn't want to go overboard adding workarounds though and I do like the idea of being specific about the version we support, either through one of the mechanisms you mentioned or even just in the documentation.