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

Supporing Records/Tuples and Maps #36

Closed
jridgewell opened this issue Jan 11, 2022 · 4 comments
Closed

Supporing Records/Tuples and Maps #36

jridgewell opened this issue Jan 11, 2022 · 4 comments

Comments

@jridgewell
Copy link
Member

tc39/proposal-record-tuple#275 brings up how groupBy could be supported on Tuples. Two open questions are:

  1. Should the groups be Array or Tuples?
  2. Should the output be an Object or a Record?

I think we can sidestep the first question for the time being (Array.p.groupBy groups into Arrays, and it wouldn't be weird for either choice to be chosen for Tuples).

But, I think we should address the second question now. We currently have Array.p.groupByToMap, and I don't think adding a new groupByToX is a great solution for new return types. I think it'll be a bit weird to have:

  • Array.p.groupBy
  • Array.p.groupByToMap
  • Array.p.groupByToRecord (this would probably be bad, because how likely is it that the array is all primitives?)
  • Tuple.p.groupBy
  • Tuple.p.groupByToMap
  • Tuple.p.groupByToRecord

@denk0403 suggests in #30 (comment) that we extend to new types by providing a static method on the constructor. Eg,

  • Array.p.groupBy(callback) returns a prototype-less Object
  • Tuple.p.groupBy(callback) returns a prototype-less Object
  • Map.groupBy(iterable, callback) returns a Map
  • Record.groupBy(iterable, callback) returns a Record
@ljharb
Copy link
Member

ljharb commented Jan 11, 2022

If this is a change we want to make, now that it's stage 3 we'll need to notify implementors quickly to ensure they don't ship groupByToMap in the meantime. Hopefully this can be added to this month's agenda.

@bakkot
Copy link
Contributor

bakkot commented Jan 11, 2022

There are lots of methods in the language which produce string-keyed collections. Should we generally want to add record-producing versions of all of them?

I think no. We should instead make it easy to get a Record out of an object, so you can convert to the type you want if you want something non-default.

Map really is a different case, because you will very often want to group by something which is not a string. So it's not sufficient to convert after the fact.

I don't think we should hide the strictly more useful (if slightly less ergonomic) version of groupBy as a static method on Map. If we are to have only a single version on Array.prototype, the correct one to choose is groupByToMap. But I agree there are compelling reasons to want the slightly more ergonomic object-returning form to be on Array.prototype - so we should have both versions be there.

And unless we add a new kind of collection which has a different kind of coercion applied to its keys (which I very much hope we don't do), Map and Object are sufficient to cover all the kinds of grouping you might want to do. If you want a different type of collection, you can choose Map or Object as appropriate and then convert. So it's appropriate for those two, and only those two to be blessed on Array.prototype.


To put it more briefly: no, I don't think we should add Array.prototype.groupByToRecord or any equivalent, for the same reason I don't think we should add a Array.prototype.groupByToWeakMap. It is not practical to provide methods which directly produce every type of interest. Rather, we should pick the appropriate type to return for each API and make it straightforward to convert between types.

@denk0403
Copy link

denk0403 commented Jan 11, 2022

I agree, Record.groupBy or any other Array.prototype.groupByToX feels redundant if there’s eventually an easy way to convert between Objects and Records or other collection types.

I personally don’t have any strong opinions about the other methods' return types, but to me, it makes sense for Tuple.prototype.groupBy to return an object (with string or symbol keys), and tuples for values. As mentioned in tc39/proposal-record-tuple#275 (comment), I think there is somewhat of an expectation that groupBy returns "sublists". And as the comment also mentions, it makes it trivial to convert the result to a Record since every inner value is already a primitive.

Tuple's Map version of groupBy obviously can't provide that same level of convenience for conversion, therefore I don't think it matters as much whether it returns a Map<K,Array> or a Map<K,Tuple>. However, perhaps one thing to consider is this: if the Map version of the method for tuples returns a Map<K,Array>, then its implementation is identical to the version on arrays. And if so, maybe then it's cleaner to place the implementation more centrally as a static method on Map.

On the other hand, if it should return a Map<K,Tuple>, then having distinct Array.prototype.groupByToMap and Tuple.prototype.groupByToMap methods makes more sense.


Building on the Map.groupBy idea further though, the static method could somewhat mirror the way Array.from works. Specifically, it would have the following signature:

groupBy(arrayLike: Iterable | ArrayLike, groupfn: (value, index) => any, thisArg?: any)

where:
arrayLike - An array-like or iterable object to convert to an array.
groupFn - grouping function to call on every element of the array.
thisArg Optional - Value to use as this when executing groupFn.

And like Array.from, subclasses of Map can inherit the method and return return instances of the subclass, not Map.

@ljharb
Copy link
Member

ljharb commented May 18, 2023

We support Map, and the Record & Tuple proposal can include a static groupBy method for consistency if it makes sense.

@ljharb ljharb closed this as completed May 18, 2023
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

4 participants