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

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

Closed
wants to merge 6 commits into from

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Dec 6, 2019

Q                       A
Fixed Issues? Fixes #10832
Patch: Bug Fix? Yes
Tests Added + Pass? Yes
License MIT

As a follow up to #10549, this PR partially reverts the recently introduced behaviour on the helper imports. It will remove .js added in #10549 when other module transforms presents.

The .js extension is only required by native ES Modules and we can safely remove it when it will be transformed to other module formats.

Compatibility Note:

When using transform-runtime 7.7.6 with

  • modules-umd < 7.7.6
  • modules-amd < 7.5.0

it will still add the .js extension to imports due to lack of context variables. This will cause trouble to AMD users. If you are experience this issue, please upgrade these plugins to 7.7.6.

@JLHwung JLHwung added PR: Bug Fix 🐛 A type of pull request used for our changelog categories Priority: High labels Dec 6, 2019
"@babel/plugin-transform-modules-*",
);
// Remove the trailing `.js` requires by ES Module when module transforms presents
const validSource = moduleTransforms
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically only amd and umd are affected by this issue since by default RequireJS does not support resolving foo.js => foo.js as-is out of the box. You may configure to support this but we did cause breaking changes to these users.

Both CommonJS and SystemJS support foo.js => foo.js, but at the same time they also support foo => foo.js. Hence I remove .js for the sake of briefness. C.f. #10549 (review)

@@ -1,5 +1,6 @@
import foo from "foo";

class Example {
method() {}
property
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rewrite this test case since it looks more practical that an ESM supported environment does not support class-properties and so we need to transform.

@JLHwung JLHwung changed the title Do not add .js extension to injected helper imports when module transforms presents. Do not add .js extension to injected helper imports when module transform presents. Dec 6, 2019
@JLHwung JLHwung changed the title Do not add .js extension to injected helper imports when module transform presents. Do not add .js extension to injected helper imports when module transform presents Dec 6, 2019
@LarsDenBakker
Copy link

LarsDenBakker commented Dec 7, 2019

This should not apply to systemjs transform, or be configurable. Systemjs is an es module compliant polyfill. We use this in https://www.npmjs.com/package/es-dev-server where we use es modules in the browser during development and fallback to systemjs if modules are not supported.

@nicolo-ribaudo
Copy link
Member

@babel/core-team What do you think about reverting the PR which introduced the regression, releasing it before Monday and then re-introducing the .js suffix behind an option?

Systemjs is an es module compliant polyfill.

Note that the es modules specification doesn't say anything about file extension. The .js extension is never mentioned in the ECMAScript standard, and I think that it isn't mentioned by all the web platform stands neither (or at least, if it's meaningless).

@chriskrycho
Copy link

The .js extension is never mentioned in the ECMAScript standard, and I think that it isn't mentioned by all the web platform stands neither (or at least, if it's meaningless).

Indeed! More, the spec is intentionally silent on how a string is to be resolved. A given module loader (browser, Node, etc.) may choose to implement its own rules for the string parse. It is required to be a string literal, but that is the only thing specified about the module specifier.

@JLHwung
Copy link
Contributor Author

JLHwung commented Dec 7, 2019

@LarsDenBakker This PR restores to behaviour for SystemJS to pre 7.7.5 so it should work.

I discussed with @nicolo-ribaudo a bit and we are now reverting #10549 for 7.7.6. This PR is thus closed.

A new option removeImportsExtension: boolean will be introduced in 7.8.0. It defaults to true so the behaviour will be aligned to pre-7.7.5, which means we still generate @babel/runtime/foo imports. For those people publishing the es modules, they will have to set it false.

We are planing to change its default to false on 8.x, which is more meaningful to current ES Modules implementation while still providing backward supports to other module standards, i.e. amd.

@LarsDenBakker
Copy link

Makes sense guys, thanks!

@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 Mar 8, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unresolvable runtime helper modules after upgrading to v7.7.5
4 participants