Feature/improve group by key types

[TkDodo]

by inferring the type of the key from the provided function; it must still extend a PropertyKey because that's what an object needs as index

[Bessonov]

@TkDodo Wow, great! I had the same idea after wondering about groupBy return types. From my point of view, you need to include undefined:

): Record<PropertyKey, NonEmptyArray<T> | undefined>;

But I'm not sure about this change: microsoft/TypeScript#13195 . It could lead to different semantic.

[TkDodo]

@Bessonov why would we need to include undefined? After we group, the resulting object will only have keys where also values exist for it. That's also the reason why we have a NonEmptyArray and not just an Array - because accessing the first element is safe, as it will always be there...

[Bessonov]

@TkDodo correct me, if I'm wrong, but without undefined you get no type error here:

interface MyType {
	type: 'right' | 'wrong'
}

const myTypes: MyType[] = [
	{ type: 'right' }
]

const grouped = groupBy(myTypes, item => item.type)

grouped.wrong.length <-- fine

But it breaks at runtime.

[Bessonov]

Or, I think, using Partial would preserve the semantic:

Partial<Record<PropertyKey, NonEmptyArray<T>>>

[TkDodo]

@Bessonov you are most certainly right 👍

However, adding undefined to the mix will make it harder to work with when iterating over the object with Object.entries. consider:

interface MyType {
	type: 'right' | 'wrong'
}

const myTypes: MyType[] = [
	{ type: 'right' }
]

const grouped = groupBy(myTypes, item => item.type)

Object.entries(grouped).forEach(([, value]) => {
    console.log(value[0])
})

Here, we should be able to access value[0], but this won't work if we add undefined to the mix.

Maybe what I'm trying to do in this PR is not the right thing to do? After all, I think things like this are the reason why TypeScript only returns string for Object.keys ...

[Bessonov]

@TkDodo I've created a playground with all three definitions above. It looks like the "Partial"-Definition

declare function groupByPartial<Item, Key extends PropertyKey>(
  items: readonly Item[], fn: (item: Item) => Key,
): Partial<Record<Key, NonEmptyArray<Item>>>

works as expected. What do you think about that?

However, adding undefined to the mix will make it harder to work with when iterating over the object with Object.entries. consider:

If I must choose between groupByOriginal and groupByUndefined, then I would choose groupByUndefined, because in this case I think that it's a bug of Object.entries and in my practice groupByUndefined gives more benefit. But I hope that groupByPartial works as expected.

[TkDodo]

@Bessonov thanks for digging into this. You are right that the Partial solution works - with TS 4.3 and onwards. I was just double checking why it doesn't work when I wrote a test for it, and it seems to be that we are on TS 4.0 still in the repo 😅 .

I will update the PR to use Partial and also update the TS version to the latest.

[TkDodo]

@Bessonov have another look please

[Bessonov]

Great, thanks!

[Bessonov]

I'm not using the indexed version, but shouldn't it have same return type?

[TkDodo]

thx for seeing that, you are right 🙏

b66c416

[Bessonov]

maybe add

    // @ts-expect-error other values are not allowed by typings
   expect(grouped.notexists).toBeUndefined();

[TkDodo]

70438f6

[Bessonov]

Not related to this PR, but I think we can replace this definition one day:

/** types that may be returned by `keyof` */
export type Key = string | number | symbol;

with PropertyKey.

[TkDodo]

done: 83e04aa

[Bessonov]

Great, I'm fine with it, thank you!