-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Regression in script dependencies in 19.5.0 #66552
Comments
Adding a few folks that have an interest here, from previous work on this: |
Yikes! Do we know when this regressed and whether this is in WP 6.7? We‘d need to prioritize it in that case. |
@swissspidy: As far as I can tell, it regressed with #65926, i.e. commit 72d2ff4. I'm not sure how to check whether that was part of WP 6.7 😕 |
#65926 was not backported to |
Another option to take care of a big chunk of the problem would be to drop support for chrome 109 (i.e. add |
Unfortunately, this kind of thing is an all-or-nothing affair, so we'd still need a way for common features not to be polyfilled at all 😕 Dropping support for chrome 109 while simultaneously excluding |
Dropping support for Chrome 109 means that as soon as we start using |
We don't really have any great solutions if we want to be able to use |
Currently 109 is at 1.43% globally, once that drops to 1% or below it'll be gone anyway with the existing browserslist config. 🤷 Breakdown by region
|
We should probably make a decision on this soon, before the regression ends up in a WP release 😕 Should we keep the current browser support and ban all the new methods? |
One thing I'd like to understand better is why Babel already doesn't detect specific method calls like But in our configuration it just bluntly adds all the Array polyfills that are needed for the supported browsers. The relevant code should be in the babel-plugin-polyfill-corejs3 package. |
I'll see if I can pinpoint the relevant code later, but one thought came to mind: could this be related to our use of |
I'm not 100% sure but I think In theory there could be a bugfix-only CoreJS polyfill but I don't see anything like this used by Babel. |
Thank you, @jsnajdr! I'm starting to suspect the arrays were a red herring, and the real issue is with |
Confirmed the array methods were a red herring. Simply doing require("core-js/modules/es.set.difference.v2.js");
require("core-js/modules/es.set.intersection.v2.js");
require("core-js/modules/es.set.is-disjoint-from.v2.js");
require("core-js/modules/es.set.is-subset-of.v2.js");
require("core-js/modules/es.set.is-superset-of.v2.js");
require("core-js/modules/es.set.symmetric-difference.v2.js");
require("core-js/modules/es.set.union.v2.js"); Excluding these features removes the dependency on |
It appears that polyfilling all possible features is an intentional limitation of how Set: define("set/index", SetDependencies), where: const SetDependencies = [
"es.set",
"es.set.difference.v2",
"es.set.intersection.v2",
"es.set.is-disjoint-from.v2",
"es.set.is-subset-of.v2",
"es.set.is-superset-of.v2",
"es.set.symmetric-difference.v2",
"es.set.union.v2",
"esnext.set.add-all",
"esnext.set.delete-all",
"esnext.set.difference",
"esnext.set.every",
"esnext.set.filter",
"esnext.set.find",
"esnext.set.intersection",
"esnext.set.is-disjoint-from",
"esnext.set.is-subset-of",
"esnext.set.is-superset-of",
"esnext.set.join",
"esnext.set.map",
"esnext.set.reduce",
"esnext.set.some",
"esnext.set.symmetric-difference",
"esnext.set.union",
...CommonIteratorsWithTag,
]; As such, anytime Similar problems exist with So I guess the question becomes: should we exclude these |
@jsnajdr / everyone else : Any thoughts on disabling the following
|
@jsnajdr / @swissspidy / @anomiex I created a new PR with the above proposal: #67230. Hopefully we can continue the discussion there! |
I'm curious how intentional this really is. The same module in
I.e., whenever Babel encounters a Could we have similar entries also for
@nicolo-ribaudo and @zloirock are the main contributors in this code, so they might be able to shed some light on this. Is it possible or is there some subtle gotcha? |
Unlike
I have some ideas on how to improve it in the future, but with the current architecture, I don't think that this option is a good idea. |
Does that mean that if I use some of the ES2024 method polyfills:
then some/all of them depend on the Looking at the polyfill, I see that it fixes bugs like In practice we are concerned mainly with Chrome 109 which is quite old but it has >1% usage so it's being polyfilled for according to our |
There are implicit dependencies of those modules. That means that the proper |
Description
#65292 introduced a mechanism to avoid adding
wp-polyfill
as a dependency to every package, based on what syntax features are actually in use. Unfortunately, a regression was since introduced where pretty much all packages now end up depending onwp-polyfill
again.The cause behind this is that new features are now being polyfilled, specifically new methods in arrays and sets.
Full list of polyfills
These new methods are not actually in use in all of these packages (if any?), but are being marked as needed due to severe limitations in
babel
. Essentially, and as far as I can tell,any code that uses an. Edit: this is incorrect, the real issue is withArray
will be marked as needing all of the array methods, even if they're not being used anywhereSet
. See the discussion below.There are two ways we could approach this:
polyfill-exclusions.js
Any of the above options will leave the methods unpolyfilled, however, and thus unavailable for general use in Gutenberg.
Beyond this, we should probably consider adding a CI job to detect similar issues in the future (e.g. by checking that a selection of basic packages, like
hooks
andi18n
, don't get a dependency onwp-polyfill
).I'd like to hear any of your thoughts in how to tackle this!
Step-by-step reproduction instructions
index.min.asset.php
files for packages likei18n
orhooks
, which shouldn't require polyfillsScreenshots, screen recording, code snippet
No response
Environment info
Please confirm that you have searched existing issues in the repo.
Please confirm that you have tested with all plugins deactivated except Gutenberg.
Please confirm which theme type you used for testing.
The text was updated successfully, but these errors were encountered: