Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make months and weekdays types slightly less restrictive #843

Merged
merged 1 commit into from
Feb 21, 2019

Conversation

johnjesse
Copy link
Contributor

I think the typings for months, weekdaysLong and weekdaysShort are too strict. For instance, I get a type error from the following

const months = ['J', 'F', 'M', 'A', 'M', 'J', 'J', 'A', 'S', 'O', 'N', 'D'];

<DayPicker
  months={months}
  ...
/>

because the inferred type of the months variable is string[]. Whilst the current typings for months, weekdaysLong and weekdaysShort are strictly true in requiring 12, 7 and 7 entries, typescript has difficulty in inferring those types and this makes them unwieldy to use in practice. I suggest loosening them slightly to be sting[].

@codecov
Copy link

codecov bot commented Dec 12, 2018

Codecov Report

Merging #843 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #843   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines         645    645           
  Branches      141    141           
=====================================
  Hits          645    645

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39ad48e...b09406c. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Dec 12, 2018

Codecov Report

Merging #843 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #843   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines         645    645           
  Branches      141    141           
=====================================
  Hits          645    645

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39ad48e...b09406c. Read the comment docs.

@giladgray
Copy link

giladgray commented Jan 16, 2019

type aliases would solve this. if you could say

import { MonthsArray, DaysArray } from "react-day-picker";

const months: MonthsArray = [ ...12 type-safe strings ];
const weekdaysLong: DaysArray = [ ...7 type-safe strings ];

@johnjesse
Copy link
Contributor Author

I hadn't thought about exposing the type aliases.

It's still a bit annoying to have to explicitly declare the type or cast them though. Incidentally that's currently how I get around the problem - by declaring my own MonthNames, and DayNames types and casting.

Playing around with it, it gives better type safety to this case (ts-playground)

Personally I don't think it gives you very much in this instance, If I'm getting the month names from something else, like moment, I have to then cast anyway. (Or convince whatever 3rd party source I'm using to use the same typing convention).

@sokmeann
Copy link

sokmeann commented Feb 7, 2019

@johnjesse any updates on this PR? it is holding up another PR I made to blueprintjs about a month ago... Would need that merged sooner rather than later as I am using the component for an app that needs to go to production :(

@giladgray

@johnjesse
Copy link
Contributor Author

@sokmeann no updates I'm afraid, I'm not a maintainer of this library
There hasn't been an update on this repo since September so I'm not sure it's still being maintained.

As a workaround I suggest you type you extract a type

type Months = [string, string, string, string, string, string, string, string, string, string, string, string]
and declare your months as
const months: Months = ['J', ......]

or the same for weekdays

@gpbl
Copy link
Owner

gpbl commented Feb 21, 2019

Hey friends, sorry for being inactive the last months. Just trying to catch it up :)

@gpbl gpbl merged commit 2e02c52 into gpbl:master Feb 21, 2019
@johnjesse
Copy link
Contributor Author

Awesome 😄 thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants