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 a Babel polyfill script #746

Closed
wants to merge 3 commits into from
Closed

Use a Babel polyfill script #746

wants to merge 3 commits into from

Conversation

aduth
Copy link
Member

@aduth aduth commented May 9, 2017

Related: 94a3887#commitcomment-22078032

This pull request seeks to introduce babel-polyfill to ensure that ES2015+ base type instance methods are available in every supported environment (e.g. Array#find, String#startsWith). We already use the transform-runtime plugin which converts ES2015 static property methods like Object.assign into ES5 equivalents.

This is intended as a developer experience improvement. We don't need the polyfill, and in fact it incurs a non-trivial weight cost. But in my experience the lack of a polyfill is very frequently overlooked, and developers will expect all ES2015+ features to work out of the box. These are subtle bugs because evergreen browsers implement these functions natively, but older yet supported browsers do not (IE11).

But alas, I grow weary of being the polyfill naysayer and enforcer. Should anyone else volunteer to assume this role, we could consider forgoing it.

By specifying the useBuiltIns option in our babel-preset-env configuration, we can ensure that polyfills included are only those which are necessary for the browsers we target.

IE11 polyfill

Implementation notes:

Because of how our Webpack configuration is set up, the polyfill is wrapped by a wp.polyfill global. This isn't great, but still works as intended. Since we'll need to reconsider our bundling setup during core reconciliation, I'm fine to punt this to a future date.

Testing instructions:

This is difficult to test because the editor is quite broken in IE11. Instead, I merely changed the wp_register_script call for wp-esnext-polyfill to wp_enqueue_script so that the script is loaded on every screen, then proceeded to try ES2015 methods in IE11's developer tools console.

@aduth aduth added the [Type] Build Tooling Issues or PRs related to build tooling label May 9, 2017
@aduth aduth requested a review from youknowriad May 9, 2017 22:19
@@ -8,6 +8,7 @@ const ExtractTextPlugin = require( 'extract-text-webpack-plugin' );

const config = {
entry: {
polyfill: 'babel-polyfill',
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the useBuiltIns works if we include the polyfill like this. Since it's a babel transformation, I think the answer is "no". but I'm not certain.

@aduth
Copy link
Member Author

aduth commented May 10, 2017

I wonder if the useBuiltIns works if we include the polyfill like this. Since it's a babel transformation, I think the answer is "no". but I'm not certain.

Yep, you were right about this. I think to use it effectively we'd need to add it to each bundle, which is a fair bit wasteful in its duplication.

I did a bit of looking around and discovered that in the upcoming release of babel-preset-env, a new useBuiltIns: "usage" feature will be added that allows you tries to dynamically add polyfills by usage in the files themselves (babel/babel-preset-env#241).

This seems to cover our need quite well. There may still be some duplication of polyfills between bundles using the same methods. Not sure yet how we'd want to tackle that.

The new feature is available in the alpha release. I had some trouble getting it to work without also upgrading Babel to its current alpha release. At this point, I think it makes sense to punt this for reconsideration once those versions become stable. The current code can serve as reference when we revisit.

@aduth
Copy link
Member Author

aduth commented Jul 11, 2018

Over a year later, we're finally seeing useBuiltIns: "usage" come to be with #7832.

@gziolo
Copy link
Member

gziolo commented Jul 11, 2018

Yes, so far it only contributed to slight increase of the size of bundles but it should open us door for future tweaks using Webpack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants