Packages: only add polyfills where needed #65292
Conversation
sgomes
requested review from
gziolo,
ntwb,
nerrad and
ajitbohra
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +178 B (+0.01%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
CC @sirreal, who will know how to check that I haven't broken the interactivity packages 🙂 |
|
Looking at the Playwright tests, it seems I may have broken the interactivity API after all. Will investigate tomorrow. |
| this.useModules && | ||
| externalRequest.includes( '@wordpress/polyfill' ) | ||
| ) { | ||
| return callback( null, '' ); |
There was a problem hiding this comment.
Is this attempting to ignore the module during when externalizing?
There was a problem hiding this comment.
Yes, that's correct! I sneaked this change in today to see if it fixed the tests. If it works, I'll see if I can simplify it later 🙂
There was a problem hiding this comment.
It looks like it would be necessary to be included somehow for @wordpress/interactivity-router.
There was a problem hiding this comment.
Right, I think this exposed the fact that while @wordpress/interactivity-router doesn't currently depend on the polyfills, it probably should, if we want it to work correctly in older browsers like Chrome 109. Although there isn't really a good mechanism to make that happen, as far as I know 🤔
|
I've got a local setup and can help test and debug this. I'm not sure what the quickest way is. It's doable via the playground but the way I'm familiar with requires several setup steps each time. |
Lightbox and router region navigation should work. There are also Interactivity API unit tests. |
Thank you @cbravobernal! |
|
Please hold off on reviewing while I investigate the interactivity router breakage. Thank you! |
sgomes
force-pushed
the
update/only-add-polyfills-to-packages-that-need-them
branch
from
7653449
to
05d8b9d
Compare
Intriguing, so that needs to be investigated. |
I think we may be preserving some undesired comments: I'll see if I can touch up that comment preservation regex. |
|
Everything should be working correctly now, after switching the strategy to magic comments instead of real imports, and rebasing on trunk! Please take a look when you have a chance! |
| @@ -146,7 +146,7 @@ module.exports = { | |||
| }, | |||
| plugins: [ | |||
| ...plugins, | |||
| new DependencyExtractionWebpackPlugin( { injectPolyfill: true } ), | |||
| new DependencyExtractionWebpackPlugin( { injectPolyfill: false } ), | |||
There was a problem hiding this comment.
What do you think of making this configurable?
I understand this would require more setup to work correctly for third parties.
This would behave like false and apply the new behavior of searching for the comment string.
| new DependencyExtractionWebpackPlugin( { injectPolyfill: false } ), | |
| new DependencyExtractionWebpackPlugin( { injectPolyfill: 'auto' } ), |
Maybe this isn't necessary, but it would also avoid applying this for third parties when it's not desired or add a path to expose it to third parties in the future.
There was a problem hiding this comment.
I'm happy to make changes, but I think we need to clarify the semantics here. As far as I can tell, what you're looking for is:
false: Don't add polyfills, regardless of the presence of magic comments.true: Add polyfills, regardless of the presence of magic comments.'auto': Add polyfills only when magic comments are present.
Does that sounds about right?
To clarify, note that this is a separate build step to the one that actually adds the magic comments.
node ./bin/packages/build.js builds the packages in situ, and wp-scripts build is the one that takes each built package from its src directory, extracts the dependencies, and places the results in the top-level build dir.
My reasoning for not adding a separate option was that if we didn't want auto polyfilling we probably wouldn't add magic comments in the first build step anyway (the addPolyfillComments option), but I'm happy to make changes if we feel it's still best to make this behaviour explicitly opt-in in DEWP as well.
There was a problem hiding this comment.
I'm not 100% decided on this… let's leave it as-is.
There was a problem hiding this comment.
The strategy outlined seems good overall. The question is whether we really want to keep the differences between false and auto? The magic comment gets injected only with custom Babe code. In effect, in WP ecosystem there should never be a case where polyfill gets listed as a dependency outside of Gutenberg and Core. The only exception would be a project that bundles in a custom way WP packages but it's unlikely they would use the webpack plugin to generate asset files.
There was a problem hiding this comment.
That said, should we ever enable magic comments by default in the default Babel preset that WordPress maintains?
There was a problem hiding this comment.
In effect, in WP ecosystem there should never be a case where polyfill gets listed as a dependency outside of Gutenberg and Core
Someone already filed Automattic/jetpack#39452 asking us to do it in Jetpack. 😀
I can't see any reason anyone would have the magic comment and not want the plugin to add the dependency.
There was a problem hiding this comment.
It looks like a follow-up is very welcomed 👌
|
Flaky tests detected in a8efe73. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10904460687
|
sgomes
force-pushed
the
update/only-add-polyfills-to-packages-that-need-them
branch
from
2b843ab
to
91eb78d
Compare
|
Rebased on trunk. |
.../dependency-extraction-webpack-plugin/test/fixtures/polyfill-magic-comment/webpack.config.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Jon Surrell <[email protected]>
sgomes
force-pushed
the
update/only-add-polyfills-to-packages-that-need-them
branch
from
b8660f5
to
28f1b5b
Compare
|
This is awesome. Tests look great. Simple yet powerful. Such a pleasure to collaborate on iterations to the build optimizations 😀 |
sgomes
deleted the
update/only-add-polyfills-to-packages-that-need-them
branch
|
This latest update seems to have broken our block plugins that used the babel preset. The two files being required are nowhere to be found in node_modules, throwing an error: Confirmed it by going to |
|
gutenberg/packages/babel-preset-default/package.json Lines 27 to 30 in 610aabe |
|
Thanks for the report @tnke, I'll work on getting a patch release out shortly. |
|
Thank you, everyone, and sorry for missing this 😕 |
|
Fix in #65481. |
| @@ -25,7 +25,7 @@ const baseConfig = { | |||
| parallel: true, | |||
| terserOptions: { | |||
| output: { | |||
| comments: /translators:/i, | |||
| comments: /(translators:|wp:polyfill)/i, | |||
There was a problem hiding this comment.
I think you could avoid having to preserve the comment through Terser by checking for the "wp:polyfill" comments in the PROCESS_ASSETS_STAGE_OPTIMIZE_COMPATIBILITY stage (rather than PROCESS_ASSETS_STAGE_ANALYSE as now).
You could pass the "needs polyfill" flag between the two hooks with a map of some sort or by doing something like compilation.updateAsset( filename, v => v, { wpPolyfill: flag } ); to set it and then asset.info.wpPolyfill to check it.
There was a problem hiding this comment.
That would work. It would be a bit more complex, as it would require scanning all the chunks twice. We decided to keep the comment so we don't have to recalculate the hash generated for chunks. It sounds like a good follow up task.
There was a problem hiding this comment.
There was a problem hiding this comment.
Awesome, I will have a look when I find some time in the next few days.
| @@ -146,7 +146,7 @@ module.exports = { | |||
| }, | |||
| plugins: [ | |||
| ...plugins, | |||
| new DependencyExtractionWebpackPlugin( { injectPolyfill: true } ), | |||
| new DependencyExtractionWebpackPlugin( { injectPolyfill: false } ), | |||
There was a problem hiding this comment.
In effect, in WP ecosystem there should never be a case where polyfill gets listed as a dependency outside of Gutenberg and Core
Someone already filed Automattic/jetpack#39452 asking us to do it in Jetpack. 😀
I can't see any reason anyone would have the magic comment and not want the plugin to add the dependency.
What?
Currently, all packages depend on
wp-polyfill, even when it's not really needed.This PR changes that, and automatically determines at build time whether a dependency on
wp-polyfillis needed, taking usage and browser support into account.This PR fixes #65216.
Why?
Packages like
wp-hooksandwp-i18nare often enqueued in the head, which means that their dependencies are placed there as well.wp-polyfillis rather large, and so we want to avoid loading it in the critical path whenever possible.How?
A new option was added to
packages/babel-preset-default, that enables adding magic comments.Enabling this option turns on the babel
useBuiltIns: 'usage'option, which addscore-jsimports to the output. These are then transformed by a small plugin into a/* wp:polyfill */magic comment instead.The webpack build then looks for the magic comments, and enables the polyfill dependency if it finds them.
In order to make this PR minimally effective, a new polyfill exclusion was also added, to avoid pedantically polyfilling
Array.prototype.pushdue to an obscure bug on some older browsers.Testing Instructions
Since this is a build-level change, the best approach is probably to build and smoke-test all of Gutenberg.
In particular, ensure that:
wp-i18nandwp-hooksdo not end up depending onwp-polyfillwp-urlandwp-block-librarydo end up depending onwp-polyfillinteractivityandinteractivity-routerdo not breakTesting Instructions for Keyboard
N/A