Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

groupBy: Treat holes as undefined #19

Closed
wants to merge 1 commit into from
Closed

Conversation

jridgewell
Copy link
Member

@jridgewell jridgewell commented Jul 14, 2021

This standardizes on the common ES6+ precedent (though it was ignored by flat and flatMap) where array holes are treated as undefined. In ES5 code, holes were skipped (callbackfn is not called).

This does have the following side-effect:

[1, 2, /* hole */, 4].groupBy(v => {
  return v <= 2;
});
// { true: [1, 2], false: [undefined, 4] }

// Previously, would have been { true: [1, 2], false: [4] }

We've explicitly transformed a hole into an undefined in the false grouping.

@zloirock
Copy link
Contributor

What about holes in .filterReject? I agree that similarity to .filter makes sense, just interesting TC39 opinion.

What about the prototype returned .groupBy object?

@jridgewell
Copy link
Member Author

What about holes in .filterReject? I agree that similarity to .filter makes sense, just interesting TC39 opinion.

filterReject will match the behavior of filter exactly (besides the obvious inverse filtering).

What about the prototype returned .groupBy object?

We're rejecting (🥁 ) the plain object. It'll either be a prototype-less object or a Map, with the possibility of having 2 methods to switch between the two return values. Also, groupBy will be split into a new proposal called "Array Grouping".

@zloirock
Copy link
Contributor

or a Map

Map is a bad option since for this case, we can't use object destructuring of the result:

const { foo = [], bar = [], baz = [] } = array.groupBy(fn);
// vs
const result = array.groupBy(fn);
const foo = result.get('foo') ?? [];
const bar = result.get('bar') ?? [];
const baz = result.get('baz') ?? [];

We already have a recent precedent of null proto object in a similar case with regex NCG:

const { foo, bar, baz } = regex.exec(string).groups;

@ljharb
Copy link
Member

ljharb commented Jul 14, 2021

That's indeed the arguments for a null object; the arguments for Map are that some do want to group by "not just property keys".

@zloirock
Copy link
Contributor

It's not harder to iterate over Object.entries(array.groupBy(fn)) than iterate over map -)

@ljharb
Copy link
Member

ljharb commented Jul 14, 2021

I agree; but if someone wants to group by an object or a boolean or a number or something, an object wouldn't let them do it.

@jridgewell
Copy link
Member Author

I agree with the ergonomic argument for preferring an object, primarily for destructuring (I imagine this will be a popular usage). I think Maps are a more pure form of data structure because they can handle any key and not just property keys, and it's possible that some users will want to take advantage of that.

I think the primary form should be the object return value, with some way to use a Map if necessary for the user.

@zloirock
Copy link
Contributor

That makes sense. So, .groupBy for objects and one new for maps? Or it's planned to rename .groupBy too?

@jridgewell
Copy link
Member Author

So, .groupBy for objects and one new for maps?

That's my expectation. We could either have groupByMap or use an init parameter like Array.p.reduce. But that's something that will need debate.

@jridgewell
Copy link
Member Author

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

Successfully merging this pull request may close these issues.

3 participants