groupBy returns a type that is not compatible with Object.entries

[nemanja-tosic]

Given the following groupBy invocation using version 0.29.10 of types

const messagesByGroupId = groupBy(
  message => message.groupId,
  messages,
);

when using entries and strict like so

for (const [groupId, messages] of Object.entries(messagesByGroupId)) {
  // messages might be undefined
}

the messages variable type is T[] | undefined forcing us to do an extra check, but it shouldn't be possible for it to be undefined since we're iterating over object properties.

This PR introduces the change - #45 that leads to the possible undefined.

[Harris-Miller]

the messages variable type is T[] | undefined forcing us to do an extra check, but it shouldn't be possible for it to be undefined since we're iterating over object properties.

This is why we added the Partial actually

Consider the example used in the Ramda docs:

const byGrade = R.groupBy(function(student) {
  const score = student.score;
  return score < 65 ? 'F' :
         score < 70 ? 'D' :
         score < 80 ? 'C' :
         score < 90 ? 'B' : 'A';
});
const students = [{name: 'Abby', score: 84},
                {name: 'Eddy', score: 58},
                // ...
                {name: 'Jack', score: 69}];
byGrade(students);
// {
//   'A': [{name: 'Dianne', score: 99}],
//   'B': [{name: 'Abby', score: 84}]
//   // ...,
//   'F': [{name: 'Eddy', score: 58}]
// }

The type of the return could be typed as

type Grades = Record<'A' | 'B' | 'C' | 'D' | 'F', Student[]>

The operation groupBy creates an object of type Grades from type Student[], but there is no guarantee that the object actually has defined on it all of those keys. It's the difference between build-time and run-time. If your array of students happens to have no Fs, grades['F'] returns undefined. And thus grades['F'].map(s => s.name) would give you the classic "Uncaught TypeError: Cannot read properties of undefined (reading 'map')". `

And it's for this very reason that MR wrapped the return in Partial. It's there for type-safety

That being said... The Partial may have also been added prematurely.

Because for either Record<'A' | 'B' | 'C' | 'D' | 'F', Student[]> or for Record<string, Student[]>, there is sort of a natural expectation that the key access can and should return | undefined. It's like that for Map#get(), and is something I believe typescript realized their mistake on, as you can opt into this behavior now in tsconfig.json with the prop noUncheckedIndexedAccess

I've been considering reverting that MR so the behavior of Record<string, T> access returning T or T | undefined can be selected by the project's tsconfig and not the library

@jeysal contributed that MR, and I'd like their opinion on this as well

[jeysal]

Hi, im at FOSDEM this weekend so things are a bit hectic, but I'll try to read through and get back to you next week! :)

[Harris-Miller]

I thought about this over night at realized you're right. The problem is Partial<Record<string, Whatever>> only makes sense for accessing on that object itself, but when you transform it into another shape like Object.entries it breaks down, exactly in the way you describe with T | undefined. Object.entries will never include the non-existent keys that would be undefined.

#94

[nemanja-tosic]

Object.entries will never include the non-existent keys that would be undefined.

Yes, this was what i was referring to. Thanks!

[jeysal]

Ok so this seems to essentially come down to the philosophic question of whether noUncheckedIndexedAccess should be used, because that implies that Records are by default to be treated as Partial.
I'm not really super qualified in that case because I have never used noUncheckedIndexedAccess. My intuition is that I don't like it very much because it prevents me to make a distinction between types that I explicitly want to strictly contain a value for every possible key and types that are partial. But I recognize that noUncheckedIndexedAccess: false is not safe in all cases and that for people which extensively rely on Record<string, T> or other types with infinite key type space, and not so much on e.g. string literal union keys or other types with finite key type space, the situation might be different because they would just have to put Partial everywhere because that's the only thing that makes sense for infinite keys.

Bear in mind though that noUncheckedIndexedAccess is not on by default and is not included in strict, and for people that do not have it on, changing this back will worsen safety, because we know that groupBy does not necessarily return a complete Record.
The given example seems more like a shortcoming in usage for Object.entries and perhaps some other functions, because the type only says "the keys A,B,C,D,E,F will all either have an array or an undefined value" when we'd ideally want to say something like "this object has a subset of keys A,B,C,D,E,F and an array value for each of them" but we cannot express this in TypeScript.

[jeysal]

Also, there might be something else strange going on? If I try Object.entries over a partial record in the playground, I don't even run into a problem of the value being | undefined as reported in this issue

