-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
addMonths is coming up 1 hour short #571
Comments
Thank you for reporting, I'll take a look! |
Could you tell me what's your local time zone? |
No problem. My timezone is currently BST (UTC +1). Should timezones affect the results if I'm comparing two UTC strings? Edit: Edit2: It would be difficult to know when to apply an offset. Some months are working correctly without an offset but March isn't |
It may affect the result as date-fns (as well as JavaScript) converts dates to the local TZ. You can avoid it by doing a trick: new Date('2017-03-01T00:00:00.000Z'.split('T')[0]) This way JS won't convert time zones and it will always product 1st of March regardless of the current time setup. If that was the problem, please close the issue as a proper solution for it is coming with UTC release (no ETA at the moment): #376 If the problem is DST-related (unlikely, but I have to check it first), I'll fix it. |
Ah unfortunately in that case I think it may be DST related. |
Just done some more testing on this. I made a range of dates using each day and you can see where the timezone changes from the offset. Anyone reading this make sure you don't adjust it by the current offset. Needs to be the computed one at that date.
|
Any updates on this issue? |
The same thing happens in Safari and older version of Chrome/Firefox with
Returns |
This is definitely DST related but only when the DST transition happens during midnight, which is the case in Brazil. Screenshots bellow are from latest Safari with my system configured to use BRT timezone. November 4th is when Brazil transitions to DST. Latest Chrome and Firefox both return Any function using
Safari seems to be the only browser where this is still a problem. |
I did some more testing and Ex:
|
A couple more information, Few examples:
The way we ended up fixing it internally was mostly not using
I have also filed a Webkit but about this: https://bugs.webkit.org/show_bug.cgi?id=188001 |
On investigating this I wrote a small function that might solve this issue. Has not been tested thoroughly, so use with caution: // Removed the dirtyDate / dirtyAmount just to test easier in isolation
function agnosticAddMonths(date, amount) {
const originalTZO = date.getTimezoneOffset();
const endDate = addMonths(date, amount);
const endTZO = endDate.getTimezoneOffset();
const dstDiff = originalTZO - endTZO;
return dstDiff ? addMinutes(endDate, dstDiff) : endDate;
} You can see the difference in the last console log, where the isoString is preserving the original UTC 00:00:00 time. This could allow us to write TZ agnostic tests (passing UTC values, expecting UTC values) that can run in your local TZ (if set) or UTC. The "issue" lies in https://github.com/date-fns/date-fns/blob/master/src/addMonths/index.js#L37, as that newly created date won't be in UTC even though you passed an UTC original date. And, as expected, |
The primary reason folks resort to a library to handle date math is because date math is not straight forward. It has many exceptions and irregularities. If it didn't no one would think to use a library when they already have math functions. There is also the issue of governments changing things up. Although, it is a finite list. Therefore, creating a library to tackle it makes sense. However, when the library doesn't fix key things like daylight savings time handling, they are voiding the primary reason you choose to use the library. |
This seemed to have worked, thanks. But I hope that this issue should be fixed soon. |
If you set your time zone to UTC in your environment you won't have to worry about DST. |
@ovcOS what if I don’t know the timezone and I don’t care about it? Which one should I set? My questions are rhethorical. If you want to be timezone agnostic this does not help. If you want your code to work a fixed timezone per env, it will. |
@GonchuB as I understand, UTC is time zone-agnostic, therefore you don't need to know your TZ. With the command above you are disregarding any local TZs and setting your time to universal |
@ovcOS no, that is the whole point. Check the difference between addMonths and agnosticAddMonths #571 (comment) |
@GonchuB I've seen the difference. I believe you may be missing the point. UTC does not care about time zones or daylight savings. Therefore, setting your machine and server to UTC will disregard these issues. It works for me. Anyone else struggling with this should give it a try. |
@ovcOS what if we run in the browser, I cannot enforce UTC (or makes no sense for a test to run enforcing UTC). And the outcome of addMonths would be wrong, as it WILL take TZ into consideration for users |
I think there are two unrelated issues being discussed here. The original problem being discussed is DST related, is only timezone related because it only happens in timezones where DST change is at midnight. It will indeed not happen if one uses UTC but that is not the point, some people need to use "timezoned" dates. Also, converting back from UTC to a date with timezone will incur in the same problem. The last thing to keep in mind is that this is a browser issue (https://bugs.webkit.org/show_bug.cgi?id=188001) and so a fix here would be just to get around that bug. I've posted this before but moment had the same problem and you can see how they fixed it here https://github.com/moment/moment/pull/4338/files |
@GonchuB Thank you for the workaround! |
Fixed function agnosticAddDays(date, amount) {
const originalTZO = date.getTimezoneOffset();
const endDate = dateFns.addDays(date, amount);
const endTZO = endDate.getTimezoneOffset();
const dstDiff = originalTZO - endTZO;
return dstDiff >= 0
? dateFns.addMinutes(endDate, dstDiff)
: dateFns.subMinutes(endDate, Math.abs(dstDiff));
} |
@bertho-zero thank you for your work around. It works like a charm. It's sad that a bug that is so fundamental and critical can be left unresolved for so long. :( It seems like Fix DST issues with add, addDays and addMonths (#1760) (closes #1682) is working in this area, however it didn't fix this issue. |
Also experiencing the problem here :) So, this thing is not going to be fixed? (i probably understanding why since the Date is exactly the same but timezone changes) First of all, browser timezone is +3 (Moscow) and ama using I am trying to:
And getting an unexpected value
instead of:
p.s. |
I just experienced the same issue as stated in the very first post, except with
Solution: Scrap date-fns and use vanilla JS:
Note that before you call |
When falling back, a bug in date-fns[^0] results in the 23 hour day being skipped when building the `dates` array in `toChartData` if the current time is between 00:00 and 01:00 UTC. This results in that day's data essentially going missing, resulting in the subtly broken charts noted in rust-lang#5477. This commit adds a workaround adapted from a comment on date-fns/date-fns#571[^1] by @bertho-zero, which correctly adjusts the new date based on the time zone offsets, and means that `dates` is built as expected. Fixes rust-lang#5477. [^0]: date-fns/date-fns#571 [^1]: date-fns/date-fns#571 (comment)
When falling back, a bug in date-fns[^0] results in the 23 hour day being skipped when building the `dates` array in `toChartData` if the current time is between 00:00 and 01:00 UTC. This results in that day's data essentially going missing, resulting in the subtly broken charts noted in #5477. This commit adds a workaround adapted from a comment on date-fns/date-fns#571[^1] by @bertho-zero, which correctly adjusts the new date based on the time zone offsets, and means that `dates` is built as expected. Fixes #5477. [^0]: date-fns/date-fns#571 [^1]: date-fns/date-fns#571 (comment)
I'm experiencing this for |
Same issue. For a date with day |
@kossnocorp @leshakoss would you accept PR(s) to allow DST-safe operations either by A) adding new functions or B) adding options to existing functions @bertho-zero would you be open to creating such a PR? A TypeScript version of import { addDays, addMinutes, addMonths, addWeeks } from 'date-fns';
export function addDaysDstSafe(date: Date, amount: number) {
const endDate = addDays(date, amount);
return addMinutes(
endDate,
date.getTimezoneOffset() - endDate.getTimezoneOffset(),
);
}
export function addWeeksDstSafe(date: Date, amount: number) {
const endDate = addWeeks(date, amount);
return addMinutes(
endDate,
date.getTimezoneOffset() - endDate.getTimezoneOffset(),
);
}
export function addMonthsDstSafe(date: Date, amount: number) {
const endDate = addMonths(date, amount);
return addMinutes(
endDate,
date.getTimezoneOffset() - endDate.getTimezoneOffset(),
);
} |
This should 100% be part of this library. I'm still running into this issue of DST when adding months. 2024-03-01 + one month should = 2024-04-01 |
this worked for assuming UTC dates: in my
|
Version: ^1.28.5
I would expect
addMonths(new Date('2017-03-01T00:00:00.000Z'), 1).toISOString();
to equal
'2017-04-01T00:00:00.000Z'
Instead it's coming up as
'2017-03-31T23:00:00.000Z'
The text was updated successfully, but these errors were encountered: