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

Packages: only add polyfills where needed #65292

Merged
merged 14 commits into from
Sep 18, 2024
2 changes: 2 additions & 0 deletions bin/packages/get-babel-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ module.exports = ( environment = '', file ) => {
name: `WP_BUILD_${ environment.toUpperCase() }`,
},
};
// Add `/* wp:polyfill */` magic comment where needed.
callerOpts.caller.addPolyfillComments = true;
switch ( environment ) {
case 'main':
// To be merged as a presetEnv option.
Expand Down
4 changes: 3 additions & 1 deletion packages/babel-preset-default/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
<!-- Learn how to maintain this file at https://github.com/WordPress/gutenberg/tree/HEAD/packages#maintaining-changelogs. -->

## Unreleased
## Internal

- Added `addPolyfillComments` option. When used, it will automatically add magic comments to mark files that need `wp-polyfill`.
sgomes marked this conversation as resolved.
Show resolved Hide resolved

## 8.7.0 (2024-09-05)

Expand Down
2 changes: 1 addition & 1 deletion packages/babel-preset-default/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ For example, if you'd like to use a new language feature proposal which has not

There is a complementary `build/polyfill.js` (minified version – `build/polyfill.min.js`) file available that polyfills ECMAScript features missing in the [browsers supported](https://make.wordpress.org/core/handbook/best-practices/browser-support/) by the WordPress project ([#31279](https://github.com/WordPress/gutenberg/pull/31279)). It's a drop-in replacement for the deprecated `@babel/polyfill` package, and it's also based on [`core-js`](https://github.com/zloirock/core-js) project.

This needs to be included before all your compiled Babel code. You can either prepend it to your compiled code or include it in a `<script>` before it.
This needs to be included in some cases, if the features being used require polyfills. You can either prepend it to your compiled code or include it in a `<script>` before it.

#### TC39 Proposals

Expand Down
11 changes: 6 additions & 5 deletions packages/babel-preset-default/bin/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,14 @@ const builder = require( 'core-js-builder' );
const { minify } = require( 'terser' );
const { writeFile } = require( 'fs' ).promises;

/**
* Internal dependencies
*/
const exclusions = require( '../polyfill-exclusions' );

builder( {
modules: [ 'es.', 'web.' ],
exclude: [
// 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',
],
exclude: exclusions,
summary: { console: { size: true, modules: true } },
targets: require( '@wordpress/browserslist-config' ),
filename: './build/polyfill.js',
Expand Down
14 changes: 14 additions & 0 deletions packages/babel-preset-default/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,12 @@
*/
const browserslist = require( 'browserslist' );

/**
* Internal dependencies
*/
const exclusions = require( './polyfill-exclusions' );
const replacePolyfills = require( './replace-polyfills' );

module.exports = ( api ) => {
let wpBuildOpts = {};
const isWPBuild = ( name ) =>
Expand All @@ -27,6 +33,13 @@ module.exports = ( api ) => {
'proposal-nullish-coalescing-operator',
'proposal-logical-assignment-operators',
],
...( wpBuildOpts.addPolyfillComments
? {
useBuiltIns: 'usage',
exclude: exclusions,
corejs: require( 'core-js/package.json' ).version,
}
: {} ),
};

if ( isTestEnv ) {
Expand Down Expand Up @@ -82,6 +95,7 @@ module.exports = ( api ) => {
},
],
maybeGetPluginTransformRuntime(),
wpBuildOpts.addPolyfillComments && replacePolyfills,
].filter( Boolean ),
};
};
10 changes: 10 additions & 0 deletions packages/babel-preset-default/polyfill-exclusions.js
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',
];
59 changes: 59 additions & 0 deletions packages/babel-preset-default/replace-polyfills.js
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;
6 changes: 6 additions & 0 deletions packages/babel-preset-default/test/fixtures/polyfill.js
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;
18 changes: 18 additions & 0 deletions packages/babel-preset-default/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,22 @@ describe( 'Babel preset default', () => {

expect( output.code ).toMatchSnapshot();
} );

test( 'transpilation includes magic comment when using the addPolyfillComments option', () => {
const filename = path.join( __dirname, '/fixtures/polyfill.js' );
const input = readFileSync( filename );

const output = transform( input, {
filename,
configFile: false,
envName: 'production',
presets: [ babelPresetDefault ],
caller: {
name: 'WP_BUILD_MAIN',
addPolyfillComments: true,
},
} );

expect( output.code ).toContain( '/* wp:polyfill */' );
} );
} );
34 changes: 27 additions & 7 deletions packages/dependency-extraction-webpack-plugin/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ class DependencyExtractionWebpackPlugin {
}
}

