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 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 left a comment

Is it mandatory?

@shimataro

This comment has been minimized.

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

This comment has been minimized.

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.

@shimataro

This comment has been minimized.

Copy link
Contributor Author

shimataro commented Oct 17, 2019

@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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

shimataro commented Oct 19, 2019

@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 nicolo-ribaudo force-pushed the shimataro:add-js-extension-to-polyfill branch from 50100e6 to 245676b Oct 19, 2019
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Oct 19, 2019

@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 left a comment

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`, {

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Oct 19, 2019

Member

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.

This comment has been minimized.

Copy link
@shimataro

shimataro Oct 19, 2019

Author Contributor

@nicolo-ribaudo
Is it fine c3b1bf5?

Copy link
Contributor

JLHwung left a comment

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

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Oct 24, 2019

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 this to the v7.8.0 milestone Nov 14, 2019
@jaydenseric

This comment has been minimized.

Copy link

jaydenseric commented Dec 6, 2019

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 changed the title Add ".js" extension to polyfill #10548 Add ".js" extension to injected polyfill imports Dec 6, 2019
@nicolo-ribaudo nicolo-ribaudo force-pushed the shimataro:add-js-extension-to-polyfill branch from 0ba6bfa to fdf5c74 Dec 6, 2019
@nicolo-ribaudo nicolo-ribaudo merged commit d3a37b5 into babel:master Dec 6, 2019
2 of 4 checks passed
2 of 4 checks passed
Travis CI - Pull Request Build Errored
Details
test262 Workflow: test262
Details
e2e Workflow: e2e
Details
test Workflow: test
Details
@nicolo-ribaudo nicolo-ribaudo removed this from the v7.8.0 milestone Dec 6, 2019
@JLHwung

This comment has been minimized.

Copy link
Contributor

JLHwung commented Dec 6, 2019

Landed in v7.7.5.

@rwjblue

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

shimataro commented Dec 7, 2019

Really sorry, I messed up...

nicolo-ribaudo added a commit that referenced this pull request Dec 7, 2019
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Dec 7, 2019

@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)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.