-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Little improvements on npm commands + Upgrade of cross-env #2894
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
base: dev
Are you sure you want to change the base?
Conversation
src/index.js
Outdated
10: 30, // November | ||
11: 31 // December | ||
} | ||
|
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.
Introducing the MONTH_DAYS constant might increase the bundle size, which may not be an ideal optimization?
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.
Hi @iamkun, if you prefer, I can roll back this change. But with minification, it compresses to roughly:
const MONTH_DAYS={0:31,1:28,2:31,3:30,4:31,5:30,6:31,7:31,8:30,9:31,10:30,11:31}
That’s 80 bytes. After gzip compression, it’s 70 bytes (source).
So in practice, this code would increase the bundle size by 0.07 KB (virtually negligible).
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've noticed this PR introduces some additional code into both utils
and index
.
A core principle of Day.js is its minimal bundle size, and we're highly sensitive to any changes that might increase it. For a change like this to be a worthwhile trade-off, we would need to see a significant performance improvement over the current endOf
implementation.
Have you conducted any performance benchmarks to demonstrate the gains of this new approach? This data would be very helpful for us to evaluate if the change is a net positive for the project.
Looking forward to your thoughts!
Hi! I have discarded the changes related to calculating the days of the month, as I don't have time to measure whether the library now weighs 1 byte more. I left the Best regards and thank you! |
Hi! First of all, thanks for this amazing project!
This pull request introduces performance enhancements to the
daysInMonth
function as well as a few improvements in thepackage.json
file. The changes are divided into two categories:Updates in
daysInMonth
and leap year handling:Updated the
daysInMonth
function to use a newMONTH_DAYS
constant and theyearIsLeap
utility (I've centralized a function to use here and in theisLeapYear
plugin) for determining the number of days in February during leap years. This is simpler than calling thestartOf
method and doing complex stuff.Also, I've improved the Typescript definition of the
daysInMonth
function to return the correct possible numbers28 | 29 | 30 | 31
.Improvements in
package.json
:lint
script by removing the relative path toeslint
.test
script to usecross-env
for better cross-platform compatibility in thepackage.json
file.cross-env
to version5.2.1
.Hope it helps 😄