Converted CommonJs to ESM

TotalTechGeek / json-logic-engine

Construct complex rules with JSON & process them.

MIT License

43 stars 9 forks source link

Converted CommonJs to ESM #2

Closed sphinxc0re closed 2 years ago

sphinxc0re commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

sphinxc0re commented 2 years ago

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

TotalTechGeek commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

Woohoo! 😃