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

chore: Convert project to us ECMAScript modules #1905

Merged
merged 2 commits into from
Jan 3, 2024

Conversation

jdufresne
Copy link
Contributor

ECMAScript modules are the official standard format to package JavaScript code for reuse. Modules are defined using a variety of import and export statements. Node fully supports ECMAScript modules as they are currently specified and provides interoperability between them and its original module format, CommonJS.

More and more Node packages are switching from CommonJS to ESM, including SVGO dependencies such as csso.

Node documentation on ESM is available at:
https://nodejs.org/api/esm.html

@SethFalco
Copy link
Member

SethFalco commented Dec 26, 2023

Related: #1429

Thanks for dedicating the time to perform the migration. This is something we knew had to be done eventually and was even one of the goals for SVGO v3, but was never prioritized. I'd also been putting it off.

I'll actually put the performance/test work on-hold to review these incoming pull requests (I can still execute them locally anyway). We're starting to get a lot of pull requests piling up now, plus it'll be great to work with modules instead.

Reference: Progress on performance and expanded regression testing.

I would like to do one more release with CommonJS with the improvements we've been making recently, and some optimizations for existing plugins Kendell R has been working on.

We'll get this reviewed and merged after that, and prep for SVGO v4. 🎉

Meanwhile, I'd be happy to assist you with rebasing as we merge other pull requests. 👍🏽

@jdufresne
Copy link
Contributor Author

Sounds great. Thanks!

lib/style.js Outdated Show resolved Hide resolved
@SethFalco SethFalco force-pushed the type-module branch 3 times, most recently from 9c48964 to 03db121 Compare January 2, 2024 21:05
@SethFalco
Copy link
Member

SethFalco commented Jan 2, 2024

I've made the following changes:

  • Rebased with main.
  • Removed use strict from all files, as that is implicit in modules.
  • Updated our Rollup config to also package a CJS bundle to preserve CommonJS compatibility.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.

https://developer.mozilla.org/docs/Web/JavaScript/Reference/Strict_mode#strict_mode_for_modules

I've seen many package maintainers drop CJS support when they move to ESM, so I assumed that maintaining both would be a challenge. After looking into it, it's actually not a problem at all as we're a stateless package. It does come with the downside of increasing the package size, but that's worth it to support a still substantial portion of the Node.js ecosystem. 🤔

We'll get this reviewed and merged after that, and prep for SVGO v4. 🎉

For now, I'd like to support both CJS and ESM. With this in mind, it's probably better to save SVGO v4 for another time, as this isn't the breaking change I was expecting it to be. I'd like to continue resolving bugs in the current version.

Later this week I'll skim over the changes again, verify that it's working properly in both ESM, CJS, CLI, and browser, and potential add tests specific to the CJS module similar to what we have with the browser bundle. Then I'll be glad to merge this!

Please let me know if you think there are any issues with my changes.

@SethFalco SethFalco force-pushed the type-module branch 4 times, most recently from 7c04efb to 0346c7e Compare January 3, 2024 16:11
@SethFalco SethFalco merged commit 2442f74 into svg:main Jan 3, 2024
8 checks passed
package.json Show resolved Hide resolved
@jdufresne jdufresne deleted the type-module branch January 3, 2024 20:29
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