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

dayjs().add(Duration) API #1093

Closed
akoskm opened this issue Sep 29, 2020 · 5 comments · Fixed by #1099
Closed

dayjs().add(Duration) API #1093

akoskm opened this issue Sep 29, 2020 · 5 comments · Fixed by #1099
Labels
enhancement New feature or request good first issue Good for newcomers released

Comments

@akoskm
Copy link

akoskm commented Sep 29, 2020

Hello, I'm replacing moment.js with dayjs in a project.

I'm not sure if the APIs are not compatible at this point or I'm making a mistake here:

// the old code basically had `moment` where you see dayjs now

const date1 = dayjs('2018-04-04T16:00:00.000Z')
const date2 = dayjs();
const duration = dayjs.duration(dayjs().diff(date1));
const date3 = date2.add(duration);
console.log(date3);

the output of the above code is:

    d {
      '$L': 'en',
      '$u': undefined,
      '$d': Invalid Date,
      '$x': {},
      '$y': NaN,
      '$M': NaN,
      '$D': NaN,
      '$W': NaN,
      '$H': NaN,
      '$m': NaN,
      '$s': NaN,
      '$ms': NaN
    }

Looking at the implementation of add:

dayjs/src/index.js

Lines 220 to 221 in f95ac15

add(number, units) {
number = Number(number) // eslint-disable-line no-param-reassign

it always expects the first parameter to be a number so that could one reason why this isn't working.

In moment.js we could add a Duration to a Moment object: https://momentjs.com/docs/#/manipulating/add/

One workaround would be to convert the duration to milliseconds and add it to the date2:

const date1 = dayjs('2018-04-04T16:00:00.000Z')
const date2 = dayjs();
const duration = dayjs.duration(dayjs().diff(date1)).asMilliseconds();;
const date3 = date2.add(duration, 'ms');
@iamkun
Copy link
Owner

iamkun commented Sep 29, 2020

Day.js does not support adding Duration at present.

However, we could support it in the Duration plugin if needed.

@iamkun iamkun added enhancement New feature or request good first issue Good for newcomers labels Sep 29, 2020
@iamkun
Copy link
Owner

iamkun commented Sep 29, 2020

This will be a nice feature added to the Duration plugin.

It's much like the workaround code mentioned above.

Feel free to draft a PR if anyone is interested in it.

@iamkun
Copy link
Owner

iamkun commented Oct 13, 2020

🎉 This issue has been resolved in version 1.9.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@franky47
Copy link

franky47 commented Jan 17, 2021

Thanks for this feature !

There are issues with the calculation though:

dayjs('2020-01-01').add(1, 'month') // 2020-02-01 ✅
dayjs('2020-01-01').add(dayjs.duration(1, 'month')) // 2020-01-31 ❌

Reproduction example:
https://runkit.com/franky47/600431082af3c5001a19f25d

This is because the add method uses the duration in milliseconds, which has no notion of context (years, months, days and minutes have variable numbers of milliseconds in them).

@mi-na-bot
Copy link

@iamkun I am finding that in 1.11.9 this does not work, with the same invalid date output. It might be a regression!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants