Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#52941 closed task (blessed) (fixed)

Remove `@babel/polyfill` in favor of `core-js/stable`.

Reported by: desrosj's profile desrosj Owned by: desrosj's profile desrosj
Milestone: 5.8 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch
Focuses: Cc:

Description

The @babel/polyfill is deprecated and should be replaced with core-js/stable:
https://babeljs.io/docs/en/babel-polyfill.

A related issue in Gutenberg with more details: https://github.com/WordPress/gutenberg/issues/22417.

Copied over from #52854 after it was brought up by @gziolo.

Change History (44)

#1 @desrosj
3 years ago

  • Milestone changed from Awaiting Review to 5.8

#2 @ocean90
3 years ago

#52946 was marked as a duplicate.

#3 @gziolo
3 years ago

It looks like we need to create a webpack entry point that imports core-js and regenerator-runtime.

import 'core-js/stable';
import 'regenerator-runtime/runtime';

They aren't defined as dependencies in package.json at the moment.

Thinking a bit more about other polyfill integrations we might also use the copy technique for individual scripts. Similar to how other polyfills in #52854 are handled.

This ticket was mentioned in PR #1191 on WordPress/wordpress-develop by gziolo.


3 years ago
#4

  • Keywords has-patch added

#5 @sergiomdgomes
3 years ago

The regenerator-runtime/runtime import shouldn't be necessary, but it shouldn't harm us either, provided that the Babel configuration is correctly set up to remove unnecessary polyfills.

Do note that core-js is extremely conservative in which polyfills to include, however, with tiny browser implementation bugs that anyone rarely cares about leading to an inclusion of the full polyfill for that feature.

As a reference, with last 2 chrome versions this setup produces a 9.7KB bundle (not gzipped), whereas with last 2 safari versions that bumps up to 54KB (not gzipped), which is the same value as for the current set of supported browsers minus IE 11 (i.e., Safari becomes the new bottleneck following the dropping of IE 11).

It may be worth diving a bit deeper and configuring the level of aggressiveness to something saner: https://github.com/zloirock/core-js#configurable-level-of-aggressiveness

Last edited 3 years ago by sergiomdgomes (previous) (diff)

gziolo commented on PR #1191:


3 years ago
#6

@sgomes, thanks for the pointer. I arrived at the same documentation before reading your comment when looking at how to reword notes in WordPress packages after dropping support for IE11.

#7 @gziolo
3 years ago

I see there is https://www.npmjs.com/package/core-js-builder that we could use to generate our own customized polyfill. I plan to test it in the Gutenberg repository first.

gziolo commented on PR #1191:


3 years ago
#8

I'm exploring a completely revised approach in https://github.com/WordPress/gutenberg/pull/31279.

desrosj commented on PR #1191:


3 years ago
#9

The revised approach was merged into the Gutenberg repository and will be merged to Core.

gziolo commented on PR #1191:


3 years ago
#10

Yes, good catch. I will try to publish the change to npm so we can try again using the polyfill from @wordpress/babel-preset-default

#11 @gziolo
3 years ago

I guess it should be as simple as publishing the recent changes for @wordpress/babel-preset-default to npm and replacing existing paths to the two files from the package:
https://github.com/WordPress/wordpress-develop/blob/95f678bf3df2058553c160db83fd695ea680ed14/tools/webpack/packages.js#L76
https://github.com/WordPress/wordpress-develop/blob/95f678bf3df2058553c160db83fd695ea680ed14/tools/webpack/packages.js#L91

The updated paths should be:
@wordpress/babel-preset-default/build/polyfill.js
@wordpress/babel-preset-default/build/polyfill.min.js

This ticket was mentioned in Slack in #core-editor by desrosj. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by desrosj. View the logs.


3 years ago

#14 @desrosj
3 years ago

This one can remain in the milestone post 5.8 beta 1. This is related to the IE 11 support drop in 5.8, so including this is preferable.

gziolo commented on PR #1361:


3 years ago
#16

I tested on macOS Big Sur with the latest:

  • Firefox
  • Safari
  • Chrome

Everything works as expected.

youknowriad commented on PR #1361:


3 years ago
#17

Looks good, what's the different in terms of Kb?

gziolo commented on PR #1361:


3 years ago
#18

When I move to dependencies:
{{{json
"@wordpress/babel-preset-default": "6.2.0",
}}}

I get the following error:

<img width="1464" alt="Screen Shot 2021-06-11 at 11 36 42" src="https://user-images.githubusercontent.com/699132/121666329-681dcf80-caa9-11eb-887b-c1f34e2a5659.png">

I'm not sure why it's happening :( Can we keep it as devDependencie?

youknowriad commented on PR #1361:


3 years ago
#19

I'm not sure why it's happening :( Can we keep it as devDependencie?

the error is weird but yeah we can keep there if it solves it.

desrosj commented on PR #1361:


3 years ago
#20

Just wanted to record the difference in file size here for historical reference:

#### Before
Unminified file: 242KB
Minified file: 102KB

#### After
Unminified file: 53KB
Minified file: 20KB

#21 @desrosj
3 years ago

  • Owner set to desrosj
  • Resolution set to fixed
  • Status changed from new to closed

In 51146:

Build/Test Tools: Replace the deprecated @babel/polyfill.

This replaces the dependency of the deprecated @babel/polyfill package with the core-js package through @wordpress/babel-preset-default.

Previously, the file consisted of a generalized group of polyfills, and not all of them were required. This change allows the contents of this file to be built according to the exact needs as dictated by the @wordpress/babel-preset-default package, which takes into account the current browser support.

Props gziolo, youknowriad.
Fixes #52941.

#23 @jeherve
3 years ago

This may be worth adding a dev-note, as some third-parties currently relying on @babel/polyfill via core-js may find themselves in a bit of a surprise when WordPress 5.8 hits. This is the case for the Jetpack plugin for example.

#24 @gziolo
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@jeherve, I think the same issue was raised in #core-editor channel on WordPress Slack today:
https://wordpress.slack.com/archives/C02QB2JS7/p1623850913102100

Hello all! With 5.8-beta2 , in a site with my block plugin, when I try to edit a post I get this error Uncaught ReferenceError: regeneratorRuntime is not defined Is this expected? Is there an easy way to fix this? Thank you in advance

The solution should be adding regeneratorRuntime as the dependency for wp-polyfill. It was previously included in @babel/polyfill.

This ticket was mentioned in Slack in #core-editor by gziolo. View the logs.


3 years ago

This ticket was mentioned in PR #1389 on WordPress/wordpress-develop by desrosj.


3 years ago
#26

…polyfill-build` script.

Trac ticket: https://core.trac.wordpress.org/ticket/52941

#27 @desrosj
3 years ago

I think the best way forward here is to fully revert, or return the wp-polyfill script to the original state and introduce a new script handle for the new version of the file. We can't assume which parts of the polyfill plugins and themes are interested in.

I've opened a PR to take a pass at the latter option, but unless the packages have their dependency changed from wp-polyfill to wp-polyfill-build upstream in Gutenberg, both versions of the polyfill would be loaded.

A few notes. I haven't been able to figure out the reason for adding `wp-polyfill` as a dependency of itself. It's not clear to me if this should remain as is, changed to the new script handle, or both.

Also, the version for the new script should probably change to reflect the version of the @wordpress/babel-preset-default package. Right now it reflects the version of @babel/polyfill.

This ticket was mentioned in Slack in #meta by dd32. View the logs.


3 years ago

#29 @gziolo
3 years ago

I'm on board with the proposed approach. We had too many reports where the expectation is that wp-polyfill used by popular plugins that offer blocks crash with the changes introduced. The alternative approach we see in the new patch should make the transition seamless.

The follow-up task would be to update@wordpress/dependency-extraction-webpack-plugin to allow setting wp-polyfill-build as the name for the injected polyfill and used that in WordPress core.

One more thing that we should consider would be ensuring that when wp-polyfill and wp-polyfill-build are enqueued, only wp-polyfill gets sent to the user because in practice wp-polyfill-build` is a subset of the former.

A few notes. I haven't been able to figure out the reason for adding wp-polyfill as a dependency of itself. It's not clear to me if this should remain as is, changed to the new script handle, or both.

It looks like this is an old change introduced while renaming the script handle from wp-polyfill-ecmascript to wp-polyfill in wp-polyfill-ecmascript. I would assume we can safely remove the line.

Also, the version for the new script should probably change to reflect the version of the @wordpress/babel-preset-default package. Right now it reflects the version of @babel/polyfill.

Yes, I missed that one previously.

#30 @herregroen
3 years ago

We're encountering this in Yoast SEO as well when testing with WordPress 5.8.

Reading the above I get the impresssion that the main difference is the lack of regenerator-runtime in core-js/stable.

If that's the case it could be worth looking into the option of keeping wp-polyfill around only as an alias rather than reverting the change entirely and having that alias depend on both the new polyfill package as well as a separate regenerator-runtime dependency.

I'm not sure if that covers the full scope of the changes but it could be an approach worth looking into.

That said, I do agree that it's very likely that a lot of plugins and themes will depend on regenerator-runtime and thus removing it is not an option. So either a revert or using wp-polyfill as an alias that enqueues both should be done.

This ticket was mentioned in PR #1405 on WordPress/wordpress-develop by desrosj.


3 years ago
#31

This is an alternate approach to fixing the regression in Core-52941.

In this PR:

  • The original wp-polyfill.js and wp-polyfill.min.js files are preserved to prevent 404 errors, just in case someone is targeting them directly.
  • A new wp-polyfill-built script handle is added to represent the new, custom built polyfill.
  • the regenerator-runtime script handle has been added representing the regenerator-runtime dependency. Scripts are built appropriately.
  • The wp-polyfill script handle is updated to be a psuedo script with wp-polyfill-built and regenerator-runtime as dependencies.

Trac ticket: https://core.trac.wordpress.org/ticket/52941.

desrosj commented on PR #1405:


3 years ago
#32

There was also a newer version of the @babel/polyfill dependency available. I updated the version used to include the final released version of this archived package.

#33 @desrosj
3 years ago

  • Keywords needs-testing added

@herregroen I quite like your suggestion of making the old wp-polyfill handle one that just loads the two required files. I've opened a new PR with an alternative approach.

I also think it's worth leaving the old wp-polyfill.js/wp-polyfill.min.js files in Core, just in case someone is attempting to load these directly.

@gziolo could you also give this a look?

#35 @desrosj
3 years ago

  • Keywords commit added; needs-testing removed

After some discussion in Slack, the most recent pull request is the route we're going to take her.

  • Add the regenerator-runtime dependency.
  • Add it to the Webpack config.
  • Add it as a dependency of wp-polyfill.

This should fix the issue plugins are experiencing. In a dev note, we'll call out that developers still needing regenerator-runtime should update their scripts to include this handle as a dependency and Core will remove this dependency from wp-polyfill in a future release.

#36 @desrosj
3 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 51212:

Build/Test Tools: Add the regenerator-runtime script as a dependency to wp-polyfill.

In [51146], the core-js package replaced the deprecated @babel//polyfill one. The core-js package builds wp-polyfill from a configuration provided by @wordpress/babel-preset-default instead of copying a one size fits all polyfill.

That change caused an issue where plugins and themes relying on the regenerator-runtime script being included in the wp-polyfill.js file encountering fatal JavaScript errors.

This adds the regenerator-runtime package to Core and registers it as a dependency for wp-polyfill. While Core does not require regenerator-runtime, it will allow for a smoother transition to using core-js.

This dependency will be removed in a future version of WordPress, so developers are encouraged to add regenerator-runtime as a dependency for any custom script that requires it.

Follow up to [51146].

Props gziolo, herregroen, jeherve, hellofromtonya, peterwilsoncc.
Fixes #52941.

#37 follow-ups: @desrosj
3 years ago

  • Keywords commit removed

@herregroen and @jeherve are you able to verify [51212] fixes the issue for you?

desrosj commented on PR #1405:


3 years ago
#39

Closing in favor of #1416, which was merged into Core.

desrosj commented on PR #1389:


3 years ago
#40

Closed in favor of #1416, which was merged into Core.

#41 in reply to: ↑ 37 @jeherve
3 years ago

Replying to desrosj:

@herregroen and @jeherve are you able to verify [51212] fixes the issue for you?

It does for me, thank you!

#42 follow-up: @gziolo
3 years ago

@herregroen and @jeherve are you able to verify [51212] fixes the issue for you?

@nerrad, can you confirm that WooCommerce blocks work correctly with WP 5.8 beta 5.3?

#43 in reply to: ↑ 37 @herregroen
3 years ago

Replying to desrosj:

@herregroen and @jeherve are you able to verify [51212] fixes the issue for you?

Yes! It does for me as well.

#44 in reply to: ↑ 42 @nerrad
3 years ago

Replying to gziolo:

@herregroen and @jeherve are you able to verify [51212] fixes the issue for you?

@nerrad, can you confirm that WooCommerce blocks work correctly with WP 5.8 beta 5.3?

Can confirm it fixes.

Note: See TracTickets for help on using tickets.