Skip to content
This repository has been archived by the owner on Nov 27, 2023. It is now read-only.

WebCompat regressions with group #44

Closed
codehag opened this issue Nov 7, 2022 · 37 comments
Closed

WebCompat regressions with group #44

codehag opened this issue Nov 7, 2022 · 37 comments

Comments

@codehag
Copy link

codehag commented Nov 7, 2022

Hey all,

We have another regression, this time with group. https://bugzilla.mozilla.org/show_bug.cgi?id=1791415#c14

It didn't get surfaced quite as quickly as the last one, as it was harder to catch. This may be true of other sites.

@codehag
Copy link
Author

codehag commented Nov 7, 2022

I think if this is unique to last pass, we may be able to have them update. However, given how hard it was to catch, this might be failing silently elsewhere? I wonder if we need to have a usage counter for this before moving forward with shipping?

@ljharb
Copy link
Member

ljharb commented Nov 7, 2022

Any idea what code on LastPass is breaking with this?

@ljharb ljharb mentioned this issue Nov 7, 2022
10 tasks
@syg
Copy link

syg commented Nov 7, 2022

@codehag Was https://bugzilla.mozilla.org/show_bug.cgi?id=1791415#c14 actually confirmed to fix the issue?

@codehag
Copy link
Author

codehag commented Nov 7, 2022

Looks like not yet, still waiting on the full picture.

@codehag
Copy link
Author

codehag commented Nov 7, 2022

We have a second regression that appears to be related to this proposal: https://bugzilla.mozilla.org/show_bug.cgi?id=1799522 / webcompat/web-bugs#112923

@codehag
Copy link
Author

codehag commented Nov 7, 2022

And a third, unfortunately: webcompat/web-bugs#112552 -- also tested and confirmed on chrome canary.

@codehag codehag changed the title WebCompat regression: LastPass on group WebCompat regressions with group Nov 7, 2022
@syg
Copy link

syg commented Nov 7, 2022

In light of these discovered incompatibilities Chrome will no longer be shipping this proposal in 108.

@neclimdul
Copy link

I haven't tested the proposed removal but Array.prototype.group = null; in the console did fix my LastPass extension. Several traces are also in the bugzilla issue that point to the group property/method.

@neclimdul
Copy link

Since LastPass isn't the only regression I doubt it matters to much but some additional information. I regularly use LastPass with the same account in Chrome unstable and haven't run into the same problem. I can confirm the other regressions from this issue on that browser so the so probably a frustrating variation between the Chrome and Firefox extension provided by LastPass.

@bricss
Copy link

bricss commented Nov 14, 2022

Maybe, it might be possible to rename group() and groupToMap() into assort() and assortToMap() and ship 🚢 it asap 🤔

@ljharb
Copy link
Member

ljharb commented Nov 15, 2022

I’m not familiar with any such word “assort” - that may not be an intuitive option.

@bricss
Copy link

bricss commented Nov 15, 2022

That's what I found in dictionary for group synonyms 😀 sounds legit, imo 🙄

@bricss
Copy link

bricss commented Nov 15, 2022

As an option might be aggregate, batch or pluck 🙂

@noppa
Copy link

noppa commented Nov 15, 2022

Any name that doesn't have "group" in it is going to hurt discoverability. If we are throwing around rename suggestions already, my choice would be groupToObject. Or moving to static functions Object.groupBy and Map.groupBy and hoping that pipelines will some day solve the ergonomics issue with that.

@zloirock
Copy link
Contributor

I hope that it will be possible to save the current name and fix it in affected places. Otherwise, Array.prototype.groupToObject LGTM.

@michaelficarra
Copy link
Member

I guess this is where we're bikeshedding backup names. My vote is for partition.

@bakkot
Copy link
Contributor

bakkot commented Nov 28, 2022

I am categorically against partition. That is a different thing.

@ljharb
Copy link
Member

ljharb commented Nov 28, 2022

@bakkot what is "partition", then?

@jridgewell
Copy link
Member

partition returns a tuple of 2 arrays: const [evens, odds] = numbers.partition(i => i % 2 === 0). It does not extend to arbitrary keying, only truthy and falsey.

@michaelficarra
Copy link
Member

partition is just groupBy special-cased to true/false. { true: [], false: [] } and [[], []] are basically the same thing. I don't think that someone who already knows partition is going to be that bothered by the difference in shape between those two. The important part is that they are conceptually very close.

@jsejcksn
Copy link

jsejcksn commented Nov 30, 2022

I'm sure this has been covered elsewhere (and please feel free to point me to prior recorded discussions — I'd love to get historical context!), but why do we allow these kinds of compat issues to prevent progress?

Specifically, when there's a possibility of breakage which might be caused by mutation of built-ins or the global namespace, etc. (things that are provided by the runtime environment — not owned by the application code): isn't breakage always a risk to each party that chooses to do that and fully its responsibility in every case?

@ljharb
Copy link
Member

ljharb commented Nov 30, 2022

@jsejcksn no, because there's users and devs, and the users are the ones who get hurt, and only the devs can fix it. Breaking users through no fault of their own or that of the devs is not an acceptable tradeoff for any language feature.

@bakkot
Copy link
Contributor

bakkot commented Nov 30, 2022

@jsejcksn Here's a good writeup of why "don't break the web" is sacrosanct.

@jsejcksn
Copy link

jsejcksn commented Nov 30, 2022

@bakkot Thanks for the link!

@ljharb I share that empathy for users — even as developers, we are all users and none of us wants a bad experience. However, isn't this actually a case of (inconsiderate) devs who are breaking the web? Nothing that's been proposed would cause a break in backward compatibility of existing standards/APIs. There will always be devs (or product owners, etc. — the "code-owning decision makers") who eschew social responsibility and always users who suffer by those devs' choices. Where do we draw the line?

@ljharb
Copy link
Member

ljharb commented Nov 30, 2022

@jsejcksn we draw the line, as always, at "no breakage". Slippery slope arguments don't change that.

@jsejcksn
Copy link

jsejcksn commented Nov 30, 2022

@jsejcksn we draw the line, as always, at "no breakage". Slippery slope arguments don't change that.

What I meant by "Where do we draw the line?" was: Every built-in is being mutated by some site out there, and its users would be affected by any new name chosen for this API (no matter the name). Maybe it's just one student's sandbox app — or one community forum site for a very small group.

What is the objective measure that we've determined to be the threshold? Is it one site? Is it 100? Is it 1% according to some index created by a single crawler? How do we define what "no breakage" means?

@ljharb
Copy link
Member

ljharb commented Nov 30, 2022

I don't think there is any objective measure - I think each browser just makes a judgement call.

@codehag
Copy link
Author

codehag commented Nov 30, 2022

We are getting a bit off topic, I think @bakkot gave a good answer to the question. Please feel free to ask for more clarification in the matrix channel for tc39-general!

@bricss
Copy link

bricss commented Jan 11, 2023

Any hope, any updates? 🤔

@bakkot
Copy link
Contributor

bakkot commented Jan 11, 2023

@bricss Since group didn't work out, we'll need to find another option. I believe the current best hope is switching to static methods on Object and Map, but the committee will need to discuss that before browsers can ship it.

@lilnasy
Copy link

lilnasy commented May 15, 2023

Safari 16.4 shipped Array#group. At the time, this broke IBM cloud login (webcompat/web-bugs#112923), but not altron.ch (webcompat/web-bugs#112552). IBM's usage involved treating an array as a query param object, one of the keys of which was group. They seem to have corrected this misuse since then.

@bricss
Copy link

bricss commented May 15, 2023

Shall we also break things and move fast ⏩ forward? 😀

@o-t-w
Copy link

o-t-w commented May 17, 2023

It's already stable Safari and all the other naming suggestions are terrible.

@bathos
Copy link

bathos commented May 17, 2023

I guess this is where we're bikeshedding backup names.

  • bucket
  • keyBy
  • toGroups
  • switch kidding

I’m not familiar with any such word “assort” - that may not be an intuitive option.

It’s a pity this is too obscure because I think it'd be a semantic upgrade from “group” if not for that — it implies grouping on the basis of some condition, not just arbitrary grouping.

@karlcow
Copy link

karlcow commented May 17, 2023

etc. all names need to be checked against what exist out there.

@ljharb
Copy link
Member

ljharb commented May 17, 2023

I don't think we need additional name suggestions right now; we're right in the middle of the TC39 plenary and we'll be discussing it further tomorrow.

@ljharb
Copy link
Member

ljharb commented May 18, 2023

The proposal has been demoted to stage 2, with the expectation that I will be requesting stage 3 for it in July with static methods (#47).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

16 participants