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

ISO 8601 parsing is not correct and erroneously uses modulo logic #4815

Closed
ipavlic opened this issue Oct 15, 2018 · 5 comments · Fixed by moment/momentjs.com#646
Closed

ISO 8601 parsing is not correct and erroneously uses modulo logic #4815

ipavlic opened this issue Oct 15, 2018 · 5 comments · Fixed by moment/momentjs.com#646

Comments

@ipavlic
Copy link

@ipavlic ipavlic commented Oct 15, 2018

Description of the Issue and Steps to Reproduce:

Given a duration >=31 days, moment.duration will incorrectly parse it with modulo logic, assuming that a month is always 31 days long.

For example, moment.duration("P32D") is converted into a 1 month 1 day moment.duration. If a duration designator is parsed in that way is added to different time instants, the absolute duration will differ, which is valid under ISO 8601. For example, adding 1 month and 1 day to February 1 is different than adding it to May 1:

  • 28+1 or 29+1 days for February
  • 31+1 days for May.

More generally, all duration elements greater than their assumed modulo are not parsed correctly. For hours, "PT36H" should not be parsed as 1 day and 12 hours, but as 36 hours, because daylight saving changes can occur.

Steps to reproduce:

var dur1 = moment.duration("P32D");
// dur.months() is 1, but should be 0
// dur.days() is 1, but should be 32

var dur2 = moment.duration("PT36H");
// dur.days() is 1, but should be 0
// dur.hours() is 12, but should be 36

Environment:

Chrome 69.0.3497.100 OSX

Mon Oct 15 2018 07:47:34 GMT-0400 (Eastern Daylight Time)
10/15/2018, 7:47:34 AM
240
Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.100 Safari/537.36
2.22.2
@guillemm
Copy link

@guillemm guillemm commented Oct 17, 2018

After looking at your issue with @sarfata, we concluded that the library behaves differently than what you expect but works as intended.

For example, adding 1 month and 1 day to February 1 is different than adding it to May 1:

28+1 or 29+1 days for February
31+1 days for May.

We found that adding P32D and P1M1D behave differently and correctly:

> moment('2018-02-01').add(moment.duration('P32D'))
moment("2018-03-05T00:00:00.000")
> moment('2018-02-01').add(moment.duration('P1M1D'))
moment("2018-03-02T00:00:00.000")

For hours, "PT36H" should not be parsed as 1 day and 12 hours, but as 36 hours, because daylight saving changes can occur.

Two functions are available, asXXX() and XXX(). The former behaves as you expect, the latter which you mention behaves differently:

> moment.duration('P32D').days()
1
> moment.duration('P32D').months()
1
> moment.duration('P32D').asDays()
32
> moment.duration('P32D').asMonths()
1.051356290683587

Notice that asMonths() returns a float.

@ipavlic
Copy link
Author

@ipavlic ipavlic commented Oct 17, 2018

You are right, durations work correctly when being added to instants and looking at the returned Duration object, it's also correct! It has appropriate time unit properties without context.

However, when functions like unit() or asUnit() are called, the output is not what would be expected by ISO 8601 nominal duration (or RFC 5545 duration), which is what is parsed.

I'll use properties directly then.

@marwahaha
Copy link
Member

@marwahaha marwahaha commented Jan 18, 2019

Please add this finding to our docs at https://github.com/moment/momentjs.com

@emer7
Copy link

@emer7 emer7 commented Feb 6, 2019

Hi, I would like to add the finding to the docs! (If it hasn't been added)

@marwahaha
Copy link
Member

@marwahaha marwahaha commented Feb 6, 2019

@emer7 Sure thing, please adjust https://github.com/moment/momentjs.com as you see fit and make a pull request. If you need help, please reach out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants