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

build!: add support for ESM and drop support for CommonJS #75

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rolandjitsu
Copy link
Collaborator

What kind of change does this PR introduce?

  • Bug Fix
  • Feature
  • Refactoring
  • Style
  • Build
  • Chore
  • Documentation
  • CI

Did you add tests for your changes?

  • Yes, my code is well tested
  • Not relevant

If relevant, did you update the documentation?

  • Yes, I've updated the documentation
  • Not relevant

Summary

Fully migrated the package to ESM and dropped support for CommonJS.

Does this PR introduce a breaking change?

Yes. Any use of this package that does not support ESM will no longer work.

Other information

@edemaine
Copy link

Looks good to me! (having read but not tested the changes)

@rolandjitsu rolandjitsu mentioned this pull request Oct 10, 2024
12 tasks
Copy link
Collaborator

@rxmarbles rxmarbles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reasoning for ESM only? I'm trying to catch up but I'm on vacation at the moment. Can I get some context @rolandjitsu ?

@rolandjitsu
Copy link
Collaborator Author

What is the reasoning for ESM only? I'm trying to catch up but I'm on vacation at the moment. Can I get some context @rolandjitsu ?

Checkout #66 and the original #64 for some context.

I've initially tried to keep support for commonjs, but a bump of node was needed, then I got into some issues with the lock file, which needed an update as well. Then other deps caused issues, etc.

To me it kind of seems that moving to ESM is the right thing to do, despite the fact that the consequence of that means some users (not sure if there is a way to show a percentage) will need to move to ESM as well, which I think that's inevitable (given that even node 18 will reach EOL next year some time - https://nodejs.org/en/about/previous-releases).

This simplifies the package in terms of build (there's nothing to build anymore) and size (just a couple hundred bytes vs kb).

We can keep the v2 version as a maintenance branch (we can update the release workflow to release from a v2.2.x branch) and patch it when necessary (we've hardly had any updates within years), but it'll require some work to make it work with pinned down versions of packages, and v3 will be fully ESM.

Let me know if that makes sense.

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.

3 participants