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 ES Module #8

Merged
merged 5 commits into from
Sep 20, 2024
Merged

Convert to ES Module #8

merged 5 commits into from
Sep 20, 2024

Conversation

eliot-akira
Copy link
Contributor

@eliot-akira eliot-akira commented Sep 20, 2024

@nicolas-jaussaud Just wanted to let you know, I converted this library from CommonJS using require to ES Module format with import.

It was necessary to edit all JS files - the tricky part was replacing dynamic require's, which often needed an additional default property when imported.

I've tested all the available commands, so hopefully everything is still backward compatible with projects using this tool. If not, please feel free to open an issue.


This change will be helpful for keeping dependencies updated, which are all eventually moving to ES Module.

My short-term motivation was that the sass library started showing a deprecation notice about their legacy JS API, used by the Rollup plugin for styles (CSS/Sass). The legacy API will be removed in Dart Sass 2.0.

As part of solving it, I replaced the plugin with a newer, better maintained fork. But one of its dependencies only works with ES Module - so I rolled up my sleeves and converted the entire library to ESM which I've been meaning to do for a while.

@nicolas-jaussaud
Copy link
Contributor

Thank you for letting me know, I hope it wasn't too painful to convert everything!

I would have a question regarding the syntax inside the tangible.config.js file

My understanding is that we have to gradually migrate them from using CommonJS to ES modules (as you explained in the related issue), but that it is not directly related to the changes from this pull request, and that tangible.config.js files will still support both syntaxes with the last changes

Is it the case, or is the CommonJS syntax not supported anymore with 2.1.0?

@eliot-akira
Copy link
Contributor Author

eliot-akira commented Sep 24, 2024

About tangible.config.js, the Roller loads it using await import() which I believe supports both CommonJS and ES Module.

So it's up to the project when it's ready to switch, it can define "type": "module" in its package.json. And update tangible.config.js to use export default instead of module.exports.


A couple days ago I was struggling with a new Docusaurus setup, where I couldn't get its bundler Webpack to work with the project in ES Module, because of some dependencies. I had to switch the project back to CommonJS and rewrite some config files to change the syntax.

One day I hope the transition in the ecosystem will be complete so we can all use the new syntax in peace.

In a future version of Node.js, ES Module will be the default.

@eliot-akira
Copy link
Contributor Author

eliot-akira commented Sep 24, 2024

The Node.js documentation on await import() states:

import() expressions - Dynamic import() is supported in both CommonJS and ES modules. In CommonJS modules it can be used to load ES modules.

It doesn't say that ES modules (like the Roller now) can import CommonJS modules. I'm pretty sure I confirmed that it works though. I'll check real quick with an example project.

tangible.config.js

module.exports = {
  build: [
    { src: 'index.js', dest: 'index.min.js' }
  ]
}

package.json

{
  "type": "commonjs"
}

Install Roller and run:

$ bun add @tangible/roller

installed @tangible/roller@2.1.0 with binaries:
 - roll

$ bunx roll build

Build for production
..Building from index.js
Built index.min.js (225 B)

OK, so it works.

@nicolas-jaussaud
Copy link
Contributor

Thank you for your detailed answer and explanations (as always!)

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