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

Build: Remove @babel/polyfill in favor of core-js/stable #1191

Closed
wants to merge 1 commit into from
Closed

Build: Remove @babel/polyfill in favor of core-js/stable #1191

wants to merge 1 commit into from

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Apr 13, 2021

Trac ticket: https://core.trac.wordpress.org/ticket/52941


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@gziolo gziolo requested a review from desrosj April 13, 2021 15:55
@gziolo gziolo self-assigned this Apr 13, 2021
@@ -73,7 +73,8 @@ module.exports = function( env = { environment: 'production', watch: false, buil

const vendors = {
'lodash.js': 'lodash/lodash.js',
'wp-polyfill.js': '@babel/polyfill/dist/polyfill.js',
'wp-polyfill-core-js.js': 'core-js/stable/index.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.

@desrosj, I need to do some research because it doesn't look like core-js has a distribution version but rather a long list of require calls ...

@@ -121,7 +123,7 @@ function wp_default_packages_vendor( $scripts ) {
$scripts->add( $handle, $path, $dependencies, $version, 1 );
}

$scripts->add( 'wp-polyfill', null, array( 'wp-polyfill' ) );
$scripts->add( 'wp-polyfill', null, array( 'wp-polyfill-core-js', 'wp-polyfill-regenerator' ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good. We need to be careful to not change the script handle. I don't think the other changes above will have any affect on that. But we should just double check.

@sgomes
Copy link

sgomes commented Apr 27, 2021

Hey @gziolo! Thanks for this PR! 👍

I just wanted to bring up a note I left on the Trac ticket around the aggressiveness of core-js polyfilling. It tends to be extremely conservative regarding full feature correctness, even for rarely used things. It may be worth customising the aggressiveness a bit to avoid e.g. shipping a Promise polyfill unnecessarily.

@gziolo
Copy link
Member Author

gziolo commented Apr 28, 2021

@sgomes, thanks for the pointer. I arrived at the same documentation before reading your comment when looking at how to reword notes in WordPress packages after dropping support for IE11.

@gziolo
Copy link
Member Author

gziolo commented Apr 28, 2021

I'm exploring a completely revised approach in WordPress/gutenberg#31279.

@desrosj
Copy link
Contributor

desrosj commented May 24, 2021

The revised approach was merged into the Gutenberg repository and will be merged to Core.

@desrosj desrosj closed this May 24, 2021
@gziolo gziolo deleted the update/babel-polyfill branch May 24, 2021 13:32
@gziolo
Copy link
Member Author

gziolo commented May 24, 2021

Yes, good catch. I will try to publish the change to npm so we can try again using the polyfill from @wordpress/babel-preset-default

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants