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
Replace moment with dayjs #7025
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7025 +/- ##
==========================================
+ Coverage 88.48% 88.49% +<.01%
==========================================
Files 229 229
Lines 8418 8422 +4
==========================================
+ Hits 7449 7453 +4
Misses 969 969
Continue to review full report at Codecov.
|
@medikoo can you take a look at this? |
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.
@cktang88 Thank you! Looks good, still I'd rename vars to dayjs
, as otherwise it may confuse devs they rely on moment
specifically when reading the code (and require
is on top of the file it can be missed).
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.
Thank you @cktang88 !
What did you implement
Closes #7022
Build size should be much smaller now, since
dayjs
is around 60kb less thanmoment
gzipped.dayjs: https://bundlephobia.com/result?p=dayjs@1.8.17
moment: https://bundlephobia.com/result?p=moment@2.24.0
How can we verify it
Run the tests.
Todos
Useful Scripts
npm run test-ci
--> Run all validation checks on proposed changesnpm run lint-updated
--> Lint all the updated filesnpm run lint:fix
--> Automatically fix lint problems (if possible)npm run prettier-check-updated
--> Check if updated files adhere to Prettier confignpm run prettify-updated
--> Prettify all the updated filesIs this ready for review?: YES
Is it a breaking change?: NO