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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Converted CommonJs to ESM #2

Merged
merged 6 commits into from
Oct 25, 2021

Conversation

sphinxc0re
Copy link
Contributor

@sphinxc0re sphinxc0re commented Oct 16, 2021

Okay, I don't expect you to merge this at any point in time since I didn't ask you whether this change was something you wanted in the first place. I totally get that. Also, this is something I threw together in an afternoon. Anyway, here is what I did:

  • Converted the whole codebase to ESM using the cjstoesm tool
  • Changed the minimum version of node to the current LTS, which is v14
  • Made the tests and coverage work
  • Ran prettier for a unified code standard
  • Ran standard JS formatter 馃槄

I guess, you can look at this PR as more of a suggestion, rather than an actual PR. It is immensely opinionated and as you can probably tell I'm using prettier, which this project did not adopt as far, as I can see.

closes #1

@TotalTechGeek
Copy link
Owner

Hey, thank you for the MR @sphinxc0re! I will try to review and merge this sometime this weekend.

On Prettier, my usage of standard is more based on tradition rather than a strong preference for it, I would be open to considering a switch to a more common style. I do have a slight preference for the --no-semi variant, but at the end of the day it's something my editor would fix on a ctrl-s, so it's really not a big deal.

Since Node v12 is still in Maintenance LTS, I would like to continue supporting it until it EOLs in April of 2022.

Aside from that, thank you a ton for your contribution!

@sphinxc0re
Copy link
Contributor Author

Wow! I didn't think this would be the reaction 馃槷 Okay, let's go through this step-by-step:

  • Standard JS is fine. I don't want to change too much. Switching this codebase to ESM is already a lot
  • I just realized that the conversion removed a whole bunch of whitespace. You might have to go through the whole codebase to tell me where to put them back 馃槗
  • I just tested the project using node 12. Works fine.

@sphinxc0re
Copy link
Contributor Author

@TotalTechGeek Is the coverage drop a problem? I didn't really add anything that is untested 馃

@TotalTechGeek TotalTechGeek merged commit e05d1d6 into TotalTechGeek:master Oct 25, 2021
@sphinxc0re sphinxc0re deleted the cjs-to-esm branch October 25, 2021 16:06
@sphinxc0re
Copy link
Contributor Author

@TotalTechGeek Nice! Thanks for letting me contribute! Would you mind publishing a new version to npm?

@TotalTechGeek
Copy link
Owner

I will soon!

Just running through some final steps (correcting some minor linting here & there with typescript, making some of the imports more consistent)

@TotalTechGeek
Copy link
Owner

@sphinxc0re Hi, 1.1.8 has been released.

The module should be able to support both CJS (via Rollup) & ESM environments, with ESM being the default.

@sphinxc0re
Copy link
Contributor Author

Woohoo! 馃槂

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.

Supply an ESM Version
2 participants