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

Convert to ESM #28

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Convert to ESM #28

wants to merge 1 commit into from

Conversation

nickdnk
Copy link

@nickdnk nickdnk commented Mar 5, 2024

Following my other PR (#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.

@nickdnk
Copy link
Author

nickdnk commented Mar 5, 2024

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

EnriqCG commented Mar 6, 2024

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

nickdnk commented Mar 7, 2024

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 (nestjs/nest#11046 (comment)) 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.

@nickdnk nickdnk changed the title Convert to ES6 module Convert to ESM Mar 7, 2024
export RCONOptions interface
change default encoding to utf8
Adjust readme
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants