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.valueOf() shall return number of milliseconds #1274

Open
chengB12 opened this issue Dec 15, 2020 · 3 comments
Open

duration.valueOf() shall return number of milliseconds #1274

chengB12 opened this issue Dec 15, 2020 · 3 comments

Comments

@chengB12
Copy link

Describe the bug
javascript convention is valueOf() shall return comparable value

const d1 = dayjs.duration(1, 'second')
const d2 = dayjs.duration(2, 'second')
console.log(d2 > d1)

Expected behavior
above script shall print true, however, false would be printed

Information

  • Day.js Version [e.g. v1.9.7]
  • OS: [windows 10]
  • Browser [node.js 14.15.0]
  • Time zone: [e.g. GMT-07:00 DST (Pacific Daylight Time)]
@franky47
Copy link

This would be problematic for some cases, where additional context is needed:

// Months have 28 to 31 days
dayjs.duration(1, 'month').valueOf()

// Minutes can include leap seconds
dayjs.duration(1, 'minute').valueOf()

// Years can include leap days
dayjs.duration(1, 'year').valueOf()

@chengB12
Copy link
Author

chengB12 commented Jan 19, 2021

if dayjs.duration(1, 'month').valueOf() has problem, dayjs.duration(1, 'month').asMilliseconds() would have same problem. So I didn't see where is problem besides apparently breaking change

@addisonElliott
Copy link

I agree with adding a valueOf() function to duration to facilitate comparisons between durations. I think it makes the most sense to return the number of milliseconds since that's how the durations are primarily stored/handled under-the-hood.

Moment's duration has a valueOf() that returns the number of milliseconds.

@franky47 Regarding your comment, I don't think the valueOf() function is problematic. Rather, the constructor is problematic. The Day.js duration stores a milliseconds value and then calculates the various units from that (e.g. years, months, days, minutes). If you call duration.asMilliseconds(), the value is constant. For example, I believe months are assumed to be 30 days and the milliseconds is calculated from that.

Even Moment.js has this issue with it's duration. See here for more information: https://momentjs.com/docs/#/durations/. They make a reasonable assumption as to the length of a month & year and go from there.

If anyone is interested, here's code for extending the Day.js duration to add a valueOf() and equals() function. Written in Typescript.

// What a wonderful hack because Day.js decides NOT to export the Duration class
const Duration = dayjs.duration(0, 'ms').constructor;

Duration.prototype.equals = function(other: dayjs.Duration) {
    return this.$ms === (other as any).$ms;
};

// valueOf is used for comparison between durations
Duration.prototype.valueOf = function() {
    // Return total number of ms
    return this.$ms;
};

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

No branches or pull requests

3 participants