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

Use the babel-plugin-polyfill-* packages in transform-runtime #12845

Merged

Conversation

nicolo-ribaudo
Copy link
Member

Q                       A
Fixed Issues? Ref #11823
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

This is the same as #12583, but for @babel/runtime 🙂

@nicolo-ribaudo nicolo-ribaudo added the PR: Internal 🏠 A type of pull request used for our changelog categories label Feb 22, 2021
// This file isn't maintaned anymore.
// Improvements should go in the babel-plugin-polyfill-corejs2 package.

export default {
Copy link
Member Author

Choose a reason for hiding this comment

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

These two files have just been moved from src to scripts. GitHub shows them as new because I changed the indentation.

}
if (!useRuntimeHelpers) return;

file.set("helperGenerator", name => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This code has only been de-indented. You can review with https://github.com/babel/babel/pull/12845/files?w=1 to disable whitespace changes.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 22, 2021

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

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

@babel-bot
Copy link
Collaborator

babel-bot commented Feb 22, 2021

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

Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

LGTM overall except for comments.


var _mapInstanceProperty = require("<CWD>/packages/babel-runtime-corejs3/core-js/instance/map");
var _mapInstanceProperty = require("@babel/runtime-corejs3/core-js/instance/map.js");
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not generate package/path/polyfill.js when absoluteRuntime is true, it should be an absolute path to the polyfill.


var _Promise = require("@babel/runtime-corejs3/core-js-stable/promise");
var _Map = require("@babel/runtime-corejs3/core-js-stable/map.js");
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the extension .js should be added since we have specified package.json#exports.

@@ -1,6 +1,6 @@
var _regeneratorRuntime = require("<CWD>/packages/babel-runtime-corejs3/regenerator");

var _mapInstanceProperty = require("<CWD>/packages/babel-runtime-corejs3/core-js/instance/map");
var _mapInstanceProperty = require("<CWD>/packages/babel-runtime-corejs3/core-js/instance/map.js");
Copy link
Member Author

Choose a reason for hiding this comment

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

Here the extension is ok correct, because when using absolute or relative paths npm ignores package.json#exports (I added it for consistency with the corejs2 transform).

This is currently broken for helpers, but the fix falls in unrelated from this PR.

@nicolo-ribaudo nicolo-ribaudo merged commit 484667b into babel:main Feb 22, 2021
@nicolo-ribaudo nicolo-ribaudo deleted the runtime-use-polyfills-packages branch February 22, 2021 21:30
@merceyz
Copy link
Contributor

merceyz commented Feb 23, 2021

This seems to fix #12106 and #10759

@nicolo-ribaudo
Copy link
Member Author

At least not #12106, that needs to be fixed in regenerator itself.

This was referenced Mar 12, 2021
@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 May 25, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
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: Internal 🏠 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants