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

Put back ESM helpers in a folder where we can use .js #12919

Merged
merged 2 commits into from Mar 1, 2021

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Feb 26, 2021

Q                       A
Fixed Issues? Closes #12914, closes #12902, also would have closed other already closed issues
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

CW: rant

When preparing #12632, I didn't initially realize that thanks to how package.json#exports works we can make Node.js and bundlers automatically choose between the ESM and CJS versions of @babel/runtime helpers even if they are in completely different places on the file system.

Chosing to use the .mjs file extension in #12632 was a 100% valid choice:

  1. All the packages on npm are published assuming that the environment uses Node.js's resolution algorithm. npm doesn't officially mean "Node package manager", but we all know that it does deep in our hearts. When explicitly using the extension, the target file is always correctly resolved. console.log(require.resolve("./demo.mjs")); correctly resolves the file not only in modern Node.js versions but also in ancient ones such as 0.8.28.
  2. All the main bundlers correctly load .mjs files as JavaScript (I think by default they load any file as JavaScript).

There were some bugs on our side (for example, _index.mjs and index.js don't have the exact same signature due to backward compat so using the "module" condition in package.json was an error), but they are all fixed now.

However, it seems like there are some other problems:

  1. Many people explicitly configure Webpack to load "any extension that is not .js/.ts/.css" as a string, breaking .mjs files. This is easily fixable on their side, but only when they know why it's broken
  2. Many pre-configured tools built around Webpack configure it to load "any extension that is not .js/.ts/.css" as a string, breaking .mjs files. This is a bug in those tools
  3. I have heard that some less known but still widely used bundlers have problems resolving .mjs file. I think the one used by ember-cli is one of them based on some bug reports we got, but I didn't try. @babel/runtime never officially cared about their custom resolution algorithms

TBH I think we would be 100% right if we decide to keep the .mjs extension: we are doing a favor to the ecosystem built around Node.js, either by making those tools fix their bugs or by making people switch to tools that don't have the mentioned problems.

However, I already spent too much time helping people whose built was broken by the @babel/runtime update because of the broken configs or tools they are using. I'm not the only one, also other people in the team spent time on that.

I completely understand them and I am not attacking any Babel user (either direct, or because their dependencies rely on @babel/runtime). If updating a dependency breaks my build, I would obviously first think that it's a problem in the dependency I updated. However, we are not a "free support for everyone" agency, and the best way to avoid doing that is to prevent their builds from crashing (even if because of non-Babel tools).

I didn't test this PR locally, but the best test we have for this is the CI job introduced in #12883. I think it will pass, but let's see.

PS (technical): if you are wondering why we can use the .js extension in the esm folder, it's because it contains a package.json file with "type": "module".

@nicolo-ribaudo nicolo-ribaudo added PR: Internal 🏠 A type of pull request used for our changelog categories i: discussion labels Feb 26, 2021
@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 26, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2e54f29:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@nicolo-ribaudo nicolo-ribaudo force-pushed the i'm-pissed-off branch 2 times, most recently from 43af301 to 807e59f Compare February 26, 2021 23:37
if (
major > 13 ||
(major === 12 && minor >= 17) ||
(major === 13 && minor >= 2)
Copy link
Member Author

Choose a reason for hiding this comment

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

ESM is not enabled by default on 13.0 and 13.1 (thanks @JLHwung for pointing it out)

@babel-bot
Copy link
Collaborator

babel-bot commented Feb 27, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/42998/

Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

I think we should revert this PR in Babel 8, as a breaking change for old tools.

@nicolo-ribaudo nicolo-ribaudo merged commit 9844eee into babel:main Mar 1, 2021
@nicolo-ribaudo nicolo-ribaudo deleted the i'm-pissed-off branch March 1, 2021 16:31
This was referenced Mar 17, 2021
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jun 1, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
i: discussion outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Internal 🏠 A type of pull request used for our changelog categories
Projects
None yet
4 participants