-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Packages: only add polyfills where needed #65292
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
Changes from all commits
f0e3386
9a490fc
9d54993
74bab7c
1f7785b
5e926d3
0d03c67
a3d11f1
a1b0480
5a51225
d9ea190
061b7eb
91eb78d
28f1b5b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| module.exports = [ | ||
| // Ignore excessively strict polyfilling of `Array.prototype.push` to work | ||
| // around an obscure bug involving non-writable arrays. | ||
| // See https://issues.chromium.org/issues/42202623 for the details of the | ||
| // bug that leads to the polyfilling, and which we are choosing to ignore. | ||
| 'es.array.push', | ||
| // This is an IE-only feature which we don't use, and don't want to polyfill. | ||
| // @see https://github.com/WordPress/gutenberg/pull/49234 | ||
| 'web.immediate', | ||
| ]; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| // Babel plugin that looks for `core-js` imports (or requires) | ||
| // and replaces them with magic comments to mark the file as | ||
| // depending on wp-polyfill. | ||
| function replacePolyfills() { | ||
| return { | ||
| pre() { | ||
| this.hasAddedPolyfills = false; | ||
| }, | ||
| visitor: { | ||
| Program: { | ||
| exit( path ) { | ||
| if ( this.hasAddedPolyfills ) { | ||
| // Add magic comment to top of file. | ||
| path.addComment( 'leading', ' wp:polyfill ' ); | ||
| } | ||
| }, | ||
| }, | ||
| // Handle `import` syntax. | ||
| ImportDeclaration( path ) { | ||
| const source = path?.node?.source; | ||
| const name = source?.value || ''; | ||
|
|
||
| // Look for imports from `core-js`. | ||
| if ( name.startsWith( 'core-js/' ) ) { | ||
| // Remove import. | ||
| path.remove(); | ||
| this.hasAddedPolyfills = true; | ||
| } | ||
| }, | ||
|
|
||
| // Handle `require` syntax. | ||
| CallExpression( path ) { | ||
| const callee = path?.node?.callee; | ||
| const arg = path?.node?.arguments[ 0 ]; | ||
|
|
||
| if ( | ||
| ! callee || | ||
| ! arg || | ||
| callee.type !== 'Identifier' || | ||
| callee.name !== 'require' | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| // Look for requires for `core-js`. | ||
| if ( | ||
| arg.type === 'StringLiteral' && | ||
| arg.value.startsWith( 'core-js/' ) | ||
| ) { | ||
| // Remove import. | ||
| path.remove(); | ||
| this.hasAddedPolyfills = true; | ||
| } | ||
| }, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| module.exports = replacePolyfills; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| // Note: this fixture may need to be updated when the browserslist or the | ||
| // core-js dependencies are updated. | ||
| // It should always test a feature that is supported, but requires | ||
| // a polyfill to work across all supported browsers. | ||
| const foo = new URLSearchParams(); | ||
| window.fooSize = foo.size; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| /* wp:polyfill */ | ||
|
|
||
| // Nothing else, really. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| const DependencyExtractionWebpackPlugin = require( '../../..' ); | ||
|
|
||
| module.exports = { | ||
| plugins: [ new DependencyExtractionWebpackPlugin() ], | ||
| }; |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -146,7 +146,7 @@ module.exports = { | |||||
| }, | ||||||
| plugins: [ | ||||||
| ...plugins, | ||||||
| new DependencyExtractionWebpackPlugin( { injectPolyfill: true } ), | ||||||
| new DependencyExtractionWebpackPlugin( { injectPolyfill: false } ), | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
Does that sounds about right? To clarify, note that this is a separate build step to the one that actually adds the magic comments. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not 100% decided on this… let's leave it as-is.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That said, should we ever enable magic comments by default in the default Babel preset that WordPress maintains?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like a follow-up is very welcomed 👌 |
||||||
| new CopyWebpackPlugin( { | ||||||
| patterns: gutenbergPackages | ||||||
| .map( ( packageName ) => ( { | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,7 +25,7 @@ const baseConfig = { | |
| parallel: true, | ||
| terserOptions: { | ||
| output: { | ||
| comments: /translators:/i, | ||
| comments: /(translators:|wp:polyfill)/i, | ||
swissspidy marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you could avoid having to preserve the comment through Terser by checking for the "wp:polyfill" comments in the You could pass the "needs polyfill" flag between the two hooks with a map of some sort or by doing something like
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Awesome, I will have a look when I find some time in the next few days. |
||
| }, | ||
| compress: { | ||
| passes: 2, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.