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

Blocks: require wp-polyfill only when really needed #39452

Open
simison opened this issue Sep 19, 2024 · 9 comments · May be fixed by #39629
Open

Blocks: require wp-polyfill only when really needed #39452

simison opened this issue Sep 19, 2024 · 9 comments · May be fixed by #39629
Assignees
Labels
Build [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Focus] Performance [Pri] Normal [Status] In Progress [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it

Comments

@simison
Copy link
Member

simison commented Sep 19, 2024

Currently, all our blocks depend on the fairly large wp-polyfill package, even if they might not need it:

$script_dependencies[] = 'wp-polyfill';

If I remember right, we added the dependency circa ~2019 after we had several bad breakages on the front end of the site in older Internet Explorer browsers (which we don't support anymore). I haven't actually reviewed our view.js builds to see if wp-polyfill is in any of them currently, but that might also change over time and be error-prone if we ignore it.

Meanwhile, in Gutenberg, there's some work by @sgomes on packages to depend on wp-polyfill only when truly needed:

This is great because now, even if we wouldn't require wp-polyfill for blocks, simply depending on any core package like wp-dom-ready would pull in the dependency anyway.

If I understand the PR correctly, dependency-extraction-webpack-plugin now supports the mechanism out of the box and would conditionally output the wp-polyfill in the view.asset.php file only when truly needed.

Possible todos

  • Update our use of dependency-extraction-webpack-plugin
  • Remove hardcoded wp-polyfill dependency

cc @oskosk @kraftbj @jeherve

@simison simison added the Build label Sep 19, 2024
@jeherve jeherve added [Focus] Performance [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Pri] Normal labels Sep 19, 2024
@sgomes
Copy link
Contributor

sgomes commented Sep 19, 2024

Thank you for the mention, @simison!

Hey folks! 👋

To clarify, there are two parts to the change:

  • The Babel preset optionally (caller.addPolyfillComments = true) adds magic comments to transformed sources (of the form /* wp:polyfill */)
  • dependency-extraction-webpack-plugin looks for these magic comments when processing files, and adds the wp-polyfill dependency if it finds any

These changes were designed primarily for internal use in Gutenberg, but it should be possible to expose them to other consumers of these packages. It may require further changes, though, depending on how all of these pieces interact.

Happy to help anyone looking into this on the Jetpack side!

@anomiex
Copy link
Contributor

anomiex commented Sep 19, 2024

We'd have to change our own Babel config to generate those comments when appropriate, since we don't use Gutenberg's. Is there a Babel plugin to do only that, or is it only embedded inside your @wordpress/babel-preset-default?

@sgomes
Copy link
Contributor

sgomes commented Sep 19, 2024

We'd have to change our own Babel config to generate those comments when appropriate, since we don't use Gutenberg's. Is there a Babel plugin to do only that, or is it only embedded inside your @wordpress/babel-preset-default?

The plugin lives inside the babel-preset-default package; it's replace-polyfills.js on the root.

If you're going to be using your own babel config and the Gutenberg-generated polyfills script, then in order for everything to work correctly, you'll need to ensure that:

  • You use the same version of core-js and browserslist as Gutenberg. This is important so that they have the same browser support info.
  • You use the same polyfill exclusions as Gutenberg. This is important so that the choice of whether to use a polyfill matches the logic in Gutenberg.
  • You use useBuiltIns: 'usage' in Babel env, so that it adds the core-js imports that the plugin can remove and replace with magic comments.

You should end up with something along the lines of:

{
  presets: [
    // ...
    [ require.resolve( '@babel/preset-env' ), {
      useBuiltIns: 'usage',
      exclude: require( '@wordpress/babel-preset-default/polyfill-exclusions.js' ),
      corejs: require( 'core-js/package.json' ).version,
    } ],
    // ...
  ]
  plugins: [
    // ...
    require( '@wordpress/babel-preset-default/replace-polyfills.js' )
  ]
}

Note that I haven't tested this, so I may have gotten things wrong (like how to address the individual JS files inside the package).

@anomiex
Copy link
Contributor

anomiex commented Sep 19, 2024

You use the same version of core-js and browserslist as Gutenberg. This is important so that they have the same browser support info.

We support the current and previous versions of WordPress, so currently 6.5 and 6.6. Is that the same versions of core-js and browserslist as in the copy of Gutenberg bundled in 6.5 or 6.6? Or people might have installed the Gutenberg plugin, do we need to account for that too?

@sgomes
Copy link
Contributor

sgomes commented Sep 19, 2024

We support the current and previous versions of WordPress, so currently 6.5 and 6.6. Is that the same versions of core-js and browserslist as in the copy of Gutenberg bundled in 6.5 or 6.6? Or people might have installed the Gutenberg plugin, do we need to account for that too?

I don't know how wp-polyfill ends up making its way from Gutenberg to Core, sorry 😕 Perhaps @sirreal knows more about that?

As far as I know (and can tell from live sites), Gutenberg doesn't override wp-polyfill, and the copy from Core is the one that's always in use. So while there is no perfect way of handling multiple versions, the latest Gutenberg should always have the most up-to-date config, and I think it should be safe to base your config on that:

  • An update that drops a polyfill because it's no longer needed in browsers is not an issue. If the site is using an older versions of wp-polyfill, that's fine; it'll have the abandoned polyfill in there, but nothing will break.
  • An update that adds a polyfill because of a new feature becoming stable is not an issue either, since presumably Jetpack already avoids those features until they're part of the polyfills for the oldest supported version of WP, so they won't be present in the codebase.

But perhaps I'm missing something?

@anomiex
Copy link
Contributor

anomiex commented Sep 19, 2024

After doing some reading and some looking around, and then some thinking, here's what I think I know.

  • To a first approximation, wp-polyfills is core-js.
  • More specifically, it's a custom build of (a certain version of) core-js:
    • It only includes es. and web. modules. No esnext..
    • It excludes a few modules (currently es.array.push and web.immediate).
    • And of those, it only includes the modules that (a certain version of) browserslist says are needed for the browsers specified by (a certain version of) @wordpress/browserslist-config.
  • With Babel, core-js is commonly used via @babel/present-env (with useBuiltIns set) or @babel/plugin-transform-runtime (with corejs set). These both wind up using babel-plugin-polyfill-corejs3 under the hood.
    • @babel/present-env uses it in "usage-global" or "entry-global" mode, while @babel/plugin-transform-runtime uses it in "usage-pure" mode.
  • The docs for @babel/plugin-transform-runtime have a warning against setting useBuiltIns for @babel/present-env while using that plugin. But AFAICT you're good as long as you don't set corejs for @babel/plugin-transform-runtime and don't care about not polluting the global environment.
  • In "usage-global" mode, babel-plugin-polyfill-corejs3 adds an import 'core-js/module/es.whatever'; or require( 'core-js/module/es.whatever' ); when it sees code that needs the "es.whatever" polyfill.
    • Unless you set absoluteImports, in which case it'll be more like import '/path/to/some/node_modules/core-js/module/es.whatever';.
  • The code in @wordpress/babel-preset-default/replace-polyfills.js looks for any imports starting with "core-js/", replacing them with the magic comment.
    • It won't find "/path/to/some/node_modules/core-js/". That would probably be a problem for us, since we'll probably have to set absoluteImports.
    • It doesn't care whether the "core-js/" import is one that's included in any version of wp-polyfills. Any import is replaced.
      • Since the only thing "usage-global" mode does is insert those imports, nothing directly breaks if the polyfill isn't included in wp-polyfills. The feature just won't get polyfilled.
  • In the Jetpack monorepo's standard Babel config, we don't set either of the options needed for core-js to be used. Instead we load wp-polyfills everywhere and hope it has any polyfills we need.
  • Configs in packages/search and packages/wordads do enable core-js though. We'll have to make sure not to mess with those, at least to start with.

So, the current state is that we get whatever core-js polyfills are included in whatever version of wp-polyfills is bundled with WordPress and no others.

If we switch to the new method for the standard config, enabling core-js polyfilling but then replacing all the polyfill imports with magic comments, we'll still get whatever core-js polyfills are included in whatever version of wp-polyfills is bundled with WordPress and no others, even if we don't match versions. Specifically:

  • If our build inserts a "core-js/" polyfill that's included in wp-polyfills, we get the dep on wp-polyfills which provides the polyfill and we're good.
  • If our build misses inserting "core-js/" polyfill that's included in wp-polyfills, presumably we didn't need it anyway for our different browserslist config. So we're still good (as long as we don't let our own deps get too outdated that we miss polyfills for new stuff).
  • If our build inserts a "core-js/" polyfill that's not included in wp-polyfills, we get the dep on wp-polyfills but no polyfill is provided. But we were requiring wp-polyfills and not getting a polyfill before, so it's no worse.
    • Ideally in this situation we wouldn't remove the "core-js/" import in the first place, and so bundle the needed polyfill, but that would require knowing which modules are included in every relevant version of wp-polyfills.

So it looks like for an initial version we can ignore any version and config matching after all, but we'll need our own custom version of replace-polyfills.js that handles absoluteImports. For packages/search and packages/wordads we'll want to not enable the replace-polyfills.js so we don't accidentally lose polyfills not in wp-polyfills.

Does it sound like I got anything wrong in there?

@sirreal
Copy link
Member

sirreal commented Sep 19, 2024

I think everything is correct in that analysis @anomiex 👍

For packages/search and packages/wordads we'll want to not enable the replace-polyfills.js so we don't accidentally lose polyfills not in wp-polyfills.

One idea here is that you may be able to add the magic comment yourself to opt-in.

@sgomes
Copy link
Contributor

sgomes commented Sep 20, 2024

@anomiex Flawless research, @anomiex, that's all correct! 👍

@anomiex
Copy link
Contributor

anomiex commented Sep 25, 2024

I've got looking at this on my list, but for the moment I'm waiting on WordPress/gutenberg#65582 to avoid having to mess with the terser config.

@anomiex anomiex linked a pull request Oct 2, 2024 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Focus] Performance [Pri] Normal [Status] In Progress [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants