Implement ES2024 groupBy
#1581
Implement ES2024 groupBy
#1581
Conversation
camnwalter
force-pushed
the
array-grouping
branch
2 times, most recently
from
767889a to
3c87078
Compare
| } | ||
|
|
||
| // LinkedHashMap used to preserve key creation order | ||
| Map<Object, List<Object>> groups = new LinkedHashMap<>(); |
There was a problem hiding this comment.
I'm curious if the spec requires that we preserve key creation order, or if we're just being nice. What do other engines do? I don't have a strong opinion but i'd generally rather stick with a regular HashMap unless the spec requires otherwise.
There was a problem hiding this comment.
The spec says it is a list of records and appends a new group each time. I just used a map as it seemed a little easier to implement that way. Haven't researched other engines yet but I'd assume they follow it.
| Map<Object, List<Object>> groups = new LinkedHashMap<>(); | ||
| final Object iterator = ScriptRuntime.callIterator(items, cx, scope); | ||
| try (IteratorLikeIterable it = new IteratorLikeIterable(cx, scope, iterator)) { | ||
| double k = 0; |
There was a problem hiding this comment.
Note to self for the future -- I wonder if there's a way that some day we can take this code and others like it and optimize it to use integers except in the rare case where we have more than 2^31 elements in an array. (And BTW I'm not so sure that we can handle more than 2^31 elements in an array anyway...)
rhino/src/main/java/org/mozilla/javascript/AbstractEcmaObjectOperations.java
Outdated
Show resolved
Hide resolved
camnwalter
force-pushed
the
array-grouping
branch
from
3c87078 to
f3567a0
Compare
| for (Map.Entry<Object, List<Object>> entry : groups.entrySet()) { | ||
| Scriptable elements = cx.newArray(scope, entry.getValue().toArray()); | ||
|
|
||
| ScriptableObject desc = (ScriptableObject) cx.newObject(scope); |
There was a problem hiding this comment.
One last nit -- you can (and should please) replace lines 712-717 with:
obj.defineProperty(entry.getKey(), elements, ScriptableObject.PERMANENT);
This is not only less code but it's more efficient, because we're not creating a new object and messing around in a hash table to figure out what attributes to set.
There was a problem hiding this comment.
The only issue with that is that the entry's key is an Object, which defineProperty doesn't allow for the first parameter. That's the main reason I did that workaround.
Edit: I just made it an if else according to if it is a symbol or not. We probably should have a dedicated function just for this, described in https://tc39.es/ecma262/#sec-topropertykey
There was a problem hiding this comment.
OK -- good point -- let's merge this!
| * @param items | ||
| * @param callback | ||
| * @param keyCoercion | ||
| * @see <a href="https://tc39.es/ecma262/#sec-groupby"></a> |
There was a problem hiding this comment.
Sorry, just seeing this now: the link to the spec doesn't reference a specific version of the spec, which it should 😐
| @@ -232,4 +242,65 @@ static void put(Context cx, Scriptable o, int p, Object v, boolean isThrow) { | |||
| base.put(p, o, v); | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Implement the ECMAScript abstract operation "GroupBy" defined in section 7.3.35 of ECMA262. | |||
There was a problem hiding this comment.
And no need to reference the section number here, as they change from version to version
|
I'll make a separate pr addressing those couple changes if you want |
|
Thnx, sry, should've spotted be it earlier |
Resolves #1545