I’d probably also credit@karlhorky and@nikeee ❤️

Done. Hope that's alright@karlhorky@nikeee.

@typescript-bot pack this

Heya@DanielRosenwasser, I've started to run the tarball bundle task on this PR at547e844. You can monitor the buildhere.

Hey@DanielRosenwasser, I've packed this intoan installable tgz. You can install it for testing by referencing it in yourpackage.json like so:

{    "devDependencies": {        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/159135/artifacts?artifactName=tgz&fileId=CFD5AF1D1FCD328827EEC59F4D7DA6580765BE88B68F32DA4FD25CE87FA5199502&fileName=/typescript-5.4.0-insiders.20231218.tgz"    }}

and then runningnpm install.

There is also a playgroundfor this build and annpm module you can use via"typescript": "npm:@typescript-deploys/pr-build@5.4.0-pr-56805-4".;

This seems good overall, though users will have to opt in to the more precise inference for keys.

I would also think to stick with the same precedent we have elsewhere for type parameter names (K andT), and Ifeel like specifying the key first might be more desirable if we ever had the ability to only provide some arguments while letting the others be inferred.

I'll let others weigh in.

This seems good overall, though users will have to opt in to the more precise inference for keys.

What do you mean by "more precise"?

Object.groupBy([0, 2, 8], x => x < 5 ? 'small' : 'large') correctly infers the key type as'small | 'large', which is what it ought to be.

Similarlytype Employee = { name: string, role: 'ic' | 'manager' }; Object.groupBy([] as Employee[], x => x.role); infers the key type as'ic' | 'manager' - again, what it should be. (These are both in the tests.)

I would also think to stick with the same precedent we have elsewhere for type parameter names (K andT), and Ifeel like specifying the key first might be more desirable if we ever had the ability to only provide some arguments while letting the others be inferred.

SGTM, done.

@bakkot I'll merge this after you merge from main (or rebaseline, whichever).

@sandersn Done.

Hi, I just noticed this and think it needs some improvements, since it doesn't handle cases like these:

functiontester<Kextendsnumber|string>(k:K,index:number):Kextendsnumber ?'numbers' :'strings'{return(typeofk==='number') ?'numbers' :'strings';}// expected numbers ✅consta1=tester(123,123);//    ^?  const a1: "numbers"// expected strings ✅consta2=tester('123',123);//    ^?  const a1: "strings"// expected error ✅consta3=tester(true,123);//    Argument of type 'boolean' is not assignable to parameter of type 'string | number'.constbasic=Object.groupBy([0,2,8,'asd'],tester);//    ^?  const basic: Partial<Record<"numbers" | "strings", (string | number)[]>>// expected number[] | undefined ❌basic.numbers//     ^?  (property) numbers?: (string | number)[] | undefined// expected string[] | undefined ❌basic.strings//     ^?  (property) numbers?: (string | number)[] | undefined

@nikelborm Typescript's types don't generally go for that level of type-level logic. It might be possible in theory but it would significantly complicate the types. The current types aren't wrong, just slightly imprecise.

They do go, and do it pretty well

It neither does "hard" logic

functiontester(k:number|string,index:number):k isstring{returntypeofk==='string';}// expected {'true'?: string[], 'false'?: number[]}constbasic=Object.groupBy([0,2,8,'asd'],tester);//  Type 'boolean' is not assignable to type 'PropertyKey'.

nor easy (it doesn't seems hard to support just booleans)

functiontester2(k:number|string,index:number):boolean{returntypeofk==='string';}// expected Partial<Record<"true" | "false", (number | string)[]>>constbasic2=Object.groupBy([0,2,8,'asd'],tester2);//    Type 'boolean' is not assignable to type 'PropertyKey'.

It's very common use case to divide an array of something into 2 groups like this:

const{true:strings,false:numbers}=Object.groupBy([0,2,8,'asd'],(k)=>typeofk==='string');

It doesn't have to be as simple as checking is this a string or not. It may cover many other business logic use cases

constpeople=[{name:'Julie',age:15},{name:'John',age:23}]const{true:adults,false:children}=Object.groupBy(people,(person)=>person.age>=18);

@nikelborm You're welcome to send a followup PR and see if the TS team accepts it.

@sandersn what do you think about this?

Hey there 🙂 You’ve raised 2 separate issues here:

1. groupBy actually accepts all return types that are coercible toPropertyKey, and we should widen the types
2. ThatgroupBy doesn’t narrow on the real-world output types of key selector functions

PropertyKey coercion

You are correct to identify that, under spec,Object.groupBy([1, 2, 3], () => true) should technically be typed as{ true?: number[], false?: number[] }.

We can see this because thearray grouping proposal specifies that outputs should be coerced to property keys if possible, and it refers toToPropertyKey, which specifies that booleans should be coerced to the literals ‘true’ and ‘false’.

TypeScript actually manages this natively for numeric types, which are usually used to index arrays (which are sort of objects under the hood)—that’s whyPropertyKey extends number, even though the spec saysproperty keys can only be strings or Symbols.

Although other types are coercible to property keys, TypeScript seems to have preferred simplicity/sanity/ease. Challenging this is much more philosophical and would affect a large amount of TypeScript, so at the very least you’ll have to raise a new issue. It would affect a lot more thanObject.groupBy().

Narrowing key selector outputs

I am by no means a TypeScript expert, but I don’t think there’s a way to do what you’re asking for. To recap, you correctly noted that (for example)groupBy([1, "2", 3], (item) => typeof item === "string" ? "string" : "number") implicitly typeskeySelector as(item: string | number) => "string" | "number", which doesn’t have enough type information to determine the output type from the input type.

To fix this, you added a generic parameter to determine the output type from the input type, such asgroupBy([1, "2", 3], <K extends number | string>(item: K): K extends string ? "string" : "number" => typeof item === "string" ? "string" : "number"). I don’t think there’s a way to achieve thiswithout generics.

What you need, concretely, is a type that can map specificK keys toT values. A generic function type can do this by itself, as we’ve seen. But the issue is that this would require higher-order generics to infer the output type from the input type within the already-genericised function. Here’s some fake code that is doing roughly what you mean:

declarefunctiongroupBy<KextendsPropertyKey,KeySelector<T>extends(item:T,index:number)=>K>(items:Iterable<genericofKeySelector>,keySelector:KeySelector,):{[keyinT]?:KeySelector<key>[]};

#1213 seems to be the canonical issue for higher-kinded types, in case you want to follow it (or someone there is looking for a use-case here).

Alternatively, you might get clever and create a mapping fromK →T directly, using mapped object types:

declarefunctiongroupBy<TypeMapextends{},KeyextendskeyofTypeMap=keyofTypeMap,KeySelectorextends(item:TypeMap[Key],index:number)=>Key=(item:TypeMap[Key],index:number)=>Key>(items:Iterable<TypeMap[Key]>,keySelector:KeySelector,):{[keyinKey]?:TypeMap[key][]};groupBy<{"string":string;"number":number}>([1,"2",3],<Kextendsnumber|string>(value:K):Kextendsstring ?"string" :"number"=>typeofvalue==="string" ?"string" :"number");

This actually works! But (a) it’s a hack, and TypeScript doesn’t accept hacks in the core library and (b) it requires you to always specifyTypeMap, which isn’t possible to infer, and as noted above TypeScript doesn’t usually accept types that are complex to use in this way. Also, perhaps more importantly, is that it’s no more ergonomic than just overriding the return type ofgroupBy yourself.

Here’s the TypeScript playground I used for the above.

I hope that clears things up! I sort of hate to write stuff like this because it feels very conservative and self-justifying; if you have a problem with either of these answers I absolutely support starting a constructive dialogue with the TypeScript team to try and change these things—it may be the case that this is the issue that tips the balance :)

(Also, just as a matter of etiquette, I personally wouldn’t @-mention a maintainer in the future if another commenter disagrees with your take. I have to imagine it’s pretty annoying to be called in to adjudicate things, especially when another commenter has explained the next course of action (raise a new issue), and it’s pretty likely they already have notifications on for this thread and will see it in due time. In most open-source repos it’s a pretty fast way to get your position ignored.)

Returning PartialPartial<Record<K, T[]>> is quite annoying in practice

See for example:

We now have to deal with entries with undefined values, which technically is not really possible in practice.

Isn't there a way to express a Record where "only keys are Partial", but values are always defined when the key exists?

I tried various things but not sure to have a good solution.

Here's my last attempt:

typeSubsetRecord<KextendsPropertyKey,T>={[PinK]?:T;};declarefunctiongroupBy<KextendsPropertyKey,T>(items:Iterable<T>,keySelector:(item:T,index:number)=>K,):SubsetRecord<K,T[]>typeUser={name:string,age:number,type:"a"|"b"|"c"}constusers:User[]=[{name:"Seb",age:42,type:"a"}]constgroups=groupBy(users,u=>u.type);// ✅ entries do not have undefinedObject.entries(groups)satisfies[string,User[]][]// ✅groups["a"]!.toSorted()// ✅groupBy(users,u=>"Seb")["Seb"]!.toSorted()// ❌ should be forbidden: value can be undefined//@ts-expect-errorgroups["a"].toSorted()// ❌ should be forbidden because keys do not match//@ts-expect-errorgroupBy(users,u=>"Seb")["blabla"]// ❌ should be forbidden: value can be undefined//@ts-expect-errorgroupBy(users,u=>"Seb")["Seb"].toSorted()

Returning PartialPartial<Record<K, T[]>> is quite annoying in practice

See for example:

We now have to deal with entries with undefined values, which technically is not really possible in practice.

Isn't there a way to express a Record where "only keys are Partial", but values are always defined when the key exists?

I agree with this.

In cases where the type of the group keyK becomesstring (instead of a more narrow type such as"a" | "b" | "c"), having thePartial there is unnecessary and causes these issues where, according to the typing, the object values could beundefined, even though they never are in practice.

This all comes down to TypeScript's limits to separate if the property is there at all vs. if they exist with valueundefined, when dealing with this kinds of types, so for now that's something that we need to live with.

However, imo the developer experience would be greatly improved if the signature ofObject.groupBy is changed to this:

interfaceObjectConstructor{/**     * Groups members of an iterable according to the return value of the passed callback.     *@param items An iterable.     *@param keySelector A callback which will be invoked for each item in items.     */groupBy<KextendsPropertyKey,T>(items:Iterable<T>,keySelector:(item:T,index:number)=>K,):stringextendsK ?Record<K,T[]> :Partial<Record<K,T[]>>;// 👈 changed return type}

The change is on the return value of the method: it now conditionally returns the regular "dictionary" type, e.g.Record<string, User[]>, without unnecessaryPartial.

Now, the previously mentioned unnecessaryundefined in the value type of e.g.Object.entries would be gone.

This return type still matches the implementation, but is more useful in cases where the grouping key is just the broad type ofstring.

Also, with this change, it also leaves up to the developer to choose theirnoUncheckedIndexedAccess configuration depending on how they would like to deal with possibly undefined values with indexed property access.

What do you think?

I think having the types incorrectly suggest that every key is present is worse than having the types incorrectly suggest that keys which are present might have valueundefined. The former will accept incorrect programs, which you discover by running into bugs at runtime and can only fix by globally enablingnoUncheckedIndexedAccess (which affects your whole program), whereas the latter will reject correct programs, which you discover at compile-time and can fix by adding! or other casts to the specific cases where you need it.

So I do not think that would be a good change.