Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crash due to no TextEncoder on nodejs #319

Open
pieper opened this issue Oct 5, 2022 · 9 comments · May be fixed by #321
Open

Crash due to no TextEncoder on nodejs #319

pieper opened this issue Oct 5, 2022 · 9 comments · May be fixed by #321

Comments

@pieper
Copy link
Collaborator

pieper commented Oct 5, 2022

Following bfd6693 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
Copy link

vjau commented Oct 6, 2022

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
Copy link

vjau commented Oct 6, 2022

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
Copy link
Collaborator Author

pieper commented Oct 6, 2022

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
Copy link

vjau commented Oct 7, 2022

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
Copy link
Collaborator Author

pieper commented Oct 7, 2022

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
Copy link

vjau commented Oct 7, 2022

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
Copy link

vjau commented Oct 7, 2022

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

@vjau
Copy link

vjau commented Oct 7, 2022

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
Copy link
Collaborator Author

pieper commented Oct 7, 2022

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.

@vjau vjau linked a pull request Oct 9, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants