Web Compatibility Issue: Sugar versions v0.9 through v1.3.9 (inclusive)
codehag opened this issue · 27 comments
Hi,
Unfortunately, we have found a web compat issue with the sugar.js library. Details here: https://bugzilla.mozilla.org/show_bug.cgi?id=1750812#c5
I didn't check all possible versions for breakage. For sure it is broken on 1.1.2, it looks (from reading the code) that it is fixed by 1.5.
I will add a pref on nightly to allow our users to keep working, and see how broad this issue is. Maybe if it is just a few consumers we can get around it...
There is a note in Sugar/CAUTION.md that indicates the override fix may be present since v1.4.0.
Version 1.4.0 improves future-compatibility by ensuring that browser updates do not cause breakages going forward.
I think the fix was implemented in andrewplummer/Sugar@bc189b4 by setting override to true when calling function extend in "lib/core.js".
It may be easier for consumers to update to v1.4.0 rather than v1.5.0, however there are breaking changes in either case.
fantastic, thank you @Andrew-Cottrell -- It will depend on how wide spread this is ... but hopefully we can work with people to update and manage to keep the name.
Another option: Can we accept string as the first argument? It's useful to simplify code
[
{ name: 'a', sex: 'F' },
{ name: 'b', sex: 'M' },
{ name: 'c', sex: 'F' },
].groupBy('sex')// https://lodash.com/docs/4.17.15#groupBy
['one', 'two', 'three'].groupBy('length');That alone wouldn't fix the compatibility issue, the sugarjs version also takes a second argument, a function that gets called for each group.
We have another failure: webcompat/web-bugs#98458
Is this a case where dropping the preposition would help, or is group also known to have collision issues?
It doesn't seem like group alone loses much clarity. It might even be clearer for some people (thinking about stuff @hax said about the value of simple names for ESL folks) and could address the use of ByTo that a number of folks found unnatural. (Apologies if this was already considered, I didn't see it when I searched tho apart from being mentioned in passing in the ByTo thread.)
We will have to check if there are collisions on other names -- adding new prototype methods to array often runs into this. But, yes, I am starting to think we may need a rename if we find more sites are failing.
In the ideal case, we will be able to resolve them like we did with at(). In that case, after the name change we still had a naming conflict, but it was a single app specific library that was failing. I am afraid we are not so fortunate here. sugar.js is calling groupBy during initialization, so it will happen for every importer of this library regardless of if they use groupBy in their code or not.
Not sure if it's time to start throwing out other name suggestions yet, but one idea is groupedBy().
Subscribing to keep up with resolution attempts, and holding off shipping in V8 in the meantime.
“old versions of sugar” seems like a feasible-but-large evangelism task; I’m still hopeful we’ll be able to keep the existing names.
groupedBy isn't terrible, but it's unfortunate we're having this clash. Given this error will trigger on every site that uses older Sugar, I'm not sure we'll be able to get everyone to update. How do we even query for sites that use it?
More name ideas:
.groupToObject().groupToMap()
Querying the HTTP Archive, there appear to be around 1100 origins using 2000 payloads that contain SugarMethods:
SELECT
page,
COUNT(*) AS volume
FROM (
SELECT * FROM `httparchive.response_bodies.2022_01_01_desktop`
UNION ALL SELECT * FROM `httparchive.response_bodies.2022_01_01_mobile`
)
WHERE body LIKE "%SugarMethods%"
GROUP BY 1
ORDER BY 2 DESCfirst 100 origins
@jridgewell does that indicate sugarjs in general, or these specific problematic versions?
I don't know how to filter for the problematic version. This is looking for SugarMethods explicitly.
There's a _third-parties table that could be utilized in HTTP Archive example. I'll put up a PR for adding Sugar to https://github.com/johnmichel/Library-Detector-for-Chrome/ . Luckily there's a version field in the library, so one should be able to make a query with that on the next month's dataset.
EDIT: Seem's Sugar.VERSION was removed for a short time and then added again, and that window.Sugar is sometimes not defined on a page, so not sure how useful this information will be in practice.
EDIT2: That table is gone in HTTP Archive, but the same results can be grokked from the mobile Lighthouse results table. At this rate, adding a grep for Sugar v{...} in the above query is probably the better route.
Hi, I help maintain HTTP Archive and I was curious to see if I can help after @connorjclark pinged me about _third-parties.
At this rate, adding a grep for
Sugar v{...}in the above query is probably the better route.
I ran with @connorjclark's suggestion and implemented it using the slightly more efficient almanac dataset from July 2021. If that's too far in the past, LMK and I can rerun it. But I think the results will be similar.
One side note is that we're relying mostly on Wappalyzer now for technology detections, so @connorjclark if you did want to add support for Sugar, I'd recommend patching that as well.
SELECT
client,
page,
url,
REGEXP_EXTRACT(body, r'\bSugar v(\d[\d.]+)') AS sugar_version
FROM
`httparchive.almanac.summary_response_bodies`
WHERE
date = '2021-07-01' AND
type IN ('html', 'script') AND
REGEXP_CONTAINS(body, r'\bSugar v(\d[\d.]+)')See the results in Google Sheets
I counted the number of unique pages for each combination of version and client (desktop or mobile):
| sugar_version | desktop | mobile |
|---|---|---|
| 1.1 | 1 | 1 |
| 1.5.0 | 10 | 8 |
| 2.0.0 | 3 | 2 |
| 2.0.2 | 15 | 15 |
| 2.0.4 | 14 | 16 |
| 2.0.6 | 36 | 28 |
| Grand Total | 79 | 70 |
So there are no websites (that match the version regex pattern) that use a version of Sugar in the range 1.1.2-1.4. And there are only 90 of them that use it at all (79 desktop and 70 mobile, with most sites in both).
We can also see how popular these 90 sites are by looking up their coarse ranking data:
| rank | pages |
|---|---|
| 100,000 | 4 |
| 1,000,000 | 10 |
| 10,000,000 | 76 |
Some websites are in the top 100k most popular according to CrUX, but most are in the longest part of the tail in the top 10M (there are only 8M sites in the dataset). The 4 in the top 100k are:
https://www.moneyguru.co.th/
https://www.ctv.co.jp/
https://www.marinelayer.com/
https://my77webshop.hu/
And they all happen to be on v2.0.4.
Hope this was somewhat helpful. Let me know if there's anything else I can do. And generally speaking, I'm super interested to get more involved with web standards groups and help you make more data-driven decisions using the HTTP Archive dataset. Feel free to ping me any time!
Hi @rviscomi and thanks for the analysis!
I have two changes that I'd like to make to the queries. First, it's very common to minify JS for production, and basically all of them will remove comments unless a @license clauses is added into the comment (which Sugar doesn't use in the version range we care about). Second, the version range we're interested in (v0.9 through v1.3.9) uses "Sugar Library v\d.\d" as it's comment (and actually v1.3.9 uses "vedge" 🤦♂️), with only v1.5+ using "Sugar v\d.\d" (we can use presence of "Sugar v" to exclude).
So something like this should get us better results:
SELECT
client,
page,
url,
REGEXP_EXTRACT(body, r'\bSugar Library v(\d[\d.]+)') AS sugar_version
# ^ May be null if the comment is not found, as it may have been stripped
FROM
`httparchive.almanac.summary_response_bodies`
WHERE
date = '2021-07-01' AND
type IN ('html', 'script') AND
body LIKE "%SugarMethods%" AND
NOT REGEXP_CONTAINS(body, r"Sugar v\d\.\d")(Sorry, not at my corp computer so I don't have the ability to run queries)
Thanks @jridgewell that explains why the results seem to start at v1.5.0
Here's the query I ran, trying to include all versions:
SELECT
client,
rank,
page,
url,
REGEXP_EXTRACT(body, r'\bSugar(?: Library)? v(\d[\d.]+)') AS sugar_version
FROM
`httparchive.almanac.summary_response_bodies`
WHERE
date = '2021-07-01' AND
type IN ('html', 'script') AND
body LIKE "%SugarMethods%"Results added to a new tab in the Sheet
| sugar_version | desktop | mobile |
|---|---|---|
| null | 384 | 354 |
| 1.2 | 1 | 1 |
| 1.2.1 | 1 | 2 |
| 1.2.2 | 4 | 4 |
| 1.2.4 | 106 | 194 |
| 1.2.5 | 4 | 2 |
| 1.3 | 5 | 5 |
| 1.3.4 | 2 | 2 |
| 1.3.5 | 1 | 1 |
| 1.3.6 | 2 | 2 |
| 1.3.7 | 2 | 4 |
| 1.3.8 | 2 | 2 |
| 1.3.9 | 30 | 32 |
| 1.4.0 | 2 | 1 |
| 1.4.1 | 567 | 518 |
| 1.5.0 | 5 | 5 |
| Grand Total | 1,118 | 1,129 |
Several hundred pages contain the SugarMethods signal but not a parseable version number. That signal also seems to have stopped being used after v1.5.0.
And here's the ranking breakdown:
| rank | COUNTUNIQUE of page |
|---|---|
| 10,000 | 2 |
| 100,000 | 21 |
| 1,000,000 | 258 |
| 10,000,000 | 980 |
| Grand Total | 1,261 |
So using this new method, more like 1,300 websites use Sugar (accounting for the ~90 or so websites on v1.5+).
Here's the most popular 23 websites:
rank | sugar_version | page
-- | -- | --
10,000 | 1.4.0 | https://jisho.org/
10,000 | 1.4.1 | https://app.roll20.net/
100,000 | null | https://app.jazz.co/
100,000 | null | https://app.theo.blue/
100,000 | null | https://my.echecks.com/
100,000 | null | https://paidviewpoint.com/
100,000 | null | https://pm.eyefinity.com/
100,000 | null | https://pubg.starladder.com/
100,000 | null | https://www.gcmgames.com.br/
100,000 | null | https://www.maximustecidos.com.br/
100,000 | null | https://www.orsola.com.br/
100,000 | 1.3.4 | https://microminimus.com/
100,000 | 1.4.1 | https://educacao.rs.gov.br/
100,000 | 1.4.1 | https://m.lauritz.com/
100,000 | 1.4.1 | https://ranking.goo.ne.jp/
100,000 | 1.4.1 | https://saude.rs.gov.br/
100,000 | 1.4.1 | https://school.eb.com/
100,000 | 1.4.1 | https://servicos.detran.rs.gov.br/
100,000 | 1.4.1 | https://www.detran.rs.gov.br/
100,000 | 1.4.1 | https://www.lauritz.com/
100,000 | 1.4.1 | https://www.prealpina.it/
100,000 | 1.4.1 | https://www.rs.gov.br/
100,000 | 1.4.1 | https://www.serengeti-park.de/
https://microminimus.com/ is the only "popular" one known to use a problematic version.
Filtering to only v0.9-v.1.3.9:
| sugar_version | COUNTUNIQUE of page |
|---|---|
| 1.2 | 1 |
| 1.2.1 | 2 |
| 1.2.2 | 4 |
| 1.2.4 | 197 |
| 1.2.5 | 4 |
| 1.3 | 6 |
| 1.3.4 | 2 |
| 1.3.5 | 1 |
| 1.3.6 | 2 |
| 1.3.7 | 3 |
| 1.3.8 | 2 |
| 1.3.9 | 31 |
| Grand Total | 254 |
254 websites in total are known to use a problematic version. Even assuming 100% of the unknown versions are within the problematic range, that's still only a total of 660 websites.
Pagerduty has now updated to a newer version of sugar.js -- do we have a plan on how to go through contacting the rest of the potentially impacted sites?
lol heads up for others who might visit the site described as "popular" above out of curiosity: it's probably not something you wanna visit at work
Hi, please forgive me for resurrecting a closed thread. As someone who has been interested in Sugar for a long time but never implemented due to the age old practice of "Never monkey patch primitives", I think its a huge bummer that we are moving forward with static methods instead of instance methods because only 660 sites chose to do that.
I remember looking at Sugar many years ago and there were clear warnings about the potential negative ramifications of monkey patching primitives. The Sugar library/author were aware of this potential and any users were aware of this potential.
The ergonomics of myArray.groupBy() are superior to static methods IMO. And it's a shame that the web will suffer for the sake of a few brave users who knowingly chose to use a "dangerous" library. I don't mean to belittle Sugar. I think its super cool and actually have a repo of my own that does some similar things inspired by Sugar. And I know Sugar changed its approach in later versions.
If this proposal is too far along for a change, I understand and you can disregard this comment. But I wanted to throw in my .02.
Thanks.
@DaddyWarbucks browsers are unwilling to cause that breakage, and have indicated that they are very unwilling to consider any prototype methods that may collide in the future (especially on Arrays), so there's really nothing to be done.
Certainly if you can convince all of those sites to upgrade, a change could be made, but that seems untenable.
Thanks for the response @ljharb. I appreciate the committee and browsers for not breaking the web. But, if this Sugar issue is the primary reason for using static methods, I politely encourage the committee to take on a less strict opinion.
groupBy is such a common use and is one of the most useful/important utility proposals I've seen added into the language. It's basically already ubiquitous. But, groupBy also loses one of its most important attributes that it can gain over its predecessors when used as a static method, which is chainability. I am sure anyone reading this understands that but I think it can't be understated. With all of the context around SugarJS, and how the author and users knew this exact thing could happen, I think we are giving these 660 sites too much weight in the opposition.
The Sugar.js repo is unmaintained, or at least inactive. I haven't seen @andrewplummer mentioned anywhere. I also searched for Discord, Slack, Twitter but didn't find much. Perhaps they could help us out by updating the README, etc.
Given the importance and ergonomics of groupBy and the low number of affected sites I actually feel like we are doing the language a disservice by using static methods.
Thanks for any consideration.
@DaddyWarbucks Here's a good writeup of why "don't break the web" is sacrosanct.
I know it's frustrating as a developer to have to use the slightly more annoying form. But our first duty is to the users of the web. And it's much more frustrating if one day everyone in Brazil wakes up and finds that their browser updated and now random government websites aren't working. Web browsers are not going to prioritize JS developers being able to use groupBy in a chainable way over citizens of Brazil being able to use their government's websites. It's just not going to happen.
@bakkot Thanks for the response. I remember SmoohGate, but I was hoping this smaller footprint might be acceptable.
I've got one last suggestion/question, and I'll go ahead and make it here instead of opening another new issue about a rename. I don't believe I have seen toGrouped suggested. I have seen in other issues the desire to keep groupBy due to its familiarity with its predecessors. Does the recent addition of toSorted, toReversed, and friends make the name toGrouped more attractive?
Thanks.
Browsers have already indicated they refuse to try anything else on Array.prototype for this proposal, after group and groupBy both failed. This proposal's shape is almost certainly not going to change further.