Skip to content

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

Merged
nicolo-ribaudo merged 2 commits intobabel:mainfrom
JLHwung:add-js-extension-to-polyfill-imports
Dec 10, 2020
Merged

Add js extension to preset-env generated polyfill imports#10862
nicolo-ribaudo merged 2 commits intobabel:mainfrom
JLHwung:add-js-extension-to-polyfill-imports

Conversation

@JLHwung
Copy link
Copy Markdown
Contributor

@JLHwung JLHwung commented Dec 11, 2019 •
edited
Loading

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

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

@nicolo-ribaudo nicolo-ribaudo Nov 20, 2020

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
Copy Markdown
Contributor Author

@JLHwung JLHwung Nov 20, 2020

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
Copy Markdown
Collaborator

babel-bot commented Nov 20, 2020 •
edited
Loading

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

@codesandbox-ci
Copy link
Copy Markdown

codesandbox-ci bot commented Nov 20, 2020 •
edited
Loading

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
Copy Markdown
Member

nicolo-ribaudo commented Dec 2, 2020 •
edited
Loading

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.

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

See babel/babel#10862
@haoqunjiang
Copy link
Copy Markdown
Contributor

haoqunjiang commented Dec 4, 2020

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

@existentialism
Copy link
Copy Markdown
Member

existentialism commented Dec 10, 2020 •
edited
Loading

@JLHwung mind fixing the conflicts?

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

@nicolo-ribaudo
Copy link
Copy Markdown
Member

nicolo-ribaudo commented Dec 10, 2020

@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
Copy Markdown
Member

nicolo-ribaudo commented Dec 10, 2020

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 @haoqunjiang @existentialism