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: duration plugin - MILLISECONDS_A_MONTH const calculation #2362

Merged
merged 3 commits into from
Jul 9, 2023

Conversation

sanjaynishad
Copy link
Contributor

MILLISECONDS_A_DAY * 30 will generate wrong value for a larger diffs, it's generating wrong durations for months

it should be either MILLISECONDS_A_DAY * 30.416666666666668 (365 / 12) or MILLISECONDS_A_YEAR / 12

@sanjaynishad
Copy link
Contributor Author

actually one more fixes required with parseFromMilliseconds

need to check if duration ms is having leap year, then need to recalculate MILLISECONDS_A_YEAR and MILLISECONDS_A_MONTH

something like

parseFromMilliseconds() {
    let { $ms } = this
    let msInYear = MILLISECONDS_A_YEAR;
    let msInMonth = MILLISECONDS_A_MONTH;
    let years = roundNumber($ms / MILLISECONDS_A_YEAR);
    if (roundNumber(years / 4)) {
      msInYear = MILLISECONDS_A_DAY * 365.25;
      msInMonth = msInYear / 12;
    }

    this.$d.years = roundNumber($ms / msInYear)
    $ms %= msInYear
    this.$d.months = roundNumber($ms / msInMonth)
    $ms %= msInMonth
    this.$d.days = roundNumber($ms / MILLISECONDS_A_DAY)
    $ms %= MILLISECONDS_A_DAY

@iamkun
Copy link
Owner

iamkun commented Jul 4, 2023

can you fix the CI as well pls?

expect(d.asWeeks()).toBe(12.142857142857142) // moment 12.285714285714286
expect(d.asDays()).toBe(85.83333333333333) // moment 86, count 2M as 61 days
expect(d.asWeeks()).toBe(12.261904761904763) // moment 12.285714285714286
expect(d.asMonths()).toBe(2.8219178082191783) // moment 2.8213721020965523
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be better to not have different results from moment.js.

This will cause a huge problem during migration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

already a huge difference with moment.js right now, after my changes it's very close to moment.js result, you can check.

e.g.
existing code
days --> 85 --> moment.js 86
weeks --> 12.142857142857142 --> moment.js 12.285714285714286
months --> 2.8333333333333335 --> moment.js 2.8213721020965523

with my changes (improved and close to moment.js now)
days --> 85.83333333333333
weeks --> 12.261904761904763
months --> 2.8219178082191783

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems you are correct

@codecov
Copy link

codecov bot commented Jul 9, 2023

Codecov Report

Merging #2362 (57cc7e0) into dev (df49762) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               dev     #2362   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          183       183           
  Lines         2111      2198   +87     
  Branches       555       593   +38     
=========================================
+ Hits          2111      2198   +87     
Impacted Files Coverage Δ
src/index.js 100.00% <100.00%> (ø)
src/plugin/duration/index.js 100.00% <100.00%> (ø)
src/plugin/minMax/index.js 100.00% <100.00%> (ø)
src/plugin/objectSupport/index.js 100.00% <100.00%> (ø)

@iamkun iamkun merged commit f0a0b54 into iamkun:dev Jul 9, 2023
@iamkun
Copy link
Owner

iamkun commented Jul 9, 2023

THX

@sanjaynishad sanjaynishad deleted the fix-duration-month-calculation branch July 10, 2023 19:49
github-actions bot pushed a commit that referenced this pull request Sep 19, 2023
## [1.11.10](v1.11.9...v1.11.10) (2023-09-19)

### Bug Fixes

* Add Korean Day of Month with ordinal ([#2395](#2395)) ([dd55ee2](dd55ee2))
* change back fa locale to the Gregorian calendar equivalent ([#2411](#2411)) ([95e9458](95e9458))
* duration plugin - MILLISECONDS_A_MONTH const calculation ([#2362](#2362)) ([f0a0b54](f0a0b54))
* duration plugin getter get result  0 instead of undefined ([#2369](#2369)) ([061aa7e](061aa7e))
* fix isDayjs check logic ([#2383](#2383)) ([5f3f878](5f3f878))
* fix timezone plugin to get correct locale setting ([#2420](#2420)) ([4f45012](4f45012))
* **locale:** add meridiem in `ar` locale ([#2418](#2418)) ([361be5c](361be5c))
* round durations to millisecond precision for ISO string ([#2367](#2367)) ([890a17a](890a17a))
* sub-second precisions need to be rounded at the seconds field to avoid adding floats ([#2377](#2377)) ([a9d7d03](a9d7d03))
* update $x logic to avoid plugin error ([#2429](#2429)) ([2254635](2254635))
* Update Slovenian locale for relative time ([#2396](#2396)) ([5470a15](5470a15))
* update uzbek language translation ([#2327](#2327)) ([0a91056](0a91056))
@github-actions
Copy link

🎉 This PR is included in version 1.11.10 🎉

The release is available on:

Your semantic-release bot 📦🚀

BePo65 pushed a commit to BePo65/dayjs that referenced this pull request Sep 20, 2023
## [1.11.10](iamkun/dayjs@v1.11.9...v1.11.10) (2023-09-19)

### Bug Fixes

* Add Korean Day of Month with ordinal ([iamkun#2395](iamkun#2395)) ([dd55ee2](iamkun@dd55ee2))
* change back fa locale to the Gregorian calendar equivalent ([iamkun#2411](iamkun#2411)) ([95e9458](iamkun@95e9458))
* duration plugin - MILLISECONDS_A_MONTH const calculation ([iamkun#2362](iamkun#2362)) ([f0a0b54](iamkun@f0a0b54))
* duration plugin getter get result  0 instead of undefined ([iamkun#2369](iamkun#2369)) ([061aa7e](iamkun@061aa7e))
* fix isDayjs check logic ([iamkun#2383](iamkun#2383)) ([5f3f878](iamkun@5f3f878))
* fix timezone plugin to get correct locale setting ([iamkun#2420](iamkun#2420)) ([4f45012](iamkun@4f45012))
* **locale:** add meridiem in `ar` locale ([iamkun#2418](iamkun#2418)) ([361be5c](iamkun@361be5c))
* round durations to millisecond precision for ISO string ([iamkun#2367](iamkun#2367)) ([890a17a](iamkun@890a17a))
* sub-second precisions need to be rounded at the seconds field to avoid adding floats ([iamkun#2377](iamkun#2377)) ([a9d7d03](iamkun@a9d7d03))
* update $x logic to avoid plugin error ([iamkun#2429](iamkun#2429)) ([2254635](iamkun@2254635))
* Update Slovenian locale for relative time ([iamkun#2396](iamkun#2396)) ([5470a15](iamkun@5470a15))
* update uzbek language translation ([iamkun#2327](iamkun#2327)) ([0a91056](iamkun@0a91056))
BePo65 pushed a commit to BePo65/dayjs that referenced this pull request Sep 23, 2023
## [1.11.10](iamkun/dayjs@v1.11.9...v1.11.10) (2023-09-19)

### Bug Fixes

* Add Korean Day of Month with ordinal ([iamkun#2395](iamkun#2395)) ([dd55ee2](iamkun@dd55ee2))
* change back fa locale to the Gregorian calendar equivalent ([iamkun#2411](iamkun#2411)) ([95e9458](iamkun@95e9458))
* duration plugin - MILLISECONDS_A_MONTH const calculation ([iamkun#2362](iamkun#2362)) ([f0a0b54](iamkun@f0a0b54))
* duration plugin getter get result  0 instead of undefined ([iamkun#2369](iamkun#2369)) ([061aa7e](iamkun@061aa7e))
* fix isDayjs check logic ([iamkun#2383](iamkun#2383)) ([5f3f878](iamkun@5f3f878))
* fix timezone plugin to get correct locale setting ([iamkun#2420](iamkun#2420)) ([4f45012](iamkun@4f45012))
* **locale:** add meridiem in `ar` locale ([iamkun#2418](iamkun#2418)) ([361be5c](iamkun@361be5c))
* round durations to millisecond precision for ISO string ([iamkun#2367](iamkun#2367)) ([890a17a](iamkun@890a17a))
* sub-second precisions need to be rounded at the seconds field to avoid adding floats ([iamkun#2377](iamkun#2377)) ([a9d7d03](iamkun@a9d7d03))
* update $x logic to avoid plugin error ([iamkun#2429](iamkun#2429)) ([2254635](iamkun@2254635))
* Update Slovenian locale for relative time ([iamkun#2396](iamkun#2396)) ([5470a15](iamkun@5470a15))
* update uzbek language translation ([iamkun#2327](iamkun#2327)) ([0a91056](iamkun@0a91056))
ohsory1324 pushed a commit to ohsory1324/dayjs that referenced this pull request Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants