You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
This repository was archived by the owner on Nov 27, 2023. It is now read-only.
Repository navigation
This repository was archived by the owner on Nov 27, 2023. It is now read-only.
.groupByMap means that the result is grouped and placed to the Map and it's how it works by the spec. I don't think that the names collision of .map method and Map constructor should be resolved in the scope of this proposal.
I do understand how it works. I'm saying that someone unfamiliar with the spec who is familiar with .map, .flat, and .flatMap and sees the name might be confused about what .groupByMap does, especially since .groupBy exists along with .map.
Reacted by Chengzhong Wu, François Best and Darien Maillet Valentine
@jridgewell what means To in this case? If that means "pass to Map constructor (or at least somehow convert to Map) the result of .groupBy" - it's incorrect since the result of .groupBy is an object and can't store such keys that are used in the resulting map. That causes more confusion than .groupByMap.
The way it was phrased during discussion was that we're converting "groupings" to a Map, not the object return value from groupBy. Essentially it's like we had "group by to Object" as groupBy and "group by to Map" as groupByToMap. This naming was overwhelmingly preferred by the delegates.
Reacted by Jordan Harband and Darien Maillet Valentine
@zloirock I agree it's awkward, but when "group by" is followed by a noun, it usually means "group by {noun}" literally - "group by name", "group by size", etc. - so group-by-map reads as "group by map" rather than "group by" + "(map variant of this method)" absent prior context.
@bathos so maybe that means that by is unnecessary or unsuitable?
.groupByToMap contains 4 words - and still confusing. Sure, it's not the only method from the JS standard library that contains 4+ words, however, IIRC all the rest of such methods are low level, like .getOwnPropertyNames, but for high-level code, we have one-word name .keys instead of .getOwnPropertyEnumerableNames. .groupByToMap suggests frequent use in the high-level code.
I’m pretty confident grouping and producing a Map will be a fairly uncommon use case
I'm just one developer working in one team and can't say how common or uncommon this is, but as a little piece of anecdotal evidence, I'll mention that we already use a Map-producing groupBy utility function in our codebase (instead of, say, lodash.groupBy) and I'll probably use the Map-producing version of these two Array methods almost exclusively.
The even/odd example is one where the object-producing version shines because the produced keys are well known and simple. But for me, grouping by (semi-)arbitrary user inputted data is a much more compelling and common use case, and there Maps are quite useful.
I don't have strong opinions about the naming, though, just wanted to put that out there. groupByToMap seems fine to me.
filter is not a subset of groupBy. it doesn't do any grouping at all. filter takes a predicate, which answers the question "should i keep this item?". groupBy returns a property key that items are grouped by.
The name of the operation is still "groupBy". There is the default one and there is the one which goes to a Map. Hence "groupBy" and "groupByToMap". This seems fine to me. Removing the "by" would be confusing because it is part of the name of the operation.
I think what's notable in this case is that you can get a result that includes the filter result with the same callback you would pass to filter itself @ljharb, which is not like everything-can-be-reduce, but I agree it's not particularly significant either way.
I know that this issue was closed as if there's nothing left to discuss and everybody else's opinion is unimportant, but groupByToMap() is such a weird sounding function name to me. I'm sure that's something I'm always going to end up forgetting and being unsure about. Would groupToMapBy() maybe be better? It flows a bit better in English.
I agree with the groupByX naming scheme. I think the groupByToMap is a bit wordy, but that's relatively low priority for proposals. The core complaint here is that the original groupByMap naming can be confused with performing a .map + .groupBy exactly like the OP describes. A few committee members had the same misunderstanding, they thought we were implementing a .map + .groupBy instead of a groupBy into a Map. The new naming was extremely favorable during the meeting, because it reduced the confusinion at the expense of an additional word. That's a pretty good trade-off in my opinion.
I think further bikesheding the name is unlikely to produce a better phrase, and I don't think the new name is so terrible that we should delay implementations (the proposal is now Stage 3, the implementation stage).
FWIW I would have called it groupIntoMap() or groupAsMap().
I don't expect these names to be used. But this is a bikeshedding thread so I'm just dropping them here so they can be said to have been considered and rejected.
I actually think @fisker is onto something and Javascript has a problem about how and where type conversion happens. But that's off topic. These are useful functions, whatever they end up being called. *wanders into the sunset muttering something about quotients and equivalence classes...*
Reacted by Denis Pushkarev, fisker Cheung, Darien Maillet Valentine and Dennis Kats
Out of curiosity, was there any consideration for making this particular method a factory method on the Map class?
I imagine something like Map.createGroupBy(array, groupFunc) mostly keeps the desired naming scheme while also indicating the creation of a Map instance.
Perhaps it is less discoverable not as an array method, but if this functionality is only intended to meet a few rare use cases, discoverability doesn't seem that important.
This could also allow it to work with any iterable (object supporting the Symbol.iterator method), rather than just something array-like. So something like: Map.createGroupBy(iterable, groupFunc).
Not sure if I even like this, but throwing this in here that there could be [Symbol.createGroupBy](iterable, groupFunc), with default implementations existing both in Object[Symbol.createGroupBy] and Map[Symbol.createGroupBy] (and why not WeakMap too), and then the Array.prototype.groupBy would take an object with [Symbol.createGroupBy] as optional second argument, defaulting to Object if it's not provided. I.e. the groupByToMap would instead become
if this functionality is only intended to meet a few rare use cases, discoverability doesn't seem that important.
I don't think there is consensus on that aspect: for some, use might be rare and for others, use might be common. I don't think discoverability would be significantly reduced if the returns-a-Map variant were associated with the Map constructor.
This could also allow it to work with any iterable (object supporting the Symbol.iterator method), rather than just something array-like. So something like: Map.createGroupBy(iterable, groupFunc).
I would expect something like Map.createGroupBy(iterable, groupFunc) to only pass the value as an argument to groupFunc.
If the groupFunc callback would be called with the index of the element as an argument then it seems more consistent for groupByToMap to be a method on the Array.prototype (and be transferable to other array-like objects).
I would expect something like Map.createGroupBy(iterable, groupFunc) to only pass the value as an argument to groupFunc.
That's a fair point. I thought the index could still be useful in some situations and easily determined from storing and incrementing a value during iteration, but you're right that it's associated more with ordered data. Either way, it's not the most inconvenient thing to spread the iterable data to an array first and then pass it to the function. In hindsight, maybe that's preferable because it enforces the input type to be the same as (array-like) the type the elements are grouped into.
If the groupFunc callback would be called with the index of the element as an argument then it seems more consistent for groupByToMap to be a method on the Array.prototype (and be transferable to other array-like objects).
As mentioned above, I think providing the index of the element just indicates that the function should act on an array somehow. However, it feels a bit reductionist to conclude that it must then belong on the Array.prototype.
My concern is that it feels a bit strange to have .groupByToMap() return a different, unrelated complex data type (a Map). With the exception of .reduce() which can return anything, no other array method seems to do that. To me, it kind of gives the feeling that the Map is created out of thin air.
On the other hand, with a factory method on the Map class, Map.createGroupBy(array, groupFunc) clearly indicates what kind of "Map" we are talking about and what data is being used and created.
My concern is that it feels a bit strange to have .groupByToMap() return a different, unrelated complex data type (a Map).
That kind of interaction between different collection types is normal in the standard libraries of other languages; for example, java.util.Map#keySet() returns a java.util.Set. I think JavaScript users will quickly get used to it, and I would find it a bit strange to set a precedent that methods of one JavaScript collection type should not return an instance of a different collection type.
Of course, that doesn't mean the returns-a-Map variant must then belong on the Array.prototype. But, given the callback is passed a value, index, object being traversed, and an optional thisArg, then the returns-a-Map variant is substantially consistent with other Array.prototype methods.
Activity
zloirock commentedon Dec 2, 2021
.groupByMapmeans that the result is grouped and placed to theMapand it's how it works by the spec. I don't think that the names collision of.mapmethod andMapconstructor should be resolved in the scope of this proposal.ljharb commentedon Dec 2, 2021
I do understand how it works. I'm saying that someone unfamiliar with the spec who is familiar with
.map,.flat, and.flatMapand sees the name might be confused about what.groupByMapdoes, especially since.groupByexists along with.map.jridgewell commentedon Dec 14, 2021
We're changing from
groupByMaptogroupByToMapzloirock commentedon Dec 14, 2021
@jridgewell what means
Toin this case? If that means "pass toMapconstructor (or at least somehow convert toMap) the result of.groupBy" - it's incorrect since the result of.groupByis an object and can't store such keys that are used in the resulting map. That causes more confusion than.groupByMap.jridgewell commentedon Dec 14, 2021
The way it was phrased during discussion was that we're converting "groupings" to a Map, not the object return value from
groupBy. Essentially it's like we had "group by to Object" asgroupByand "group by to Map" asgroupByToMap. This naming was overwhelmingly preferred by the delegates.zloirock commentedon Dec 15, 2021
In this case, it's unrelated to
flatMapcomparison and this issue. Ok, however, I still think that...ByTo...is terrible for this case.fisker commentedon Dec 16, 2021
Map.fromArray(array, groupFunction)Object.fromArray(array, groupFunction)?
bathos commentedon Dec 16, 2021
@zloirock I agree it's awkward, but when "group by" is followed by a noun, it usually means "group by {noun}" literally - "group by name", "group by size", etc. - so group-by-map reads as "group by map" rather than "group by" + "(map variant of this method)" absent prior context.
zloirock commentedon Dec 16, 2021
@bathos so maybe that means that
byis unnecessary or unsuitable?.groupByToMapcontains 4 words - and still confusing. Sure, it's not the only method from the JS standard library that contains 4+ words, however, IIRC all the rest of such methods are low level, like.getOwnPropertyNames, but for high-level code, we have one-word name.keysinstead of.getOwnPropertyEnumerableNames..groupByToMapsuggests frequent use in the high-level code.ljharb commentedon Dec 16, 2021
I’m pretty confident grouping and producing a Map will be a fairly uncommon use case; it’s just an important one in those rare cases.
The “by” was considered critical to keep by multiple delegates, because “group by” has an existing connotation about what the callback returns.
noppa commentedon Dec 16, 2021
I'm just one developer working in one team and can't say how common or uncommon this is, but as a little piece of anecdotal evidence, I'll mention that we already use a Map-producing
groupByutility function in our codebase (instead of, say, lodash.groupBy) and I'll probably use the Map-producing version of these two Array methods almost exclusively.The even/odd example is one where the object-producing version shines because the produced keys are well known and simple. But for me, grouping by (semi-)arbitrary user inputted data is a much more compelling and common use case, and there Maps are quite useful.
I don't have strong opinions about the naming, though, just wanted to put that out there.
groupByToMapseems fine to me.zloirock commentedon Dec 16, 2021
I have cases for a such method several times for a month.
We can say almost the same about most of
Arraymethods - for example, why.reduceor.filterhaven't such clarifications in their names?3 remaining items
ljharb commentedon Dec 16, 2021
filteris not a subset of groupBy. it doesn't do any grouping at all. filter takes a predicate, which answers the question "should i keep this item?". groupBy returns a property key that items are grouped by.bathos commentedon Dec 16, 2021
Not a true subset, but it seems fair to say filter is a kind of "grouping":
arr.groupByToMap(predicate).get(true)?However filter seems like an argument for keeping "by" because it's not exactly intuitive that you return true to say don't filter.
ljharb commentedon Dec 16, 2021
@bathos that you can do every array method with
reducedoesn't mean they're all a form of reducing :-pbakkot commentedon Dec 16, 2021
The name of the operation is still "groupBy". There is the default one and there is the one which goes to a Map. Hence "groupBy" and "groupByToMap". This seems fine to me. Removing the "by" would be confusing because it is part of the name of the operation.
zloirock commentedon Dec 16, 2021
Ok, I will not impose my opinion further if others are against it.
@ljharb
.filteris a kind of grouping anyway and how it works makes no meaning.bathos commentedon Dec 16, 2021
I think what's notable in this case is that you can get a result that includes the filter result with the same callback you would pass to filter itself @ljharb, which is not like everything-can-be-reduce, but I agree it's not particularly significant either way.
msikma commentedon Dec 18, 2021
I know that this issue was closed as if there's nothing left to discuss and everybody else's opinion is unimportant, but
groupByToMap()is such a weird sounding function name to me. I'm sure that's something I'm always going to end up forgetting and being unsure about. WouldgroupToMapBy()maybe be better? It flows a bit better in English.bakkot commentedon Dec 18, 2021
The name of the operation is "groupBy"; I think any name which doesn't keep that as a substring is not a good choice.
jridgewell commentedon Dec 20, 2021
I agree with the
groupByXnaming scheme. I think thegroupByToMapis a bit wordy, but that's relatively low priority for proposals. The core complaint here is that the originalgroupByMapnaming can be confused with performing a.map+.groupByexactly like the OP describes. A few committee members had the same misunderstanding, they thought we were implementing a.map+.groupByinstead of agroupByinto aMap. The new naming was extremely favorable during the meeting, because it reduced the confusinion at the expense of an additional word. That's a pretty good trade-off in my opinion.I think further bikesheding the name is unlikely to produce a better phrase, and I don't think the new name is so terrible that we should delay implementations (the proposal is now Stage 3, the implementation stage).
fuchsia commentedon Dec 23, 2021
FWIW I would have called it
groupIntoMap()orgroupAsMap().I don't expect these names to be used. But this is a bikeshedding thread so I'm just dropping them here so they can be said to have been considered and rejected.
I actually think @fisker is onto something and Javascript has a problem about how and where type conversion happens. But that's off topic. These are useful functions, whatever they end up being called. *wanders into the sunset muttering something about quotients and equivalence classes...*
denk0403 commentedon Dec 29, 2021
Out of curiosity, was there any consideration for making this particular method a factory method on the Map class?
I imagine something like
Map.createGroupBy(array, groupFunc)mostly keeps the desired naming scheme while also indicating the creation of aMapinstance.Perhaps it is less discoverable not as an array method, but if this functionality is only intended to meet a few rare use cases, discoverability doesn't seem that important.
This could also allow it to work with any iterable (object supporting the
Symbol.iteratormethod), rather than just something array-like. So something like:Map.createGroupBy(iterable, groupFunc).noppa commentedon Dec 29, 2021
Not sure if I even like this, but throwing this in here that there could be
[Symbol.createGroupBy](iterable, groupFunc), with default implementations existing both inObject[Symbol.createGroupBy]andMap[Symbol.createGroupBy](and why notWeakMaptoo), and then theArray.prototype.groupBywould take an object with[Symbol.createGroupBy]as optional second argument, defaulting toObjectif it's not provided. I.e. thegroupByToMapwould instead becomeAndrew-Cottrell commentedon Dec 29, 2021
I don't think there is consensus on that aspect: for some, use might be rare and for others, use might be common. I don't think discoverability would be significantly reduced if the returns-a-Map variant were associated with the
Mapconstructor.I would expect something like
Map.createGroupBy(iterable, groupFunc)to only pass the value as an argument togroupFunc.If the
groupFunccallback would be called with the index of the element as an argument then it seems more consistent forgroupByToMapto be a method on theArray.prototype(and be transferable to other array-like objects).denk0403 commentedon Dec 29, 2021
That's a fair point. I thought the index could still be useful in some situations and easily determined from storing and incrementing a value during iteration, but you're right that it's associated more with ordered data. Either way, it's not the most inconvenient thing to spread the iterable data to an array first and then pass it to the function. In hindsight, maybe that's preferable because it enforces the input type to be the same as (array-like) the type the elements are grouped into.
As mentioned above, I think providing the index of the element just indicates that the function should act on an array somehow. However, it feels a bit reductionist to conclude that it must then belong on the
Array.prototype.My concern is that it feels a bit strange to have
.groupByToMap()return a different, unrelated complex data type (aMap). With the exception of.reduce()which can return anything, no other array method seems to do that. To me, it kind of gives the feeling that theMapis created out of thin air.On the other hand, with a factory method on the
Mapclass,Map.createGroupBy(array, groupFunc)clearly indicates what kind of "Map" we are talking about and what data is being used and created.Andrew-Cottrell commentedon Dec 30, 2021
That kind of interaction between different collection types is normal in the standard libraries of other languages; for example,
java.util.Map#keySet()returns ajava.util.Set. I think JavaScript users will quickly get used to it, and I would find it a bit strange to set a precedent that methods of one JavaScript collection type should not return an instance of a different collection type.Of course, that doesn't mean the returns-a-Map variant must then belong on the
Array.prototype. But, given the callback is passed a value, index, object being traversed, and an optionalthisArg, then the returns-a-Map variant is substantially consistent with otherArray.prototypemethods.