Comments (16)
Ironically, typescript is had this same discussion in the MR to support Object.groupBy
: https://github.com/microsoft/TypeScript/pull/56805/files#r1439935333
from types.
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
from types.
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 F
s, 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
from types.
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! :)
from types.
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.
from types.
Object.entries
will never include the non-existent keys that would be undefined.
Yes, this was what i was referring to. Thanks!
from types.
Ok so this seems to essentially come down to the philosophic question of whether noUncheckedIndexedAccess
should be used, because that implies that Record
s 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.
from types.
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
from types.
Interestingly enough, the issue is when a string is used as opposed to a union type.
from types.
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
from types.
Regardless, to me Partial
still seems safer / the most correct type given what the type system can express, as outlined in my comment
from types.
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
from types.
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?
from types.
@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
from types.
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.
from types.
@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
from types.
Related Issues (20)
- Make `isNotNil` a type guard HOT 3
- pipe and useWith get ts error HOT 3
- Missing copyright notice HOT 2
- propEq type doesn't account for optional properties HOT 1
- Issues with upgrading from 0.28.25 to 0.29.1 when using yarn PnP HOT 1
- Why is `ElementOf` so complex? HOT 1
- `filter` chooses wrong type overload when used with `map`. HOT 2
- @types/ramda failing under 5.4 HOT 3
- Symbol type is not filtered out of keys in types for fromPairs, keys, toPairs, and toPairsIn HOT 2
- [0.29.9] `pluck` inside `pipe` get ts error HOT 2
- Real world usage of omit is now awkward HOT 6
- 'Placeholder' is not assignable to parameter of type 'string | ((match: string, ...args: readonly any[]) => string)' in replace HOT 3
- `curry` no longer works with `map` HOT 2
- forEach requires type parameter HOT 2
- `groupBy` partial object creates issues with downstream usage. HOT 2
- Export isNotEmpty types as it is now part of ramda 0.30.0 HOT 1
- 0.30.0 lensProp type-check fail HOT 1
- ramda pick type inference is not correct HOT 1
- Typescript type inferencing HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from types.