jkbrzt / rrule

JavaScript library for working with recurrence rules for calendar dates as defined in the iCalendar RFC and more.
https://jkbrzt.github.io/rrule
Other
3.3k stars 513 forks source link

Typescript definition for RRule constructor is incorrect. #425

Open LouiseBC opened 3 years ago

LouiseBC commented 3 years ago

Rrule version: 2.6.6

Problem: The typescript definition for the rrule constructor is incorrect. It is defined as Partial<Options>, which turns something like interval: number into interval?: number | undefined. It is true that the interval key can be left out, but the rrule.between function never returns if you pass interval: undefined to the library. I haven't spent much time with the library so I can't tell if this same issue is true for any of the other options.

smichel17 commented 3 years ago

Partial<{ interval: number }> is equivalent to { interval?: number }, not { interval?: number|undefined } :slightly_smiling_face:

https://github.com/microsoft/TypeScript/blob/master/src/lib/es5.d.ts#L1439-L1441

Often people check for the presence of keys with obj.key !== undefined rather than "key" in obj, so the Typescript compiler may be doing some shenanigans that allows you to pass { interval: undefined } to the constructor even though it isn't valid… but it shouldn't. So I think this is an issue with typescript, not rrule.

LouiseBC commented 3 years ago

I think it's a bit confusing, but the type you linked results in type Partial<T> = { [P in keyof T]?: T[P] | undefined; }. I think because the key is optional, the value is also automatically optional. If I hover over the 'Partial' type in my IDE it confirms this. So I still think the typings for this library are incorrect

smichel17 commented 3 years ago

If

type Partial<T> = {
    [P in keyof T]?: T[P];
}; 

Results in

type Partial<T> = {
    [P in keyof T]?: T[P]|undefined;
}; 

Then I don't see how it would be possible to express the version without undefined, so I'm not sure how one could fix the type. Maybe with something like,

type DefinedOnly<T> = {
    [P in keyof T]: T[P] extends undefined ? never : T[P];
};

Which you would use like, DefinedOnly<Partial<<T>>. (This isn't a full definition, you'd need an extends somewhere in there so it doesn't make all the properties required again.

Overall this reinforces that it seems like a bug or shortcoming in typescript -- making properties optional is a relatively simple transformation and if there's not a way to express it cleanly, that's a problem.

It's also possible that it's a bug in your IDE's type hinting. Edit: Or that the Partial type you're seeing is not the one I linked?

LouiseBC commented 3 years ago

Hm I see your point. I think you could express what is meant, but it would be unnecessarily convoluted. This isn't an issue with my IDE, I checked on the typescript playground and it's giving me the same results.

Let me change my question then: Could the implementation be changed in such a way that it doesn't break (quite dramatically) if interval: undefined is passed as an option? It seems reasonable to me that values that are not defined are ignored