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

Regression in script dependencies in 19.5.0 #66552

Open
2 of 6 tasks
sgomes opened this issue Oct 29, 2024 · 22 comments · May be fixed by #67230
Open
2 of 6 tasks

Regression in script dependencies in 19.5.0 #66552

sgomes opened this issue Oct 29, 2024 · 22 comments · May be fixed by #67230
Labels
[Type] Bug An existing feature does not function as intended

Comments

@sgomes
Copy link
Contributor

sgomes commented Oct 29, 2024

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 on wp-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
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.array-buffer.detached for {"chrome":"109"}
es.array-buffer.transfer for {"chrome":"109"}
es.array-buffer.transfer-to-fixed-length for {"chrome":"109"}
es.map.group-by for {"chrome":"109","ios":"17.5","safari":"17.5"}
es.object.group-by for {"chrome":"109","ios":"17.5","safari":"17.5"}
es.promise.with-resolvers for {"chrome":"109"}
es.regexp.flags for {"chrome":"109"}
es.set.difference.v2 for {"chrome":"109","ios":"17.5","opera-android":"80","safari":"17.5","samsung":"25"}
es.set.intersection.v2 for {"chrome":"109","ios":"17.5","opera-android":"80","safari":"17.5","samsung":"25"}
es.set.is-disjoint-from.v2 for {"chrome":"109","ios":"17.5","opera-android":"80","safari":"17.5","samsung":"25"}
es.set.is-subset-of.v2 for {"chrome":"109","ios":"17.5","opera-android":"80","safari":"17.5","samsung":"25"}
es.set.is-superset-of.v2 for {"chrome":"109","ios":"17.5","opera-android":"80","safari":"17.5","samsung":"25"}
es.set.symmetric-difference.v2 for {"chrome":"109","ios":"17.5","opera-android":"80","safari":"17.5","samsung":"25"}
es.set.union.v2 for {"chrome":"109","ios":"17.5","opera-android":"80","safari":"17.5","samsung":"25"}
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":"129","chrome":"109","chrome-android":"129","edge":"127","ios":"17.5","opera":"113","opera-android":"80","safari":"17.5","samsung":"25"}
web.structured-clone for {"android":"129","chrome":"109","chrome-android":"129","edge":"127","firefox":"129","ios":"17.5","opera":"113","opera-android":"80","safari":"17.5","samsung":"25"}
web.url.can-parse for {"chrome":"109"}
web.url.parse for {"chrome":"109","ios":"17.5","opera-android":"80","safari":"17.5","samsung":"25"}
web.url-search-params.delete for {"chrome":"109"}
web.url-search-params.has for {"chrome":"109"}
web.url-search-params.size for {"chrome":"109"}

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 Array will be marked as needing all of the array methods, even if they're not being used anywhere. Edit: this is incorrect, the real issue is with Set. See the discussion below.

There are two ways we could approach this:

  • We could exclude the problematic polyfills explicitly in polyfill-exclusions.js
  • We could roll back the package updates that triggered the change

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 and i18n, don't get a dependency on wp-polyfill).

I'd like to hear any of your thoughts in how to tackle this!

Step-by-step reproduction instructions

  1. Build Gutenberg
  2. Analyse the index.min.asset.php files for packages like i18n or hooks, which shouldn't require polyfills

Screenshots, screen recording, code snippet

No response

Environment info

  • Gutenberg 19.5.0 and later.

Please confirm that you have searched existing issues in the repo.

  • Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

  • Yes

Please confirm which theme type you used for testing.

  • Block
  • Classic
  • Hybrid (e.g. classic with theme.json)
  • Not sure
@sgomes sgomes added the [Type] Bug An existing feature does not function as intended label Oct 29, 2024
@sgomes
Copy link
Contributor Author

sgomes commented Oct 29, 2024

Adding a few folks that have an interest here, from previous work on this:

@anomiex @gziolo @jsnajdr @sirreal @swissspidy

@swissspidy
Copy link
Member

Yikes! Do we know when this regressed and whether this is in WP 6.7? We‘d need to prioritize it in that case.

@sgomes
Copy link
Contributor Author

sgomes commented Oct 29, 2024

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 😕

@jsnajdr
Copy link
Member

jsnajdr commented Oct 29, 2024

#65926 was not backported to wp/6.7 and the relevant packages still have the old versions in that branch. So we're dealing with a regression that is present only in trunk and in the Gutenberg plugin.

@anomiex
Copy link
Contributor

anomiex commented Oct 29, 2024

There are two ways we could approach this:

  • We could exclude the problematic polyfills explicitly in polyfill-exclusions.js
  • We could roll back the package updates that triggered the change

Another option to take care of a big chunk of the problem would be to drop support for chrome 109 (i.e. add not chrome 109 to the browserslist config). It seems likely it's still at > 1% due to it being the last version of Chrome supporting Windows 7 and 8.

@sgomes
Copy link
Contributor Author

sgomes commented Oct 30, 2024

Another option to take care of a big chunk of the problem would be to drop support for chrome 109 (i.e. add not chrome 109 to the browserslist config). It seems likely it's still at > 1% due to it being the last version of Chrome supporting Windows 7 and 8.

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 es.map.group-by and es.object.group-by might do the trick?

@jsnajdr
Copy link
Member

jsnajdr commented Oct 30, 2024

Dropping support for Chrome 109 means that as soon as we start using array.toSorted their Gutenberg will start crashing and they will be completely cut off. I think the benefit is not big enough to justify this.

@sgomes
Copy link
Contributor Author

sgomes commented Oct 30, 2024

We don't really have any great solutions if we want to be able to use array.toSorted. We'd either need to polyfill it for every package (i.e., maintain the status quo); or we'd need to somehow improve Babel's feature detection to check whether it's actually being used, rather than checking for whether any arrays are being used.

@anomiex
Copy link
Contributor

anomiex commented Oct 30, 2024

Dropping support for Chrome 109 means that as soon as we start using array.toSorted their Gutenberg will start crashing and they will be completely cut off. I think the benefit is not big enough to justify this.

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
These browsers account for 1.43% of all users globally
                           0% of all users in the CX
                           0% of all users in the NU
                           0% of all users in the PN
                           0% of all users in the TK
                           0.01% of all users in the BM
                           0.02% of all users in the AS
                           0.03% of all users in the GN
                           0.04% of all users in the ALT-AN
                           0.04% of all users in the TD
                           0.06% of all users in the CF
                           0.07% of all users in the NR
                           0.09% of all users in the VU
                           0.12% of all users in the GL
                           0.15% of all users in the BS
                           0.15% of all users in the GY
                           0.15% of all users in the JE
                           0.16% of all users in the KI
                           0.17% of all users in the HT
                           0.17% of all users in the MC
                           0.18% of all users in the MH
                           0.18% of all users in the NF
                           0.19% of all users in the SL
                           0.21% of all users in the SD
                           0.22% of all users in the PM
                           0.23% of all users in the GD
                           0.23% of all users in the JM
                           0.24% of all users in the BZ
                           0.25% of all users in the LC
                           0.25% of all users in the VG
                           0.27% of all users in the KY
                           0.27% of all users in the MS
                           0.28% of all users in the SB
                           0.31% of all users in the NE
                           0.31% of all users in the SO
                           0.33% of all users in the ML
                           0.35% of all users in the DM
                           0.35% of all users in the MV
                           0.36% of all users in the CD
                           0.37% of all users in the GF
                           0.37% of all users in the PG
                           0.39% of all users in the AI
                           0.39% of all users in the IE
                           0.39% of all users in the YT
                           0.41% of all users in the KN
                           0.41% of all users in the MW
                           0.42% of all users in the LR
                           0.42% of all users in the SC
                           0.42% of all users in the ZM
                           0.43% of all users in the NO
                           0.45% of all users in the IM
                           0.46% of all users in the AE
                           0.46% of all users in the CU
                           0.46% of all users in the PS
                           0.47% of all users in the SA
                           0.48% of all users in the PF
                           0.5% of all users in the AG
                           0.51% of all users in the YE
                           0.52% of all users in the AD
                           0.52% of all users in the BH
                           0.55% of all users in the FJ
                           0.55% of all users in the FK
                           0.56% of all users in the CR
                           0.56% of all users in the DJ
                           0.56% of all users in the KW
                           0.58% of all users in the CG
                           0.58% of all users in the GB
                           0.58% of all users in the NZ
                           0.58% of all users in the TO
                           0.59% of all users in the CM
                           0.59% of all users in the SG
                           0.59% of all users in the VC
                           0.6% of all users in the ZW
                           0.61% of all users in the PL
                           0.62% of all users in the RW
                           0.63% of all users in the GU
                           0.63% of all users in the US
                           0.64% of all users in the KM
                           0.65% of all users in the AW
                           0.65% of all users in the CV
                           0.65% of all users in the KR
                           0.65% of all users in the LU
                           0.65% of all users in the NL
                           0.67% of all users in the FI
                           0.67% of all users in the VN
                           0.68% of all users in the ALT-OC
                           0.7% of all users in the ALT-NA
                           0.7% of all users in the KH
                           0.7% of all users in the MM
                           0.71% of all users in the AU
                           0.72% of all users in the CK
                           0.72% of all users in the DK
                           0.72% of all users in the LS
                           0.72% of all users in the NC
                           0.73% of all users in the BI
                           0.73% of all users in the DE
                           0.73% of all users in the QA
                           0.73% of all users in the ZA
                           0.74% of all users in the MQ
                           0.74% of all users in the NG
                           0.75% of all users in the BE
                           0.75% of all users in the MR
                           0.75% of all users in the SZ
                           0.77% of all users in the MP
                           0.77% of all users in the SR
                           0.78% of all users in the AT
                           0.82% of all users in the IL
                           0.82% of all users in the SY
                           0.83% of all users in the UG
                           0.85% of all users in the JO
                           0.85% of all users in the RE
                           0.86% of all users in the OM
                           0.86% of all users in the PW
                           0.87% of all users in the IQ
                           0.87% of all users in the LA
                           0.89% of all users in the CA
                           0.9% of all users in the BB
                           0.91% of all users in the SE
                           0.92% of all users in the GG
                           0.92% of all users in the VA
                           0.93% of all users in the KP
                           0.93% of all users in the PA
                           0.94% of all users in the VI
                           0.95% of all users in the TZ
                           0.96% of all users in the ES
                           0.96% of all users in the GP
                           0.96% of all users in the JP
                           0.97% of all users in the GT
                           0.98% of all users in the GM
                           0.99% of all users in the PT
                           1% of all users in the LY
                           1% of all users in the MT
                           1.02% of all users in the SN
                           1.08% of all users in the CY
                           1.1% of all users in the PR
                           1.11% of all users in the FM
                           1.12% of all users in the EE
                           1.14% of all users in the BD
                           1.18% of all users in the BF
                           1.19% of all users in the AX
                           1.2% of all users in the BJ
                           1.2% of all users in the KE
                           1.21% of all users in the ALT-EU
                           1.21% of all users in the HN
                           1.22% of all users in the LT
                           1.22% of all users in the PH
                           1.23% of all users in the CO
                           1.24% of all users in the LI
                           1.26% of all users in the CZ
                           1.26% of all users in the ID
                           1.27% of all users in the BT
                           1.27% of all users in the HK
                           1.28% of all users in the CL
                           1.28% of all users in the HU
                           1.3% of all users in the FR
                           1.31% of all users in the AL
                           1.31% of all users in the CN
                           1.31% of all users in the MU
                           1.31% of all users in the TC
                           1.33% of all users in the RO
                           1.35% of all users in the GH
                           1.38% of all users in the DO
                           1.38% of all users in the MO
                           1.4% of all users in the TT
                           1.41% of all users in the ALT-AF
                           1.43% of all users in the ALT-WW
                           1.46% of all users in the AF
                           1.48% of all users in the LB
                           1.49% of all users in the NA
                           1.49% of all users in the NI
                           1.5% of all users in the LK
                           1.52% of all users in the GA
                           1.52% of all users in the HR
                           1.54% of all users in the NP
                           1.55% of all users in the TJ
                           1.56% of all users in the WS
                           1.57% of all users in the GI
                           1.61% of all users in the AM
                           1.63% of all users in the AO
                           1.64% of all users in the MZ
                           1.64% of all users in the SV
                           1.68% of all users in the TL
                           1.7% of all users in the CI
                           1.73% of all users in the ALT-AS
                           1.76% of all users in the IN
                           1.76% of all users in the MY
                           1.78% of all users in the BN
                           1.78% of all users in the BW
                           1.78% of all users in the UZ
                           1.8% of all users in the FO
                           1.81% of all users in the SK
                           1.89% of all users in the IS
                           1.93% of all users in the CH
                           1.94% of all users in the MX
                           1.98% of all users in the ME
                           2.05% of all users in the MN
                           2.12% of all users in the KZ
                           2.12% of all users in the SI
                           2.14% of all users in the ET
                           2.15% of all users in the GQ
                           2.15% of all users in the IT
                           2.15% of all users in the UY
                           2.21% of all users in the KG
                           2.21% of all users in the ST
                           2.22% of all users in the EC
                           2.25% of all users in the PE
                           2.27% of all users in the PK
                           2.32% of all users in the MK
                           2.42% of all users in the TH
                           2.49% of all users in the LV
                           2.55% of all users in the BG
                           2.55% of all users in the ER
                           2.63% of all users in the MA
                           2.68% of all users in the TG
                           2.77% of all users in the RU
                           2.9% of all users in the IR
                           2.91% of all users in the BY
                           3.01% of all users in the TW
                           3.06% of all users in the BA
                           3.18% of all users in the TR
                           3.31% of all users in the EG
                           3.39% of all users in the MD
                           3.5% of all users in the ALT-SA
                           3.51% of all users in the RS
                           3.54% of all users in the TN
                           3.67% of all users in the DZ
                           3.74% of all users in the BO
                           3.74% of all users in the UA
                           3.84% of all users in the AR
                           3.99% of all users in the SM
                           4.11% of all users in the PY
                           4.16% of all users in the GE
                           4.22% of all users in the BR
                           4.5% of all users in the AZ
                           4.66% of all users in the SH
                           4.73% of all users in the TM
                           5.11% of all users in the GW
                           5.28% of all users in the VE
                           5.56% of all users in the MG
                           5.61% of all users in the TV
                           5.79% of all users in the GR
                           16.91% of all users in the WF

@sgomes
Copy link
Contributor Author

sgomes commented Nov 12, 2024

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?

@jsnajdr
Copy link
Member

jsnajdr commented Nov 13, 2024

we'd need to somehow improve Babel's feature detection to check whether it's actually being used, rather than checking for whether any arrays are being used.

One thing I'd like to understand better is why Babel already doesn't detect specific method calls like .toSorted. I thought that it already worked this way for a long time: Babel sees something.toSorted(), and that triggers inclusion of the es.array.to-sorted polyfill. That's what the useBuiltIns: 'usage' option should be about.

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.

@sgomes
Copy link
Contributor Author

sgomes commented Nov 13, 2024

One thing I'd like to understand better is why Babel already doesn't detect specific method calls like .toSorted

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 bugfixes: true?

@jsnajdr
Copy link
Member

jsnajdr commented Nov 13, 2024

I'm not 100% sure but I think bugfixes are not related to polyfills. I've seen it only when related to syntax transforms. For example, Safari might have a bug related to arrow functions or spread syntax. With bugfixes: false this leads to all such syntax being transpiled to ES5 code. While with bugfixes: true there is more precise detection whether the code actually triggers the bug and the transpilation is more targeted.

In theory there could be a bugfix-only CoreJS polyfill but I don't see anything like this used by Babel.

@sgomes
Copy link
Contributor Author

sgomes commented Nov 13, 2024

Thank you, @jsnajdr!

I'm starting to suspect the arrays were a red herring, and the real issue is with Set. Will investigate further and report back when I have something concrete.

@sgomes
Copy link
Contributor Author

sgomes commented Nov 13, 2024

Confirmed the array methods were a red herring. Simply doing new Set() appears to result in the following:

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 wp-polyfill from hooks and i18n. Now to look into why new Set() is enough to require all these polyfills.

@sgomes
Copy link
Contributor Author

sgomes commented Nov 13, 2024

It appears that polyfilling all possible features is an intentional limitation of how core-js does things for Set. From the babel-plugin-polyfill-corejs3 code:

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 Set is used, all the (non-excluded) features are marked as needed, being listed as dependencies.

Similar problems exist with TypedArray and ArrayBuffer, but these are much more infrequently used, and thus only affect a few packages (blocks and sync).

So I guess the question becomes: should we exclude these Set features in our polyfill exclusions? It would solve the problem for hooks and i18n, and most other packages I've checked.

@sgomes
Copy link
Contributor Author

sgomes commented Nov 15, 2024

@jsnajdr / everyone else : Any thoughts on disabling the following Set feature polyfills as a fix?

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

@sgomes
Copy link
Contributor Author

sgomes commented Nov 22, 2024

@jsnajdr / @swissspidy / @anomiex

I created a new PR with the above proposal: #67230. Hopefully we can continue the discussion there!

@sgomes sgomes linked a pull request Nov 22, 2024 that will close this issue
@jsnajdr
Copy link
Member

jsnajdr commented Nov 22, 2024

It appears that polyfilling all possible features is an intentional limitation of how core-js does things for Set. From the babel-plugin-polyfill-corejs3 code:

Set: define("set/index", SetDependencies),

I'm curious how intentional this really is. The same module in babel-polyfills declares also an InstanceProperties dictionary which has entries like:

toPrecision: define(null, ["es.number.to-precision"]),
toReversed: define("instance/to-reversed", ["es.array.to-reversed"]),

I.e., whenever Babel encounters a .toReversed method call, it includes the es.array.to-reversed polyfill. The object the method is called on is not necessarily an array (it's impossible to know with the basic static analysis that is done here), but it could be, so the plugin ensures that the polyfill is there.

Could we have similar entries also for Set methods? Like:

union: define("instance/union", ["es.set.union.v2"])

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

@zloirock
Copy link

zloirock commented Nov 22, 2024

Could we have similar entries also for Set methods?

Unlike Array methods polyfills, Set methods polyfills have heavy dependencies, like full Set constructor polyfill. .union is a very popular name in userland code. It's tenths of unneeded KB in case of user methods with those names.

Set constructor polyfill dependency without polyfills of methods can overwrite not completely correct implementation and in non-transpiled files users could have a problem with lack of some methods / features that were on this implementation, but not available on this raw Set constructor polyfill.

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.

@jsnajdr
Copy link
Member

jsnajdr commented Nov 22, 2024

Set methods polyfills have heavy dependencies, like full Set constructor polyfill.

Does that mean that if I use some of the ES2024 method polyfills:

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

then some/all of them depend on the es.set.constructor polyfill and will automatically include it?

Looking at the polyfill, I see that it fixes bugs like .has(-0) returning wrong result or .add() not being chainable or set-like objects with size: -1 not behaving well. These bugs are present in very old versions of V8/Chrome. Or are not very interesting in practical code.

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 browserslist config. Anything older is not a concern.

@zloirock
Copy link

zloirock commented Nov 22, 2024

Does that mean that if I use some of the ES2024 method polyfills ... then some/all of them depend on the es.set.constructor polyfill and will automatically include it?

There are implicit dependencies of those modules. That means that the proper Set constructor is required for es.set.union.v2, es.set.constructor it's present in entries like core-js/es/set/union and should be injected by default by preset-env for this. For modern targets like Chrome 109 this dependency is removed by transpiler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants