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

Improve @babel/runtime esm stability #12883

Merged
merged 3 commits into from Feb 24, 2021

Conversation

nicolo-ribaudo
Copy link
Member

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

Q                       A
Fixed Issues? Maybe fixes #12861, but there are not enough info to tell it for sure. Also maybe fixes #12881 (comment)
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

We didn't get any bug report about this because technically it works, but this PR introduces two improvements that make @babel/runtime better when using ESM helpers with older tools like Webpack 4:

  • It removes the warning about useESModules. This option is still necessary if you want to force Webpack 4 to use esm helpers
  • It uses relative imports to load ESM dependencies of ESM helpers. Otherwise webpack 4 would resolve the CJS version, because it doesn't read export maps in package.json (we don't need to rewrite imports to cjs polyfills, since they are cjs anyway).

I also added a CI step to check that @babel/runtime works with different Node.js versions and bundlers.

The `useESModules` option is unnecessary only when the target bundlers
support exports conditions in `package.json`.

When they don't, it's not possible to force them to use the native ESM
files unless you use this option, that makes it work by using a different
non-overloaded entrypoint.
@nicolo-ribaudo nicolo-ribaudo added the PR: Internal 🏠 A type of pull request used for our changelog categories label Feb 23, 2021
@babel-bot
Copy link
Collaborator

babel-bot commented Feb 23, 2021

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 23, 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 03208b5:

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

@JLHwung
Copy link
Contributor

JLHwung commented Feb 23, 2021

If we don't get any emergent bug reports, can you add runtime consumer e2e test in this PR? So we are confident that it does not break some bundler/engine versions.

@nicolo-ribaudo
Copy link
Member Author

Yes

JLHwung
JLHwung previously approved these changes Feb 24, 2021
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.

LGTM with comments. Although we haven't received any bug reports on Rollup, should we add Rollup integration test?

@@ -0,0 +1,3 @@
import "./import-auto.mjs";
import "./import-esm.mjs";
import "./require-auto.cjs";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a case on ./require-esm.cjs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should it work or throw?

Copy link
Contributor

Choose a reason for hiding this comment

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

It should throw on Node. But other bundlers, especially legacy versions may have different behaviour.

"private": true,
"exports": "./index.js",
"devDependencies": {
"@babel/runtime": "workspace:*",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you duplicate cases to @babel/runtime-corejs3? We have extra core-js imports in that package.

@JLHwung JLHwung dismissed their stale review February 24, 2021 15:04

We still need absolute runtime tests

@nicolo-ribaudo
Copy link
Member Author

Do we also need to test other bundlers, or do we trust that modern bundlers will do the correct thing?

This makes sure that even if the target bundler doesn't support export
conditions in `package.json`, an ESM helper will only load other ESM
helpers.
@nicolo-ribaudo nicolo-ribaudo force-pushed the better-runtime branch 2 times, most recently from e7a61df to 238c29c Compare February 24, 2021 18:27
This was referenced Mar 8, 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 May 27, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: tests 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