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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from Dec 10, 2020
Merged

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

merged 2 commits into from Dec 10, 2020

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Dec 11, 2019 •
edited

Q                       A
Fixed Issues? preset-env generated esm polyfill imports will throw on Node.js 13, fixes #12378
Patch: Bug Fix? Yes
Tests Added + Pass? Yes
License MIT

In this PR we add .js extension to the injected polyfill imports by @babel/preset-env. The new import specifier path/to/polyfill.js could cause trouble for AMD module compatibility (see #10832 ) but practically it is okay in this scenario because both our upstream polyfills: core-js and regenerator do not support amd.

Note that this PR is different than #10549 which handles transform-runtime instead.

/cc @rwjblue

@JLHwung JLHwung added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: preset-env labels Dec 11, 2019
@JLHwung JLHwung mentioned this pull request Dec 11, 2019
Merged
@shimataro shimataro mentioned this pull request Dec 21, 2019
Merged
nicolo-ribaudo added a commit to babel/babel-polyfills that referenced this pull request Mar 20, 2020
@nicolo-ribaudo
Add explicit .js extensions
8ab748b 
babel/babel#10862
nicolo-ribaudo added a commit to babel/babel-polyfills that referenced this pull request Apr 17, 2020
@nicolo-ribaudo
Add explicit .js extensions
9b22f4b 
babel/babel#10862
nicolo-ribaudo added a commit to babel/babel-polyfills that referenced this pull request Apr 17, 2020
@nicolo-ribaudo
Add explicit .js extensions
538b413 
babel/babel#10862
@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Nov 20, 2020
Closed
1 task
nicolo-ribaudo
nicolo-ribaudo reviewed Nov 20, 2020
View reviewed changes
packages/babel-preset-env/src/utils.js
@@ -152,11 +152,11 @@ export function isPolyfillSource(source: ?string): boolean {
}

const modulePathMap = {
"regenerator-runtime": "regenerator-runtime/runtime",
"regenerator-runtime": "regenerator-runtime/runtime.js",
Copy link
Member

Choose a reason for hiding this comment

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

regenerator-runtime works in browsers, so it currently works with AMD I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

regenerator-runtime/runtime.js does not use define to declare its name and dependencies, if any. So I guess it is likely that no one consume it as an AMD module.

@JLHwung JLHwung changed the base branch from master to main November 20, 2020 15:14
@JLHwung JLHwung force-pushed the add-js-extension-to-polyfill-imports branch from 2515fce to 4f7bb76 Compare November 20, 2020 15:16
@babel-bot
Copy link
Collaborator

babel-bot commented Nov 20, 2020 •
edited

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 20, 2020 •
edited

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 f8afd66:

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

@JLHwung JLHwung mentioned this pull request Dec 1, 2020
Closed
@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Dec 2, 2020 •
edited

You need to patch the vue e2e test similarly to what we did in e2e-create-react-app.sh, since this PR is making those tests failing.

@sodatea This PR will make vue-cli tests fail (https://app.circleci.com/pipelines/github/babel/babel/4604/workflows/bae839be-16f9-4ea1-975b-2477fb57eaff/jobs/33616/parallel-runs/0/steps/0-103), since they test that the code produced by Babel contains an exact substring changed by this PR.
Note that this isn't a breaking change, since we never give any guarantee about the exact output.

sodatea added a commit to sodatea/vue-cli that referenced this pull request Dec 3, 2020
@sodatea
test: should not expect babel polyfill paths to be extension-less
9b5a282 
See babel/babel#10862
@sodatea sodatea mentioned this pull request Dec 3, 2020
Merged
9 tasks
sodatea added a commit to vuejs/vue-cli that referenced this pull request Dec 4, 2020
@sodatea
test: should not expect babel polyfill paths to be extension-less (#6106
17339d7 
)

See babel/babel#10862
@sodatea
Copy link
Contributor

sodatea commented Dec 4, 2020

@nicolo-ribaudo Thanks for the caution! I've fixed the tests.

@existentialism
Copy link
Member

existentialism commented Dec 10, 2020 •
edited

@JLHwung mind fixing the conflicts?

EDIT: @nicolo-ribaudo i dunno what you're talking about ;)

@nicolo-ribaudo
Copy link
Member

@existentialism I think you pinged the wrong person 😛

Btw, fixing the conflicts probably also fixes the e2e tests, since they are failing exactly in those two files.

@JLHwung JLHwung force-pushed the add-js-extension-to-polyfill-imports branch from 4f7bb76 to f8afd66 Compare December 10, 2020 16:51
@nicolo-ribaudo
Copy link
Member

Spammish, but tomorrow it's the birthday of this PR 🥳

@nicolo-ribaudo nicolo-ribaudo merged commit 1382437 into babel:main Dec 10, 2020
@nicolo-ribaudo nicolo-ribaudo deleted the add-js-extension-to-polyfill-imports branch December 10, 2020 19:16
@ertrzyiks ertrzyiks mentioned this pull request Dec 22, 2020
Open
@ertrzyiks ertrzyiks mentioned this pull request Dec 23, 2020
Open
@nickvergessen nickvergessen mentioned this pull request Jan 7, 2021
Closed
@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 12, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@existentialism existentialism existentialism approved these changes

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

Assignees
No one assigned
Labels
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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

core-js 3 missing .js extension when 'type': 'module'
5 participants
@JLHwung @babel-bot @nicolo-ribaudo @sodatea @existentialism