-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Framework: Use babel-polyfill in place of runtime builtins #9643
Conversation
} ], | ||
isTestEnv && [ '@babel/preset-env' ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change for this package, should we update the changelog accordingly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do some cleanup to the get-babel-config.js
file and remove the override of useBuiltIns: false,
Nice work, I can't test and approve at the moment. It will be the first thing I'll do in the morning unless someone beats me to it. |
5 unit tests failed, investigating. |
I included a small CSS tweak to fix the block toolbars in 43cde18 (display flex elements in IE need flex-shrink: 0) if they are inside other flex elements :) I didn't do a separate PR because people won't be able to test otherwise. |
43cde18
to
06a2231
Compare
Hey hey looks like the tests pass now 🎉 |
lib/client-assets.php
Outdated
); | ||
wp_add_inline_script( | ||
'wp-polyfill-fetch', | ||
'wp.deprecated( "wp-polyfill-fetch script handle", { plugin: "Gutenberg", version: "4.0" } );' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it mean that wp-polyfill-ecmascript
contains also polyfill for fetch?
I can see some code for Promise
in https://cdnjs.cloudflare.com/ajax/libs/babel-polyfill/7.0.0/polyfill.js, but can't see anything related to fetch
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it relies on core-js
I don't think it's included https://github.com/zloirock/core-js#missing-polyfills
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, occurred to me as well the polyfill is only going to provide language features (the specification). Since fetch
is a browser standard, it needs to remain polyfilled. I'll add back.
@@ -8,7 +8,6 @@ module.exports = function( api ) { | |||
targets: { | |||
browsers: [ 'extends @wordpress/browserslist-config' ], | |||
}, | |||
useBuiltIns: 'usage', | |||
} ], | |||
isTestEnv && [ '@babel/preset-env', { | |||
useBuiltIns: 'usage', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to revert the change which removes:
useBuiltIns: 'usage',
from test environment because some tests were failing. Those which use Array.values
method which isn't supported by Node.
@youknowriad suggested using babel-polyfill
instead to have the setup closer to the browser but I found out that it can cause memory issues with Jest. See jestjs/jest#2755 and the line in the changelog:
Breaking Change: Replaced auto-loading of babel-polyfill with only regenerator-runtime, fixes a major memory leak.
@@ -16,6 +16,7 @@ describe( 'Babel preset default', () => { | |||
|
|||
const output = transform( input, { | |||
configFile: false, | |||
envName: 'production', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this change, we test again production configuration rather than test env ... It is more helpful now :)
We still need to test it against IE11, @jorgefilipecosta or @tofumatt can you help to ensure it works consistently? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not part of ECMAScript standard, so not provided through Babel polyfill
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Tested in IE and chrome. Works great.
I didn't re-test in IE but if @youknowriad says it works in IE11 that's good enough for me; dismissing my review 😄 |
Fixes #9640
Related: #9275
Alternative to: #9641
This pull request seeks to disable our use of the
@babel/runtime
useBuiltIns
option, replaced instead by@babel/polyfill
as a dependency of every script registered by Gutenberg. This could be further enhanced to conditionally load the script only for browsers where expected features are not supported, but at this time it is not evident how such a condition would be formed.While this technically adds a new script to be loaded, there is a net decrease in bundle size, from 2011.96kb total to 1803.72kb (+89.6kb polyfill) (minified, pre-gzip), for an overall savings of 118.64kb. Full build comparison below:
Before
After
Testing instructions:
Repeat Steps to Reproduce from #9640, verifying both in Internet Explorer and in your preferred browser that there are no issues loading or using the editor.