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

Add Array.prototype.groupBy[toMap] to JS lib definitions. #47171

Closed
Pokute opened this issue Dec 16, 2021 · 26 comments · Fixed by #56805
Closed

Add Array.prototype.groupBy[toMap] to JS lib definitions. #47171

Pokute opened this issue Dec 16, 2021 · 26 comments · Fixed by #56805
Labels
Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Effort: Casual Good issue if you're already used to contributing to the codebase. Harder than "good first issue". Help Wanted You can do this
Milestone

Comments

@Pokute
Copy link

Pokute commented Dec 16, 2021

The proposal for Array.prototype.groupBy and Array.prototype.groupByToMap reached stage 3 recently.

Search terms: proposal-array-grouping array groupBy ernest

lib Update Request

With advancement into stage 3, it's time for JS engines, transpilers and toolings to start implementing support for new features and provide feedback for the proposal's advancement into stage 4.

Configuration Check

This is so new that it needs to go into ESNext target.

Missing / Incorrect Definition

Missing Array.prototype.groupBy and Array.prototype.groupByToMap.

Sample Code

const array = [1, 2, 3, 4, 5];

// groupBy groups items by arbitrary key.
// In this case, we're grouping by even/odd keys
array.groupBy((num, index, array) => {
  return num % 2 === 0 ? 'even': 'odd';
});

// =>  { odd: [1, 3, 5], even: [2, 4] }

// groupByToMap returns items in a Map, and is useful for grouping using
// an object key.
const odd  = { odd: true };
const even = { even: true };
array.groupByToMap((num, index, array) => {
  return num % 2 === 0 ? even: odd;
});

// =>  Map { {odd: true}: [1, 3, 5], {even: true}: [2, 4] }

Sample code from https://github.com/es-shims/Array.prototype.groupBy

Documentation Link

Proposal: proposal-array-grouping

Shim implementation that can the code can be tested against: es-shims/Array.prototype.groupBy

@orta orta added Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Effort: Casual Good issue if you're already used to contributing to the codebase. Harder than "good first issue". labels Dec 16, 2021
@orta
Copy link
Contributor

orta commented Dec 16, 2021

Open for someone to take a look. I'm not quite sure off the bat what this should look like FWIW.

I think we probably need to make the same assumptions as we do with Object.entries where we can't use keyof #38520

@orta
Copy link
Contributor

orta commented Dec 16, 2021

I asked, and we generally think this might be ok to have the more accurate typing accurately reflecting the source object. Will need a bit of testing out when there's a PR

@DanielRosenwasser
Copy link
Member

Yeah, I think we could think of this as closer to fromEntries; however, fromEntries doesn't try to preserve keys at all (and I don't think it even (cleanly) could).

@DanielRosenwasser
Copy link
Member

I feel like there is something close you can get with reversed mapped types - we discussed this in a recent design meeting about correlated unions.

#47109 for some description of this.

@Hanaffi
Copy link

Hanaffi commented Jan 3, 2022

@orta @DanielRosenwasser Can you assign this to me?

@orta
Copy link
Contributor

orta commented Jan 3, 2022

We don't assign issues to external folks, you're welcome to make the PR as long as there isn't an existing one (which you would see in this issue's timeline above) - so go for it

@Hanaffi
Copy link

Hanaffi commented Jan 3, 2022

@orta Thanks. Its my first time to contribute to TypeScript. So can you give me some tips and how to get started on this issue?
I feel lost in the codebase :D

@yessGlory17
Copy link

@DanielRosenwasser I want to work on this issue. It's my first time to contribute to Typescript. Is it possible for you to give me some information?

@SamB
Copy link

SamB commented Feb 24, 2023

@DanielRosenwasser

Yeah, I think we could think of this as closer to fromEntries; however, fromEntries doesn't try to preserve keys at all (and I don't think it even (cleanly) could).

You mean there would be a problem with the following overload, or have I misunderstood?

interface ObjectConstructor {
    fromEntries<K extends PropertyKey, T = any>(entries: Iterable<readonly [K, T]>): Record<K, T>;
}

@DanielRosenwasser
Copy link
Member

Well, I don't know if you can have multiple overloads, but that fails in the case of heterogenous property types:

interface ObjectConstructor {
    fromEntries<K extends PropertyKey, T = any>(entries: Iterable<readonly [K, T]>): Record<K, T>;
}

Object.fromEntries([["a", 42], ["b", "hello"]]); // errors

@niieani has a PR at #50203 but it fails once the value is not as const'd or contextually typed.

interface ObjectConstructor {
    fromEntries<KeyValue extends readonly [PropertyKey, any]>(
        entries: Iterable<KeyValue>,
    ): [KeyValue] extends [[PropertyKey, any]]
        ? { [k: string]: KeyValue[1] }
        : { [K in KeyValue[0]]: KeyValue extends readonly [K, infer V] ? V : never };
}

const a = [["a", 42], ["b", "hello"]];
Object.fromEntries(a)
//                 ~ error

(CC @sandersn)

@SamB
Copy link

SamB commented Feb 25, 2023

Oh, back on-topic, it looks like there were web-compatibility problems with the name Array.prototype.groupBy (issue: tc39/proposal-array-grouping#37) and with the next name they tried, Array.prototype.group (issue: tc39/proposal-array-grouping#44).

It looks like people are now thinking that the next thing to try is adding groupBy/groupToMap functionality as static factory methods on Object and Map, (PR tc39/proposal-array-grouping#47), with no particular provision for subclasses.

We didn't need provision for subclasses anyway …

which makes sense to me because:

  • For the ArrayLike to object version, you'll basically always want a null prototype to avoid key collisions.
  • For the ArrayLike to Map version, this keeps the engine devs' jobs relatively simple (there's no way for user code to observe any of the steps between "7. Let map be ! Construct(%Map%)." and "9. Return map.", so they can optimize the heck out of it as long as the entries end up in the correct order, without needing to worry about fallback to a "slow" version) but you can easily override the method on a subclass constructor if you want.

Anyway, the functions on Array.prototype didn't make any specific provision for subclasses, either.

@niieani
Copy link

niieani commented Feb 28, 2023

@DanielRosenwasser Just FYI, I've addressed these failure cases, though the overall complexity has increased. The strict and sound typing for Object.fromEntries is published in an NPM package nesity-types.

@nickmccurdy
Copy link
Contributor

nickmccurdy commented Jul 18, 2023

The proposal has moved Array.prototype.groupBy to Array.groupBy and Array.prototype.groupByToMap to Object.groupBy.

Here are the type declarations I'm using for reference (though I'm not sure about the soundness issue):

global {
	interface ObjectConstructor {
		groupBy<T>(
			items: Iterable<T>,
			callbackfn: (value: T, index: number) => string,
		): Record<string, T[]>;
	}

	interface MapConstructor {
		groupBy<T, U>(
			items: Iterable<T>,
			callbackfn: (value: T, index: number) => U,
		): Map<U, T[]>;
	}
}

If you're committing them here or to another open source project, please attribute me. For example, using git:

git commit --author "Nick McCurdy <nick@nickmccurdy.com>"

@karlhorky
Copy link
Contributor

karlhorky commented Aug 29, 2023

@nickmccurdy thanks for this! Some small improvements that I needed to make:

declare global {
  interface ObjectConstructor {
    groupBy<Item, Key extends PropertyKey>(
      items: Iterable<Item>,
      keySelector: (item: Item, index: number) => Key,
    ): Record<Key, Item[]>;
  }

  interface MapConstructor {
    groupBy<Item, Key>(
      items: Iterable<Item>,
      keySelector: (item: Item, index: number) => Key,
    ): Map<Key, Item[]>;
  }
}

@bakkot
Copy link
Contributor

bakkot commented Oct 10, 2023

Drive-by: the type for the Object version should be at least a little more precise. E.g.

declare function 
    ObjectGroupBy<Item, K extends PropertyKey>(
      items: Iterable<Item>,
      keySelector: (item: Item, index: number) => K,
    ): Record<K, Item[]>;