// Go through the assets and hash the sources. We can't just use
// Prepare to hash the sources. We can't just use
// `chunk.contentHash` because that's not updated when
// assets are minified. In practice the hash is updated by
// `RealContentHashPlugin` after minification, but it only modifies
Expand All @@ -278,12 +278,32 @@ class DependencyExtractionWebpackPlugin {
const { hashFunction, hashDigest, hashDigestLength } =
compilation.outputOptions;

const contentHash = chunkFiles
.sort()
.reduce( ( hash, filename ) => {
const asset = compilation.getAsset( filename );
return hash.update( asset.source.buffer() );
}, createHash( hashFunction ) )
const hashBuilder = createHash( hashFunction );

const processContentsForHash = ( content ) => {
hashBuilder.update( content );
};

// Prepare to look for magic comments, in order to decide whether
// `wp-polyfill` is needed.
const processContentsForMagicComments = ( content ) => {
if ( content.includes( '/* wp:polyfill */' ) ) {
chunkStaticDeps.add( 'wp-polyfill' );
}
};

// Go through the assets to process the sources.
// This allows us to generate hashes, as well as look for magic comments.
chunkFiles.sort().forEach( ( filename ) => {
const asset = compilation.getAsset( filename );
const content = asset.source.buffer();

processContentsForHash( content );
processContentsForMagicComments( content );
} );

// Finalise hash.
const contentHash = hashBuilder
.digest( hashDigest )
.slice( 0, hashDigestLength );

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,13 @@ exports[`DependencyExtractionWebpackPlugin modules Webpack \`overrides\` should
]
`;

exports[`DependencyExtractionWebpackPlugin modules Webpack \`polyfill-magic-comment\` should produce expected output: Asset file 'main.asset.php' should match snapshot 1`] = `
"<?php return array('dependencies' => array('wp-polyfill'), 'version' => '9725ab782f6b09598d3d', 'type' => 'module');
"
`;

exports[`DependencyExtractionWebpackPlugin modules Webpack \`polyfill-magic-comment\` should produce expected output: External modules should match snapshot 1`] = `[]`;

exports[`DependencyExtractionWebpackPlugin modules Webpack \`runtime-chunk-single\` should produce expected output: Asset file 'a.asset.php' should match snapshot 1`] = `
"<?php return array('dependencies' => array('@wordpress/blob'), 'version' => 'a1906cfc819b623c86f8', 'type' => 'module');
"
Expand Down Expand Up @@ -619,6 +626,13 @@ exports[`DependencyExtractionWebpackPlugin scripts Webpack \`overrides\` should
]
`;

exports[`DependencyExtractionWebpackPlugin scripts Webpack \`polyfill-magic-comment\` should produce expected output: Asset file 'main.asset.php' should match snapshot 1`] = `
"<?php return array('dependencies' => array('wp-polyfill'), 'version' => '464c785b5c938d4fde3f');
"
`;

exports[`DependencyExtractionWebpackPlugin scripts Webpack \`polyfill-magic-comment\` should produce expected output: External modules should match snapshot 1`] = `[]`;

exports[`DependencyExtractionWebpackPlugin scripts Webpack \`runtime-chunk-single\` should produce expected output: Asset file 'a.asset.php' should match snapshot 1`] = `
"<?php return array('dependencies' => array('wp-blob'), 'version' => 'd3cda564b538b44d38ef');
"
Expand Down
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() ],
};
2 changes: 1 addition & 1 deletion tools/webpack/packages.js
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ module.exports = {
},
plugins: [
...plugins,
new DependencyExtractionWebpackPlugin( { injectPolyfill: true } ),
new DependencyExtractionWebpackPlugin( { injectPolyfill: false } ),
Copy link
Member

Choose a reason for hiding this comment

The 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 false and apply the new behavior of searching for the comment string.

Suggested change
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.

Copy link
Contributor Author

@sgomes sgomes Sep 17, 2024

Choose a reason for hiding this comment

The 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:

  • 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

The 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 ) => ( {
Expand Down
2 changes: 1 addition & 1 deletion tools/webpack/shared.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

It sounds like a good follow up task.

I'll take that as an invitation. 🙂

#65582

Copy link
Member

Choose a reason for hiding this comment

The 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,
Expand Down
Loading