Add ".js" extension to injected polyfill imports by shimataro · Pull Request #10549 · babel/babel

shimataro · 2019-10-14T03:21:47Z

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.

hg-pyun

Is it mandatory?

shimataro · 2019-10-16T03:21:05Z

@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 · 2019-10-16T19:07:43Z

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 · 2019-10-17T03:17:45Z

@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 · 2019-10-19T00:46:09Z

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 · 2019-10-19T01:05:35Z

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

Note:

Many files are changed, but source files are only 2. The rest are test case.
- packages/babel-preset-env/src/utils.js
- packages/babel-plugin-transform-runtime/src/index.js
Since @babel/runtime/regenerator is a directory, import "@babel/runtime/regenerator" transforms into import "@babel/runtime/regenerator/index.js".

nicolo-ribaudo · 2019-10-19T10:37:00Z

@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

nicolo-ribaudo

Apart from a minor comment, this looks good 👍

nicolo-ribaudo · 2019-10-19T10:41:06Z

packages/babel-plugin-transform-runtime/src/index.js

          cached = t.cloneNode(cached);
        } else {
-          cached = addDefault(file.path, source, {
+          cached = addDefault(file.path, `${source}.js`, {


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.

@nicolo-ribaudo
Is it fine c3b1bf5?

JLHwung

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 · 2019-10-24T05:50:26Z

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).

jaydenseric · 2019-12-06T02:33:13Z

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

Note that the injected Babel helper imports still require file extensions to work in Node.js, see: babel/babel#10549 (comment)

JLHwung · 2019-12-06T14:04:08Z

Landed in v7.7.5.

rwjblue · 2019-12-06T20:54:47Z

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.

This reverts commit d3a37b5.

JLHwung · 2019-12-07T17:09:24Z

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 · 2019-12-07T21:37:11Z

Really sorry, I messed up...

…10835) This reverts commit d3a37b5.

nicolo-ribaudo · 2019-12-07T22:37:48Z

@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 · 2019-12-21T11:30:54Z

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

hg-pyun reviewed Oct 14, 2019

View reviewed changes

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

nicolo-ribaudo force-pushed the add-js-extension-to-polyfill branch from 50100e6 to 245676b Compare October 19, 2019 10:34

nicolo-ribaudo reviewed Oct 19, 2019

View reviewed changes

nicolo-ribaudo approved these changes Oct 19, 2019

View reviewed changes

JLHwung reviewed Oct 24, 2019

View reviewed changes

JLHwung approved these changes Nov 14, 2019

View reviewed changes

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

jaydenseric added a commit to jaydenseric/graphql-react that referenced this pull request Dec 6, 2019

Valid Node.js ESM deep imports.

8d204ac

Note that the injected Babel helper imports still require file extensions to work in Node.js, see: babel/babel#10549 (comment)

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 changed the title ~~Add ".js" extension to polyfill #10548~~ Add ".js" extension to injected polyfill imports Dec 6, 2019

shimataro added 3 commits December 6, 2019 13:25

Node modules compatibility: add ".js" extension to imported polyfills

8dced5e

Update fixtures

42e95c0

add extensions on calling addDefaultImport(), not in the function.

fdf5c74

nicolo-ribaudo force-pushed the add-js-extension-to-polyfill branch from 0ba6bfa to fdf5c74 Compare December 6, 2019 12:30

nicolo-ribaudo merged commit d3a37b5 into babel:master Dec 6, 2019

nicolo-ribaudo removed this from the v7.8.0 milestone Dec 6, 2019

rwjblue mentioned this pull request Dec 6, 2019

Unresolvable runtime helper modules after upgrading to v7.7.5 #10832

Closed

JLHwung mentioned this pull request Dec 6, 2019

Do not add .js extension to injected helper imports when module transform presents #10833

Closed

nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Dec 7, 2019

Revert "Add ".js" extension to injected polyfill imports (babel#10549)"

0ef94b6

This reverts commit d3a37b5.

JLHwung added a commit that referenced this pull request Dec 7, 2019

Revert "Add ".js" extension to injected polyfill imports (#10549)"

757d4a0

This reverts commit d3a37b5.

JLHwung mentioned this pull request Dec 7, 2019

Revert "Add ".js" extension to injected polyfill imports" #10835

Merged

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

Revert "Add ".js" extension to injected polyfill imports (#10549)" (#…

4436ffd

…10835) This reverts commit d3a37b5.

JLHwung mentioned this pull request Dec 11, 2019

Add js extension to preset-env generated polyfill imports #10862

Merged

snyk-bot mentioned this pull request Dec 19, 2019

[Snyk] Upgrade @babel/runtime from 7.7.4 to 7.7.6 jakeherp/todo-list#4

Open

snyk-bot mentioned this pull request Jan 12, 2020

[Snyk] Upgrade @babel/runtime from 7.4.4 to 7.7.7 mboocannon/msgraph-sdk-javascript#1

Open

snyk-bot mentioned this pull request Jan 20, 2020

[Snyk] Upgrade @babel/runtime from 7.4.4 to 7.8.0 mboocannon/msgraph-sdk-javascript#3

Open

snyk-bot mentioned this pull request Feb 5, 2020

[Snyk] Upgrade @babel/runtime from 7.4.4 to 7.8.3 mboocannon/msgraph-sdk-javascript#4

Open

This was referenced Feb 22, 2020

[Snyk] Upgrade @babel/runtime from 7.4.4 to 7.8.4 mboocannon/msgraph-sdk-javascript#5

Open

[Snyk] Upgrade @babel/runtime from 7.2.0 to 7.8.4 mieweb/sqladmin#5

Open

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade @babel/runtime from 7.6.3 to 7.8.4 godaddy/mssql-pool-party#16

Closed

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Mar 22, 2020

github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2020

Uh oh!

Conversation

shimataro commented Oct 14, 2019 • edited by nicolo-ribaudo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hg-pyun left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimataro commented Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolo-ribaudo commented Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shimataro commented Oct 17, 2019

Uh oh!

buildsize bot commented Oct 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shimataro commented Oct 19, 2019

Uh oh!

nicolo-ribaudo commented Oct 19, 2019

Uh oh!

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo Oct 19, 2019

Choose a reason for hiding this comment

Uh oh!

shimataro Oct 19, 2019

Choose a reason for hiding this comment

Uh oh!

JLHwung left a comment

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo commented Oct 24, 2019

Uh oh!

jaydenseric commented Dec 6, 2019

Uh oh!

JLHwung commented Dec 6, 2019

Uh oh!

rwjblue commented Dec 6, 2019

Uh oh!

JLHwung commented Dec 7, 2019

Uh oh!

shimataro commented Dec 7, 2019

Uh oh!

nicolo-ribaudo commented Dec 7, 2019

Uh oh!

shimataro commented Dec 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

shimataro commented Oct 14, 2019 •

edited by nicolo-ribaudo

Loading

hg-pyun left a comment •

edited

Loading

shimataro commented Oct 16, 2019 •

edited

Loading

nicolo-ribaudo commented Oct 16, 2019 •

edited

Loading

buildsize bot commented Oct 19, 2019 •

edited

Loading