Open nickdnk opened 8 months ago
nickdnk
commented
8 months ago Also:
Set default encoding to utf8 as it increases compatibility out of the box.
Exported the RCONOptions interface as well, so it can be imported if used anywhere else in a TS application.
Removed the npm run version command as it calls npm run format which is not defined.
EnriqCG
commented
8 months ago Hey, thanks for taking the time to investigate and creating a PR.
I am unsure if it needs to remain a commonJS module for some reason, but in that case I think it should be discussed how compatibility can be improved, which evidently is required based on the bug above.
I'd like for this lib to continue having support for CommonJS. I'm all for ESM but I know rcon-srcds is being used with big frameworks that don't support ESM (I'm looking at you, NestJS). And I agree the current setup can, and should be improved. I know much more about how Node modules work today than I did when I started this project.
For this, moving to a build tool like esbuild can be interesting, as it can produce outputs in both CJS and ESM.
I've set the requirement at Node 14, which I think is reasonable I bumped to version 3.0.
I agree.
nickdnk
commented
8 months ago I'll leave that up to you. You could also consider just telling anyone requiring commonJS to stay on v2.x, as that still works fine, aside from the "not a constructor" issue which was why I ended up here in the first place.
Looking at nest.js specifically, it seems they're being absolutely hammered with issues about ESM not working, so that's more likely a nest.js issue and not something you should be particularly worried about going forward, at least not if v2 still works for that. Looks like there's also a workaround (https://github.com/nestjs/nest/issues/11046#issuecomment-1416983059) for that problem.
Anyway, I now have a branch of this package for myself which I can use in my project (just a github: dependency) which solves my problem, so I don't care too much about the outcome.
Edit: You could even add this package in the thread above to add to the push for ESM support. Edit 2: Okay never mind, it's locked.
Following my other PR (https://github.com/EnriqCG/rcon-srcds/pull/27) I took a look at converting the package to ESM instead of odd hotfixes.
I am unsure if it needs to remain a commonJS module for some reason, but in that case I think it should be discussed how compatibility can be improved, which evidently is required based on the bug above.
If you are not interested in making these changes, or you don't care about the package, please let me know and I can publish an ESM-version as a separate package.
I've set the requirement at Node 14, which I think is reasonable, and I also updated all dependencies. I've verified that the package works in an ESM project using the syntax provided in the readme, which I also updated.
For this PR, I bumped to version 3.0.