so that if your selector returns (e.g.) one of a fixed list of string values, that is reflected in the type of the result (like this).

@karlhorky
Copy link
Contributor

karlhorky commented Oct 11, 2023

Thanks for the generic parameter idea @bakkot, updated my post above with credit to you 🙌

@aiktb
Copy link

aiktb commented Nov 12, 2023

Object.groupBy is now fully supported, need this badly.
image

@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this Bug A bug in TypeScript labels Nov 13, 2023
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Nov 13, 2023
@noahehall
Copy link

+1

and its now a static on Map

@EgizianoEG
Copy link

bump +1

@Pyrolistical
Copy link

Object.groupBy and Map.groupBy achieved consensus 4 days ago at TC39. tc39/ecma262#3176

@nikeee
Copy link
Contributor

nikeee commented Dec 9, 2023

    groupBy<Item, Key extends PropertyKey>(
      items: Iterable<Item>,
      keySelector: (item: Item, index: number) => Key,
    ): Record<Key, Item[]>;

I stumbled upon that: Returning Record could lead to a wrong return type. For example, this is always evaluates to the same key. TypeScript doesn't always know that some branch is effectively dead or that the original collection doesn't map something to a specific group (the simplest case is when it's empty).

Using Partial<Record<Key, Item[]>> as a return type would be more correct, I think.

@huw
Copy link

huw commented Dec 16, 2023

I’d like to +1 @nikeee there, I raised DefinitelyTyped/DefinitelyTyped#67896 yesterday but we agreed it might be an issue for TypeScript core to determine, since it revolves around the utility of noUncheckedIndexedAccess and whether the design goals of this type lean toward usefulness or correctness.

I’ll argue that a very common (or at least very useful) use of groupBy will be to simulate a .filter() that returns accepted and rejected items, like so:

Object.groupBy([2, 4, 6], value => value % 2 === 0 ? "even" : "odd");

I think it is reasonable that some number of these filter-like use-cases will pass an array that may not satisfy all branches of the comparator. In all of these cases, the outputs will be incorrectly typed. The example above will produce { even: [2, 4, 6] } but have the type { even: number[]; odd: number[] }. typeof result.odd satisfies number[], even when noUncheckedIndexedAccess is enabled.

I can understand if it doesn’t make sense to change the output type to Partial, but in that case perhaps a compiler flag could be useful to simulate noUncheckedIndexedAccess-like behaviour on this output?

(Additionally, the lift of const { even = [], odd = [] } = Object.groupBy(…) isn’t that high for most developers, I don’t think it’s an unreasonable ask and pretty cleverly uses language features)

@nikeee
Copy link
Contributor

nikeee commented Dec 16, 2023

Well, if the passend array is empty, the result will be an empty object, so the type would be completely wrong if we're not returning a Partial.

I think this isn't even an edge case and it will happen very often. In fact, I ran into this issue when I used a polyfill.

@huw
Copy link

huw commented Dec 16, 2023

And since Daniel is already comparing this to Object.fromEntries above, I think Ryan’s reasoning on the type correctness there would probably also apply:

It's not correct to use the input type to determine the output type, because knowing what might be in an array is not the same as knowing what's actually in it. This program is legal per the above definitions but unsound:

const arr: Array<["A", 1] | ["B", 2]> = [];
let o = fromEntries(arr);
let m: number = o.A;

@bakkot
Copy link
Contributor

bakkot commented Dec 16, 2023

Typing as Partial<Record<Key, Item[]>> sounds right to me. You can still do destructuring, so it's still usable.

@bakkot
Copy link
Contributor

bakkot commented Dec 16, 2023

Went ahead and opened #56805. I credited @nickmccurdy in the commit per request above; if anyone else who contributed wants to be credited as well I'm happy to add you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Effort: Casual Good issue if you're already used to contributing to the codebase. Harder than "good first issue". Help Wanted You can do this
Projects
None yet
Development

Successfully merging a pull request may close this issue.