DEWP: Check for magic comments before minification

[anomiex]

What?

Look for magic wp:polyfill comments before minification.

Why?

Avoids having to configure Terser to preserve these comments, and also avoids having them in the minified JS.

See https://github.com/WordPress/gutenberg/pull/65292/files#r1767653086

How?

Minifiers run in the processAssets hook at the
PROCESS_ASSETS_STAGE_OPTIMIZE_SIZE stage. If we hook at the PROCESS_ASSETS_STAGE_OPTIMIZE_COMPATIBILITY stage to check for the magic comments, we don't have to worry about configuring Terser to preserve them (and won't have to have them making the output slightly larger either).

Testing Instructions

Build everything, then compare the result with the results of a build from the equivalent trunk version. Changes should be limited to removing unnecessary /* wp:polyfill */ comments in .min.js output files. wp-polyfills shoud not be being removed from any .asset.php files.

When I did this myself, I found that a few of the outputs (build/editor/index.min.asset.php, build/edit-site/index.min.asset.php, build/router/index.min.asset.php) now have wp-polyfills added in .assets.php where they didn't have it previously. I think the comments were getting dropped when Terser converts code like this

const foo = 1;
;// CONCATENATGED MODULE: ./packages/whatever/index.js
/* wp:polyfill */
const bar = 2;

to be like this

const foo = 1, bar = 2;

(compare https://www.npmjs.com/package/@automattic/i18n-check-webpack-plugin#user-content-lost-comments-due-to-expression-movement where something similar is discussed in relation to translator comments)

Testing Instructions for Keyboard

N/A

[github-actions]

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: anomiex <[email protected]>
Co-authored-by: gziolo <[email protected]>
Co-authored-by: sirreal <[email protected]>
Co-authored-by: sgomes <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[sgomes]

When I did this myself, I found that a few of the outputs (build/editor/index.min.asset.php, build/edit-site/index.min.asset.php, build/router/index.min.asset.php) now have wp-polyfills added in .assets.php where they didn't have it previously.

Good catch! 👍 It looks like these modules were already getting polyfills via transitive dependencies, but this would not necessarily always be the case going forward, so this should help us avoid future bugs.

[gziolo]

I was contemplating why the version didn't change for existing test cases, where the wp-polyfill was listed. However, I figured out we didn't use modification before.

[sirreal]

This seems great, will you add a CHANGELOG entry to the package? I think this can be an "enhancement."

[anomiex]

This seems great, will you add a CHANGELOG entry to the package? I think this can be an "enhancement."

Added!

[sirreal]

One thought about this, it's now hooked into another phase and doing more processing, what do you think of making it configurable now?

[anomiex]

Personally I think it's probably fine. But it's up to all of you.

[sirreal]

This looks good, thanks @anomiex!

[sirreal]

With #65292 in the latest package releases and considering #65582 (comment), I think this should be merged to the release branch so it's released soon. It's not a package that ships with Core but it should be the latest released version. I'm not totally sure how the flows work in this case so I've added backport labels.

[github-actions]

I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: 2f96654

[cbravobernal]

Cherry picked for Gutenberg 19.4 RC in 6cc91c7