uvarov-frontend / vanilla-calendar-pro

The Vanilla Calendar Pro is a versatile JavaScript date and time picker component with TypeScript support, making it compatible with any JavaScript frameworks and libraries. It is designed to be lightweight, easy to use, and feature-rich, without relying on external dependencies.
https://vanilla-calendar.pro
Other
570 stars 66 forks source link

Too strict types for e.g. `selectedMonth` and `FormatDateString`? #327

Closed johanrd closed 3 weeks ago

johanrd commented 3 weeks ago

Currently the allowed type for selectedMonth is quite strict:

selectedMonth?: 0 | 1 | 2 | 5 | 10 | 4 | 3 | 6 | 7 | 8 | 9 | 11 | undefined

This makes sense in an of itself, but given typescript's return type of .getMonth() is just number, it feels strict that this return type gives an error:

options: { selectedMonth: new Date().getMonth() }
// Type error: Type 'number' is not assignable to type '0 | 1 | 2 | 3 | 10 | 11 | 4 | 5 | 6 | 7 | 8 | 9 | undefined' ts(2322)

Not sure at all, it could easily be considered a bug in typescript, instead of a bug in vanilla-calendar pro.

The alternative approach is to type selectedMonth: ReturnType<Date["getMonth"]>. This way, the types of vanilla-calendar-pro just "uses the platform" (i.e. using the typescript types instead of maintaining its own type definitions), and will be updated automatically whenever typescript evolves.

Posting as dx feedback, please feel free to close if you disagree.

ghiscoding commented 3 weeks ago

I saw similar strictness when giving a try to the Beta release and looking at the internal code, I simply went with importing and using the same type as internal which is Range<12> and that resolved my issue :)

In real world, I would say that it's the getMonth() that is sadly not strict enough because it's impossible for that function to return a number higher than 11 (zero index), so why are they returning number? I'm assuming it's just because returning a number type is much simpler

uvarov-frontend commented 3 weeks ago

@johanrd I agree with the month, but what about `FormatDateString'? Are there any options?)

ghiscoding commented 3 weeks ago

@uvarov-frontend I'm not sure if you heard about TypeScript ReturnType<T>, but even then for the month, it will return a number type, however the benefit is that it's auto-inferred. It might be nice to use in some cases, however I'm not sure there's real benefit here though, but anyway it's a "nice to know" thing

let month: ReturnType<typeof Date.prototype.getMonth>;

image

uvarov-frontend commented 3 weeks ago

@ghiscoding This is useful to know, thanks, but as you noted, it is useless in this case. @johanrd ReturnType<typeof Date.prototype.getMonth> returns type number and we need the range from 0 to 11.

johanrd commented 3 weeks ago

we need the range from 0 to 11.

Maybe yes. But also maybe no; There are no runtime errors if you pass in e.g. 13 (the runtime fallback is good!), so it may be 'unnecessarily clever' to narrow the types more than the base types of typescript.

I agree with the month, but what about `FormatDateString'? Are there any options?)

An option is to allow just a normal 'string': new Date().toISOString() | new Date().toISOString().substring(0,10) | dayjs().format('YYYY-MM-DD') all simply return string.

These return types are how many developers will pass dates into vanilla-calendar, and when they do, it may feel weird to need to cast or //@ts-ignore values you 'know will work in practice' when interacting with vanilla-calendar.

In real world, I would say that it's the getMonth() that is sadly not strict enough because it's impossible for that function to return a number higher than 11 (zero index), so why are they returning number? I'm assuming it's just because returning a number type is much simpler

Yes. And this is of course highly subjective. I see there have been several discussions (36401, 43222). I do understand the usefulness/sophistication when manually typing dates – it just felt a bit unexpected when passing in return types of standard javascript date objects (and other date libraries).

Another beauty of relying on the typescript return types, is that the bikeshedding I am now guilty of (sorry:p) is 'outsourced' to typescript. If/when typescript ever decides to narrow their types from 'simple' to 'correct', vanilla calendar will get the update 'for free' at the same time as the rest of the wider js ecosystem.

uvarov-frontend commented 3 weeks ago

@johanrd You can enter any knowledge in any format in the line and it would be strange to allow it and expect a normal reaction from the calendar, the same with the dates, why specify the 13th month of the calendar and count on the fact that there will be no error? Or we need to add validation for each disputed parameter, and I would not want to do it, since there is a typescript for this. And besides, you can import all existing types from the library and apply them, especially since there are not so many such parameters, but they bring clarity on which value is allowed and which is not.

In any case, I agree with you that it is unexpected to receive a notification of a typing error when you use the standard method of receiving the month, but I think it is a problem that this method objects to the type of any number and not a range of real months.

uvarov-frontend commented 3 weeks ago

In short, I’ll close this because everything is clear already 😀