-
Notifications
You must be signed in to change notification settings - Fork 4
replace moment with dayjs #1706
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
Conversation
Jest Unit Test Statistics 1 files ±0 193 suites ±0 8m 8s ⏱️ -6s For more details on these failures, see this check. Results for commit f80e13a. ± Comparison against base commit 6578b6b. ♻️ This comment has been updated with latest results. |
BogdanDenis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @JohnC-80, the PR looks good. I've added one commit to fix some import formatting
| const packageRange = moment.range(packageBeginCoverageDate, packageEndCoverageDate); | ||
| const packageBeginCoverageDate = dayjs.utc(packageBeginCoverage); | ||
| const packageEndCoverageDate = packageEndCoverage ? dayjs.utc(packageEndCoverage) : dayjs.utc(); | ||
| const packageRange = new DayRange(packageBeginCoverageDate, packageEndCoverageDate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @JohnC-80 this is a very very belated review, apologies for that.
It looks like functions for moment.contains and dayjs.isBetween work differently.
moment returns true if we check dates that are either the beginning or end date, but dayjs will return false.
For example, for when checking if date 2021-01-01 is in range 2021-01-01 to 2021-01-31 moment will return true, but dayjs will return false.
The isBetween plugin has a parameter that defines if begin and end dates should or should not be included: https://day.js.org/docs/en/plugin/is-between
I think since we've been using moment and by default it includes start and end date then we should do the same with dayjs. Do you have any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am working on making this PR up-to-date so we can merge it, just need to confirm with you if we should make the date inclusivity a default behaviour or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created a PR to handle date ranges correctly folio-org/stripes-components#2443
Jest Unit Test Results 1 files ±0 195 suites ±0 4m 57s ⏱️ +15s Results for commit 9f041f4. ± Comparison against base commit 8f8548b. This pull request removes 1 and adds 3 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
|


The moment project has been sunset for a bit now.
In efforts to move the FOLIO platform to a supported library with as easy a transition as possible. we intend to make the move to dayJS.
It's practically a drop-in replacement for moment, with a VERY similar API.
Its locale information is split from the main bundle and it's very modular/plugin-based.
stripes-componentswill re-export/provide the import for dayjs with numerous useful plugins included as well as utilities for dynamically loading dayjs locale data.More information is available at https://day.js.org/en/
Related PR's: