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

Duration format result is not as expected #1521

Closed
awenger63 opened this issue Jun 4, 2021 · 6 comments · Fixed by #1611
Closed

Duration format result is not as expected #1521

awenger63 opened this issue Jun 4, 2021 · 6 comments · Fixed by #1611
Labels

Comments

@awenger63
Copy link

awenger63 commented Jun 4, 2021

Describe the bug
When use duration format for an ISO8601, result it's not as expected :
myDuration = dayjs.duration('PT3M39.096S')
myDuration.format("HH:mm:ss") => "NaN:03:39.096"
myDuration.format("mm:ss") => "03:39.096"
myDuration.format("ss") => "39.096"
myDuration.format("SSS") => "undefined"

Expected behavior
myDuration.format("HH:mm:ss") => "00:03:39"
myDuration.format("mm:ss") => "03:39"
myDuration.format("ss") => "39"
myDuration.format("SSS") => "096"

Information

  • Day.js Version [1.10.5]
  • OS: [Windows]
  • Browser [chrome 90]
@awenger63
Copy link
Author

Did you take a look at this issue ? Dou you need more test case ?

@michaelnguyenm
Copy link

I'm actually having a similar issue with the above problem with regards to NaN and would like to see it fixed if possible.

Upon inspecting the code here:

if (typeof input === 'string') {
const d = input.match(durationRegex)
if (d) {
const properties = d.slice(2)
const numberD = properties.map(value => Number(value));
[
this.$d.years,
this.$d.months,
this.$d.weeks,
this.$d.days,
this.$d.hours,
this.$d.minutes,
this.$d.seconds
] = numberD
this.calMilliseconds()
return this
}
}

Which uses the regex:

const durationRegex = /^(-|\+)?P(?:([-+]?[0-9,.]*)Y)?(?:([-+]?[0-9,.]*)M)?(?:([-+]?[0-9,.]*)W)?(?:([-+]?[0-9,.]*)D)?(?:T(?:([-+]?[0-9,.]*)H)?(?:([-+]?[0-9,.]*)M)?(?:([-+]?[0-9,.]*)S)?)?$/

It appears that there are multiple issues with regards to parsing ISO 8601 duration strings. The first is that some capture groups will not always be present as defined by ISO 8601, resulting in undefined after using String.prototype.match(). These values should be converted as 0 instead of NaN when converted into a Number. This is apparent when using an input value such as PT3H19M where the seconds will be missing, so the seconds will be rendered as NaN. See:

const input = 'PT3H19M';
const regex = /^(-|\+)?P(?:([-+]?[0-9,.]*)Y)?(?:([-+]?[0-9,.]*)M)?(?:([-+]?[0-9,.]*)W)?(?:([-+]?[0-9,.]*)D)?(?:T(?:([-+]?[0-9,.]*)H)?(?:([-+]?[0-9,.]*)M)?(?:([-+]?[0-9,.]*)S)?)?$/;
const found = input.match(regex);
console.log(found);
// > Array ["PT3H19M", undefined, undefined, undefined, undefined, undefined, "3", "19", undefined]

The second issue is that the regex for seconds will capture both integers and decimals. When converted to a Number, the decimal will be kept when the duration object $d is should have separate properties for seconds and milliseconds. In other words, they should be parsed separately.

@cypressious
Copy link
Contributor

I've just hit this problem and I've found that calling duration.add(0, 'seconds') on a broken instance of Duration will return an instance where the NaN components will be replaced with 0s.

@iamkun
Copy link
Owner

iamkun commented Sep 10, 2021

🎉 This issue has been resolved in version 1.10.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

@awenger63
Copy link
Author

Hello, @cypressious & @iamkun , thanks a lot for your action, but new issue #1665 is open for complet solve this
For help improvement ;-)

@adroste
Copy link

adroste commented Dec 27, 2021

@iamkun I'm on 1.10.7 and this is definitely not fixed. See #1665

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