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

Manipulating days with decimal numbers ignores hours #2013

Open
sroettering opened this issue Aug 5, 2022 · 8 comments
Open

Manipulating days with decimal numbers ignores hours #2013

sroettering opened this issue Aug 5, 2022 · 8 comments

Comments

@sroettering
Copy link

Describe the bug
When manipulating a date with unit type day and an arbitrary decimal number does not manipulate the time of the date.

Expected behavior
The time is manipulated as well.

Information
Stackblitz reproduction:
https://stackblitz.com/edit/js-akaprq?file=index.js

@Bykiev
Copy link

Bykiev commented Aug 5, 2022

Hi, the moment.js behaviour is the same, it seems to be a lack of docs in dayjs library:

Moment.js docs:
As of 2.12.0 when decimal values are passed for days and months, they are rounded to the nearest integer. Weeks, quarters, and years are converted to days or months, and then rounded to the nearest integer.

Here you can find more info: moment/moment#2430 (comment)

Upd: strange behaviour with subtracting 1.5 day, the date should be 2 days ago. Seems to be a bug for me if my assumes with day rounding are correct.

return Utils.w(d.date(d.date() + Math.round(n * number)), this)

-1.5 is rounded to -1 and 1.5 is rounded to 2

Momentjs is using absolute rounding, that's why result is different.

@BePo65
Copy link
Contributor

BePo65 commented Aug 6, 2022

Seems to be a bug for me if my assumes with day rounding are correct.

So it looks to me. To my knowledge, rounding to the nearest integer for negative numbers is done with the absolute value (and adding the sign again after rounding).
Besides this, in moment.js floating point numbers for days and months are rounded to the nearest integer, while in days.ja days and weeks are rounded, while months and years are just added. IMHO regarding this point, moment and dayjs should behave the same.

How about a PR?

I will do a PR for the documentation as the rounding to the nearest integer (as moment does) should / must be part of the documentation.

@Bykiev
Copy link

Bykiev commented Aug 6, 2022

I think we should wait @iamkun to discuss this behaviour, what do you think about it? I can create PR for this if we come to common decision about dayjs behaviour

@BePo65
Copy link
Contributor

BePo65 commented Aug 6, 2022

Of course you are right. IMO one of the basic definitions for dayjs is that it behaves just like moment (ok, there are a few exceptions :-). In this regard this is at least another difference.

But as this would be a breaking change (if not of the documentation that says nothing about it, so at least regarding the functionality), perhaps this should become an action point for dayjs 2.0.

@iamkun is there a place, where those requirements for dayjs 2.0 could be documented?

PS: @Bykiev you are pretty fast responding to issues 🥇 👍 😄

@Bykiev
Copy link

Bykiev commented Aug 6, 2022

But as this would be a breaking change (if not of the documentation that says nothing about it, so at least regarding the functionality), perhaps this should become an action point for dayjs 2.0.

Totally agree, this change is better suited for dayjs 2.0

PS: @Bykiev you are pretty fast responding to issues 🥇 👍 😄

Yeah, recently we've migrated our project from moment.js to dayjs. We've spent many times analysing open issues to discover the possible issues (still waiting response from @iamkun in #1921). That's why I've fresh knowledges about it and I like to help new users to use this library.

@sroettering
Copy link
Author

Thanks for your elaborations. I agree that the docs need to at least warn the user about this behavior. I can also understand why you want to behave dayjs the same as momentjs but at some point you have to consider removing bugs that moment had and I would consider this clearly as a bug since most other units are able to subtract/add decimal values. A fix in dayjs 2.0 would be OK. We implemented a workaround for now.

@Bykiev
Copy link

Bykiev commented Aug 8, 2022

Thanks for your elaborations. I agree that the docs need to at least warn the user about this behavior. I can also understand why you want to behave dayjs the same as momentjs but at some point you have to consider removing bugs that moment had and I would consider this clearly as a bug since most other units are able to subtract/add decimal values. A fix in dayjs 2.0 would be OK. We implemented a workaround for now.

It's not a bug in moment.js, see explanation here: moment/moment#2430 (comment).

@Nantris
Copy link

Nantris commented Nov 19, 2022

Is this planned for 2.x? Partial unit manipulations silently fail as if the user only passed in an integer and not a float.

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

4 participants