-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
refac(dayjs): refactoring duration internal constants #2403
Conversation
src/plugin/duration/index.js
Outdated
let $d | ||
let $u | ||
let $d = null | ||
let $u = null |
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 initialized it to null
to explicitly write that the value is currently empty.
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.
any reason to initialize it to null, can we simply use let $d
?
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.
@iamkun
Yes, my initial intention was to explicitly initialize null to indicate that there is no current value, but I'll revert back to that.
good👍 |
The reason to have a constant in duration plugin rather than in the common constant is that the |
Does this make a big difference in package size? |
This is because not everyone needs the duration plugin, and should not download these constants in the core module |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## dev #2403 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 183 183
Lines 2249 2249
Branches 636 636
=========================================
Hits 2249 2249
☔ View full report in Codecov by Sentry. |
@iamkun I understand, I thought it was a very small difference. 🙏 |
Hello 👋 @iamkun Thank you for sharing such a great library.
It would be nice to have
MILLISECONDS_A_YEAR
andMILLISECONDS_A_MONTH
insideduration
managed together in "constants.js", what do you think?