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

fix: match day counting behavior with Day.js in English locale #21

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

0ui
Copy link

@0ui 0ui commented Oct 23, 2024

I ran into an issue when counting days where the behavior seemed a little odd to me. I noticed that Day.js was cited for reference and confirmed that the behavior in Day.js is different. Essentially if I compare Monday 8:00AM to Wednesday 8:01AM it will return 3 days ago instead of 2 days ago and it makes day counts feel 12 hours too early.

timediff Day.js
Day 1 00 - 35 (35hrs) 00 - 36 (36hrs)
Day 2 35 - 48 (13hrs) 36-60 (24hrs)
Day 3 48 - 72 (24hrs) 60-84 (24hrs)
Day 4 72 - 96 (24hrs) 84-108 (24hrs)

Day.js has regular 24-hour intervals after the first long day, but timediff has a shorter 13-hour second day due to not offsetting for the long first day.

This change makes all the intervals a regular 24 hours after the first and updates the tests to reflect the changeover time. Hope that all makes sense. I could also try to adjust the other locales if that's helpful. Cheers!

@0ui
Copy link
Author

0ui commented Oct 23, 2024

Not sure if it was the intent to entirely match with Day.js behavior, but when I compared the existing tests with Day.js, I found about 14 that didn't align with the output, largely due to precision around months/years, but at the precision level expected for a library like this they all seem very fixable.
Screenshot 2024-10-23 at 7 52 35 PM

There are also discrepancies in inclusiveness forward and backward in time. If that would be helpful, I could also assist in matching that behavior in a separate PR.

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.

1 participant