Improve dependency extraction to only include `wp-polyfill` when needed

[sgomes]

What problem does this address?

Currently, simple scripts like wp-hooks depend on wp-polyfill, even when they don't seem to make use of any of the features that are currently present in wp-polyfill:

The list of polyfilled features in `wp-polyfill` as of issue creation

``` es.array.push es.array.to-reversed es.array.to-sorted es.array.to-spliced es.array.with es.map.group-by es.object.group-by es.promise.with-resolvers es.regexp.flags es.string.is-well-formed es.string.to-well-formed es.typed-array.to-reversed es.typed-array.to-sorted for es.typed-array.with web.dom-exception.stack web.structured-clone web.url.can-parse web.url-search-params.delete web.url-search-params.has web.url-search-params.size ```

As far as I can tell, the dependency on wp-polyfill comes from a hardcoded boolean in tools/webpack/packages.js:

new DependencyExtractionWebpackPlugin( { injectPolyfill: true } ),

This approach is problematic, since it can lead to the fairly large wp-polyfill being loaded in the head to support simple scripts like wp-hooks and wp-i18n.

What is your proposed solution?

At a high level, it seems that DependencyExtractionWebpackPlugin should be able to determine on its own whether polyfills are needed, and the injectPolyfill configuration option should be dropped (or perhaps turned into an optional override).

Perhaps the plugin could look for core-js imports, as it currently does for WordPress ones? Though I'm not sure if it runs before or after Babel.

Alternatively, we could hardcode a list of excluded packages, but that would involve some amount of error-prone maintenance.

CC @gziolo @jsnajdr @tyxla for some ideas! 🙏

[gziolo]

At a high level, it seems that DependencyExtractionWebpackPlugin should be able to determine on its own whether polyfills are needed, and the injectPolyfill configuration option should be dropped (or perhaps turned into an optional override).

I'm all in favor of adding auto mode and making it the default at this point, which would detect whether a script entry depends on anything related to polyfill. It's very likely that the usage is very low today. We introduced that 6-7 years ago and the landscape has changes drastically 😄

Interestingly enough, this setting is set to true only for WP packages serving as script entry points. It doesn't apply to code for blocks or code bundles as script modules, where we know it is even less likely that it is still needed.

[tyxla]

Good one @sgomes!

Unless I'm seriously missing something, all of the polyfilled features are already baseline available, so I don't see a good reason to continue defaulting to including the polyfills.

Just like @gziolo, I'd be happy to see defaulting to an auto option that by default polyfills something only if really needed.

[sgomes]

Thank you both, @gziolo @tyxla! 🙌

I've got the outline of a solution, but it's a bit contrived, and I'm not sure I like it too much:

• We switch to using useBuiltIns: 'usage' in the babel config. This causes imports for core-js modules to be added.
• We add a Babel plugin that looks for these, removes them (we don't actually need them), and records in a separate file whether any were present
• We look for these files in DependencyExtractionWebpackPlugin when choosing whether to inject polyfills

Can you think of a better approach?

[sgomes]

CC @jsnajdr, who has a bunch of experience with babel and webpack plugins, and might know a better way.

[jsnajdr]

I'm thinking about this right now 🙂 The current polyfill includes the following modules:

es.array.push for {"chrome":"109","opera-android":"80","samsung":"25"}
es.array.to-reversed for {"chrome":"109"}
es.array.to-sorted for {"chrome":"109"}
es.array.to-spliced for {"chrome":"109"}
es.array.with for {"chrome":"109"}
es.map.group-by for {"chrome":"109","ios":"16.6-16.7","safari":"17.4"}
es.object.group-by for {"chrome":"109","ios":"16.6-16.7","safari":"17.4"}
es.promise.with-resolvers for {"chrome":"109","ios":"16.6-16.7","safari":"17.4","samsung":"25"}
es.regexp.flags for {"chrome":"109"}
es.string.is-well-formed for {"chrome":"109"}
es.string.to-well-formed for {"chrome":"109"}
es.typed-array.to-reversed for {"chrome":"109"}
es.typed-array.to-sorted for {"chrome":"109"}
es.typed-array.with for {"chrome":"109"}
web.dom-exception.stack for {"android":"125","chrome":"109","chrome-android":"125","edge":"124","ios":"16.6-16.7","opera":"109","opera-android":"80","safari":"17.4","samsung":"25"}
web.structured-clone for {"android":"125","chrome":"109","chrome-android":"125","edge":"124","firefox":"126","ios":"16.6-16.7","opera":"109","opera-android":"80","safari":"17.4","samsung":"25"}
web.url.can-parse for {"chrome":"109","ios":"16.6-16.7","samsung":"25"}
web.url-search-params.delete for {"chrome":"109","ios":"16.6-16.7","samsung":"25"}
web.url-search-params.has for {"chrome":"109","ios":"16.6-16.7","samsung":"25"}
web.url-search-params.size for {"chrome":"109","ios":"16.6-16.7"}

It mostly adds new methods, except the Array.push polyfill (no idea yet what it does) and something with an exception stack.

It puzzles me that Chrome 109 is mentioned, since we are supposed to support only last 2 Chrome version and Chrome is currently at version 128. Let me try to upgrade the packages with compat data, if that radically changes the output.

The useBuiltIns: 'usage' method looks like something that could work 🙂

[gziolo]

I did a quick test by including some code that should use polyfills:

const arr = [ 1, 2, 3, 4, 5 ];
console.log( arr.with( 2, 6 ) );

const illFormed = 'https://example.com/search?q=\uD800';

try {
    encodeURI( illFormed );
} catch ( e ) {
    console.log( e ); // URIError: URI malformed
}

console.log( encodeURI( illFormed.toWellFormed() ) );

However, nothing got added to the file processed by Babel as expected as we have useBuiltIns disabled. If that trick with useBuiltIns would work, and we can keep the same content of the files as published to npm now, then it's perfect.

Alternative path. Can we go the other way around and disallow code that requires polyfilling? In effect, if we detect not fully supported syntax the Babel processing errors or ESLint complaints?

[sgomes]

Thank you for your comment, @jsnajdr! To answer your questions:

It mostly adds new methods, except the Array.push polyfill (no idea yet what it does)

It works around an obscure bug in some browsers: https://issues.chromium.org/issues/42202623

I'm planning on excluding this from the list of polyfills, as this seems to me like yet another case of core-js being too pedantic.

It puzzles me that Chrome 109 is mentioned, since we are supposed to support only last 2 Chrome version and Chrome is currently at version 128

We also support > 1%, and Chrome 109 is included there, unfortunately. I believe it's because that's the last version of Chrome supported in several versions of Windows, including the still-quite-popular Windows 7.

[sgomes]

Alternative path. Can we go the other way around and disallow code that requires polyfilling? In effect, if we detect not fully supported syntax the Babel processing errors or ESLint complaints?

That's an intriguing option, thank you @gziolo! Are we okay with disabling all of the functionality that @jsnajdr listed in his comment (except for es.array.push, see above comment), across all of packages/?

[jsnajdr]

We also support > 1%, and Chrome 109

Yes, that's what I found after upgrading caniuse-lite didn't help. Chrome 109 has 1.43% usage according to the data.

[gziolo]

Alternative path. Can we go the other way around and disallow code that requires polyfilling? In effect, if we detect not fully supported syntax the Babel processing errors or ESLint complaints?

That's an intriguing option, thank you @gziolo! Are we okay with disabling all of the functionality that @jsnajdr listed in his comment (except for es.array.push, see above comment), across all of packages/?

I would consider it today as the type of polyfills included looks like nice to haves. In the past, we needed it to be able to use all the latest shiny features that had large impact on the developer experience. The list you shared needs to be investigated to check actual usage.

[sgomes]

The list you shared needs to be investigated to check actual usage.

I'm already knee-deep in babel polyfills, so this shouldn't take too long 👍 I'll let you know what I find!

[sgomes]

@gziolo : It doesn't look like it's possible while we need to polyfill web.url-search-params.size. Too many packages end up depending on it.

Here's a list of packages and one of the polyfills they use (there may be more):

packages/block-library/src/index.js: Needs web.url-search-params.size
packages/block-library/src/embed/embed-preview.native.js: Needs web.url-search-params.size
packages/block-library/src/embed/edit.native.js: Needs web.url-search-params.size
packages/block-library/src/embed/edit.js: Needs web.url-search-params.size
packages/block-library/src/form/view.js: Needs web.url-search-params.size
packages/blob/src/index.ts: Needs web.url-search-params.size
packages/e2e-test-utils/src/create-url.js: Needs web.url-search-params.size
packages/e2e-test-utils/src/is-current-url.js: Needs web.url-search-params.size
packages/react-native-editor/src/globals.js: Needs core-js/features/array/flat-map
packages/interactivity-router/src/index.ts: Needs web.url-search-params.size
packages/router/src/history.js: Needs web.url-search-params.size
packages/sync/src/webrtc-http-stream-signaling.js: Needs web.url-search-params.size
packages/url/src/get-filename.js: Needs web.url-search-params.size
packages/url/src/get-query-string.js: Needs web.url-search-params.size
packages/url/src/is-url.js: Needs web.url-search-params.size
packages/e2e-test-utils/src/mocks/create-embedding-matcher.js: Needs web.url-search-params.size
packages/sync/src/y-webrtc/y-webrtc.js: Needs es.typed-array.to-reversed
packages/sync/src/y-webrtc/crypto.js: Needs es.typed-array.to-reversed
packages/block-editor/src/components/iframe/index.js: Needs web.url-search-params.size
packages/block-editor/src/components/image-editor/use-transform-image.js: Needs web.url-search-params.size
packages/blocks/src/api/raw-handling/image-corrector.js: Needs web.dom-exception.stack
packages/edit-site/src/hooks/use-theme-style-variations/use-theme-style-variations-by-property.js: Needs web.dom-exception.stack
packages/edit-site/src/hooks/push-changes-to-global-styles/index.js: Needs web.dom-exception.stack
packages/editor/src/components/media-categories/index.js: Needs web.url-search-params.size
packages/widgets/src/blocks/legacy-widget/edit/control.js: Needs web.url-search-params.size

[jsnajdr]

It works around an obscure bug in some browsers:

I see it now, old versions of V8 have a bug when the .length property is not writable and you push nothing (array.push()), then it complains about the unwritability of .length although the length doesn't need to be updated 🤦

[sgomes]

I see it now, old versions of V8 have a bug when the .length property is not writable and you push nothing (array.push()), then it complains about the unwritability of .length although the length doesn't need to be updated 🤦

I think it should be okay to ignore this polyfill, and have browsers work on a best-effort basis there. It's already highly unlikely that our code will somehow run into such a contrived scenario, let alone it happening to the 1.4% of users on a browser with the bug. What do you think, @jsnajdr?

[sgomes]

In the meantime, I'll continue developing the "auto" mode we discussed, based on the approach I laid out.

[sgomes]

I think I got it working, and it turned out pretty clean! 🙂 Please take a look at #65292 when you get a chance!