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

Add ".js" extension to injected polyfill imports #10549

Merged
merged 3 commits into from Dec 6, 2019

Conversation

shimataro
Copy link
Contributor

@shimataro shimataro commented Oct 14, 2019

Q                       A
Fixed Issues? Fixes #10548, fixes #8462
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? No
Documentation PR Link
Any Dependency Changes? No
License MIT

input:

for (const x of [1, 2, 3]) {
  console.log(x);
}

output(before):

import "core-js/modules/es.array.iterator";

for (const x of [1, 2, 3]) {
  console.log(x);
}

output(after):

import "core-js/modules/es.array.iterator.js"; // <- extension added!

for (const x of [1, 2, 3]) {
  console.log(x);
}

As of Node.js v12, import syntax requires extension.
https://medium.com/@nodejs/announcing-a-new-experimental-modules-1be8d2d6c2ff

Please refer #10548 for more details.

Copy link

@hg-pyun hg-pyun left a comment

Choose a reason for hiding this comment

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

Is it mandatory?

@shimataro
Copy link
Contributor Author

shimataro commented Oct 16, 2019

@hg-pyun
Thank you for your comment.

Is it mandatory?

Yes, it seems there are no reason that omitting extensions is better.

But, considering compatibility, is it better to add flag (for example, polyfillExtension) in .babelrc?

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Oct 16, 2019

Node 13.0.0 will be released on October 22nd, and a copule of weeks later node 13.1.0 with unflagged modules support will be released.
https://twitter.com/jasnell/status/1184265267951493120?s=20

Given that there isn't a single reason for preferring it without the extension, I don't see why it should be introduced behind a flag.

Note that it also needs to be updated in @babel/plugin-transform-runtime.

@nicolo-ribaudo nicolo-ribaudo added area: modules area: node pkg: preset-env PR: New Feature 🚀 A type of pull request used for our changelog categories labels Oct 16, 2019
@shimataro
Copy link
Contributor Author

@nicolo-ribaudo
Thank you for your comment.

Note that it also needs to be updated in @babel/plugin-transform-runtime.

OK, I will do it later.

@buildsize
Copy link

buildsize bot commented Oct 19, 2019

File name Previous Size New Size Change
babel-preset-env.js 2.73 MB 2.73 MB 20 bytes (0%)
babel-preset-env.min.js 1.65 MB 1.65 MB 18 bytes (0%)
babel.js 2.92 MB 2.92 MB 75 bytes (0%)
babel.min.js 1.61 MB 1.61 MB 63 bytes (0%)
test262.tap 4.84 MB [deleted]

@shimataro
Copy link
Contributor Author

@nicolo-ribaudo @hg-pyun
I also updated @babel/plugin-transform-runtime. Could you please review?

Note:

  1. Many files are changed, but source files are only 2. The rest are test case.
  2. Since @babel/runtime/regenerator is a directory, import "@babel/runtime/regenerator" transforms into import "@babel/runtime/regenerator/index.js".

@nicolo-ribaudo
Copy link
Member

@shimataro I have reordered your commits to make it easier to review them. The first commit only contains the code changes, while the second one only the changes in the tests.

If you need to update your local branch, you should run

git fetch
git reset origin/add-js-extension-to-polyfill --hard

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Apart from a minor comment, this looks good 👍

@@ -254,7 +254,7 @@ export default declare((api, options, dirname) => {
if (cached) {
cached = t.cloneNode(cached);
} else {
cached = addDefault(file.path, source, {
cached = addDefault(file.path, `${source}.js`, {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I would prefer not to add the extension here and to add it where addDefaultImport, so that we are explicit about the files we are importing with assDefaultImport.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolo-ribaudo
Is it fine c3b1bf5?

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 am a little bit concerned about the output size increase for CommonJS program source (sourceType === "script") -- every injected runtime helper now costs 3 bytes more.

While import requires extension to be explicitly specified, could we only add *.js extension only when

path.find(p => p.isProgram()).node.sourceType === "module"

so that CommonJS source does not have to add extra .js.

Practically I don't think both babel runtime helpers and third party polyfill will have both foo and foo.js together, which is definitely a breaking change, so it is safely to remove *.js for CommonJS source type.

@nicolo-ribaudo
Copy link
Member

I don't think that it is a problem: in browsers those imports/requires don't work.
This code is targeting either node (for which a few more bites are way less important), or module bundles (which replace the import specifier with an id).

@JLHwung JLHwung added the PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release label Nov 14, 2019
@JLHwung JLHwung added this to the v7.8.0 milestone Nov 14, 2019
@jaydenseric
Copy link

Very keen for this; it's blocking me being able to support native ESM in a few packages.

jaydenseric added a commit to jaydenseric/graphql-react that referenced this pull request Dec 6, 2019
Note that the injected Babel helper imports still require file extensions to work in Node.js, see: babel/babel#10549 (comment)
@nicolo-ribaudo nicolo-ribaudo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories and removed PR: New Feature 🚀 A type of pull request used for our changelog categories labels Dec 6, 2019
@nicolo-ribaudo nicolo-ribaudo changed the title Add ".js" extension to polyfill #10548 Add ".js" extension to injected polyfill imports Dec 6, 2019
@nicolo-ribaudo nicolo-ribaudo merged commit d3a37b5 into babel:master Dec 6, 2019
@nicolo-ribaudo nicolo-ribaudo removed this from the v7.8.0 milestone Dec 6, 2019
@JLHwung
Copy link
Contributor

JLHwung commented Dec 6, 2019

Landed in v7.7.5.

@rwjblue
Copy link
Member

rwjblue commented Dec 6, 2019

FWIW, this has definitely broken users in the Ember ecosystem (using ember-cli-babel). There are many different bundling systems, and they do not all require that you include the file extension in the final module path. In fact, if you compile these @babel/runtime modules with Babel itself (via @babel/plugin-transform-modules-amd) the extension is not included in the defined module.

@JLHwung
Copy link
Contributor

JLHwung commented Dec 7, 2019

Updates: this PR will be reverted in 7.7.6. We will bring back the behaviour under a configurable option in 7.8.0. C.f. #10833 (comment)

@shimataro
Copy link
Contributor Author

Really sorry, I messed up...

@nicolo-ribaudo
Copy link
Member

@shimataro It's not a problem! We should have been more careful when reviewing the PR 😉

Would you like to reopen this PR, behind an option, as described in #10833 (comment)?

@shimataro
Copy link
Contributor Author

@nicolo-ribaudo
I'm back, sorry too late.
I saw #10833 (comment), #10853 and #10862.

Is it resolved by #10853 and #10862, and no longer needed removeImportsExtension?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: modules area: node outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: preset-env PR: Bug Fix 🐛 A type of pull request used for our changelog categories PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release
Projects
None yet
6 participants