dcmjs icon indicating copy to clipboard operation
dcmjs copied to clipboard

Crash due to no TextEncoder on nodejs

Open pieper opened this issue 3 years ago • 9 comments
trafficstars

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.

pieper avatar Oct 05 '22 20:10 pieper

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 avatar Oct 06 '22 20:10 vjau

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.

vjau avatar Oct 06 '22 20:10 vjau

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.

pieper avatar Oct 06 '22 21:10 pieper

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 ?

vjau avatar Oct 07 '22 20:10 vjau

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.

pieper avatar Oct 07 '22 20:10 pieper

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 avatar Oct 07 '22 21:10 vjau

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

vjau avatar Oct 07 '22 21:10 vjau

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

vjau avatar Oct 07 '22 21:10 vjau

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.

pieper avatar Oct 07 '22 22:10 pieper