https://www.typescriptlang.org/play?#code/MYewdgzgLgBAHgLhgBQIYCcoEtUBsA8ASgKajoAm+A5KlQD5UBGVANGAK4C2jx6A2gF0AfEJgBeGAG8AUDBiokfACwAmAS2kBfaQDMQ6GAApQkWH1QtGAmCB0wA8owBWpKADpiYKOizEIhuABKQKlZGEYtIA

[nemanja-tosic]

Interestingly enough, the issue is when a string is used as opposed to a union type.

[jeysal]

That is interesting indeed. It is using the same entries<T>(o: { [s: string]: T } | ArrayLike<T>): [string, T][]; signature but in the string-keyed case T is inferred with | undefined

[jeysal]

Regardless, to me Partial still seems safer / the most correct type given what the type system can express, as outlined in my comment

[Harris-Miller]

Ironically, typescript is had this same discussion in the MR to support Object.groupBy: https://github.com/microsoft/TypeScript/pull/56805/files#r1439935333

[Harris-Miller]

Looking at the typescript MR for Object.groupBy, they are returning Partial<> for it same as well. It's almost a perfect match for R.groupBy

[Harris-Miller]

Given how we now know that R.groupBy is the same as the new native Object.groupBy in terms of the return signature being wrapped in Partial<>, I'm going to close this issue.

There still an odd difference between union keys Record<'a' | 'b', number[]> versus a string Record<string, number[]>, (see here: https://tsplay.dev/WY5JvW), but that's a typescript issue and not a Ramda issue

@nemanja-tosic Thank you for bringing the issue to our attention none-the-less. It made for a god piece of conversation

[fuadsaud]

Stumbled upon this problem recently and, as I'm relatively new to TypeScript, I'm having a bit of trouble wrapping my head around it.

My use case is quite simple and doesn't rely on union types for the keys, here's a fictional, simplified example:

type Item = {name: string, category: string}

const items = [...]

R.groupBy(R.prop('category'), items)

Intuitively I expected the last expression to return a Record<string, Item>. I can see how this might not work if the type of the key is more strict. But is there a better way to approach this for generic, string type keys?

[Harris-Miller]

@fuadsaud from your example, originally the return type was Record<string, Item[]>, but was updated to be Partial<Record<string, Item[]>> in #45

We're keeping the Partial<> because it matches the upcoming typings for the now native Object.groupBy function

Playground of your example: https://tsplay.dev/N78bDm

[AndrewSouthpaw]

Do you have any recommendations for usage? It's unfortunate that a forEach/map/etc of a Partial object returns | undefined for every key, when none of the values are actually undefined.

I understand this point, about a return type of type Grades = Record<'A' | 'B' | 'C' | 'D' | 'F', Student[]> potentially resulting in incorrect runtime type. But if we type it as type Grades = Record<string, Student[]> and iterate across it, I'd hope to not have to do an undefined check, because it seems from my middle understanding of TS that it would never happen?

I was under the impression that with noUncheckedIndexedAccess, when you iterate across an array or object it'll still give you the type without | undefined, and that it's more about protecting against direct access (obj.foo instead of an iteration). It would be nice if that were possible behavior to preserve here. (I'm guessing that ties into this point, which is being treated as a TS issue and not a ramda issue?)

I'm no TS expert though so I could be totally wrong here.

[Harris-Miller]

@AndrewSouthpaw Your problem isn't unique to ramda, it will be true if you use R.groupBy or Object.goupBy. Using the example you gave in #113

  Object.values(byColor).map((grouping: Foo[]) => {
    console.log(grouping);
  });

The main problem there is you manually typed grouping: Foo[]. If you do

  Object.values(byColor).map(grouping => {
    console.log(grouping);
  });

grouping will be inferred for you as Foo[] | undefined. See this playground for full example: https://tsplay.dev/WyVAZw

Regardless of the correct typing, you're right in that grouping should be Foo[] and not Foo[] | undefined. Because in this scenario, Object.values() doesn't include those undefined values in the array

However! Understand that the ? doesn't mean that "the key may not exist, but when it does is of that value". What it really means is "key can be undefined". Because this is also valid:

In this case, you do have an undefined in the array. Now while you can't get that from Object.groupBy, understand that the runtime situation I'm showing here is 100% valid to the type definition Partial<Record<string, Foo[]>>. Is this annoying? Yes. But it's also accurate! And that type-safety is important to prevent you from runtime errors

The only solution there is is just to check for | undefined. Fortunately, R.isNil and R.isNotNil both do type narrowing for you which helps with this greatly! Check out with playground for an example: https://tsplay.dev/Na57BN