Wrong date manipulation when date passes through tz offset change

[moment]

Wrong date manipulation when date passes through tz offset change

[mattjohnsonpint]

@timrwood - Do we consider a "day" in this context as a calendar day for the target zone? Or is it bound to the runtime's local zone?

I hope it's not just 24 fixed hours. :)

[timrwood]

Days should be considered calendar days, so adding one day should result in the same hour but a different offset. This looks like it is a bug.

Adding 24 hours is handled differently than days, and may result in a different hour if the offset is different.

What is really strange about this bug is that the total time added is only one hour, not 23 or 25 hours as I would expect.

I think this is likely a bug somewhere in the addOrSubtractDurationFromMoment function in moment core, but will have to look into it.

[mattjohnsonpint]

That sounds right, but when moment-timezone is present we should be sure we are moving through days in the target zone's calendar.

I would guess that moment is normally using the local zone provided via the Date class.

I haven't dug in enough to see if moment-timezone overrides the moment default behavior in this regard.

[zgmnkv]

As far as I understand, moment-timezone does not override moment default function, we need to call .tz() function to set timezone, so internally created moment objects have local timezone, that can cause bugs.
Moreover, moment-timezone can not override moment default function that is used internally, because it is a local variable inside anonymous function, so we need some support from moment project.

As for this concrete bug, moment logic is like 'save hours and minutes, add 1 day, set hours and minutes back'.
In combination with updateOffset() on every moment change, it causes the following flow:

1. save hours and minutes (hours = 0, minutes = 0, moment = '2012-10-28T00:00:00+01:00')
2. set date = date + 1, (moment = '2012-10-29T00:00:00+01:00')
   2.1. updateOffset() (moment = '2012-10-28T23:00:00+00:00')
3. set minutes = saved minutes (moment = '2012-10-28T23:00:00+00:00')
   3.1. updateOffset() (moment = '2012-10-28T23:00:00+00:00')
4. set hours = saved hours (moment = '2012-10-28T00:00:00+00:00')
   4.1. updateOffset() (moment = '2012-10-28T01:00:00+01:00')

[zgmnkv]

I think that saving hours and minutes and setting them back is a hacky way.
Maybe we can save tz offset before date change, check if offset changed after days (weeks, months, years) addition (subtraction) and then add offsets difference in this case.

[zgmnkv]

Same shit with startOf(), endOf(), month() functions. I think all manipulation functions that need to change date preserving time works wrong.

[mattjohnsonpint]

This is proabably also effected by moment #831

[zgmnkv]

I have some progress with this issue, the following code seems to fix the issue at least for add/subtract/startOf/endOf day/month.
I overrode 'date' and 'startOf' functions and made time correction based on offsets difference.
'month' function is not overriden because it uses 'date' function inside, and 'endOf' function uses 'startOf' function, so it is not overriden too.

var oldDateFunction = moment.fn.date;
moment.fn.date = function() {
    if (arguments.length >= 1) {
        var oldOffset = this.zone();
        var result = oldDateFunction.apply(this, arguments);
        var newOffset = this.zone();
        this.add('minutes', newOffset - oldOffset);//restore proper time
        return result;
    } else {
        return oldDateFunction.apply(this, arguments);
    }
};
moment.fn.dates = moment.fn.date;

var oldStartOfFunction = moment.fn.startOf;
moment.fn.startOf = function(units) {
    if (units === 'day' || units === 'days' || units === 'd') {
        var oldOffset = this.zone();
        var result = oldStartOfFunction.apply(this, arguments);
        var newOffset = this.zone();
        this.add('minutes', newOffset - oldOffset);//restore proper time
        return result;     
    } else {
        return oldStartOfFunction.apply(this, arguments);
    }
}

See http://jsfiddle.net/27ytF/4/ for example and tests.

[joshuabambrick]

I haven't really looked at your solution but for anyone else who comes here, you will probably want to add in || units === 'd'. Or change the whole thing to /^d(ays?)?$/.test(units);.

[icambron]

Just chiming in with my two cents. Let's make sure we're solving the root problem. As I understand it, the underlying issue is that date() isn't doing its job here. From @zgmnkv's very nice sequence writeup:

2. set date = date + 1, (moment = '2012-10-29T00:00:00+01:00')
2.1. updateOffset() (moment = '2012-10-28T23:00:00+00:00')

That should be '2012-10-29T00:00:00+00:00', not '2012-10-28T23:00:00+00:00'. That's just straight-up off by an hour. The thing about saving the hours and restoring them certainly exacerbates the problem; if I understand its purpose correctly (big if), then it's actually trying to adjust for the ill effects of updateZone messing with the hour, but here it screws things up by applying itself to the wrong date. I submit that we should just fix date(), i.e. updateZone should just get things right. Then the hour/minute stuff would just be a no-op, and we could get rid of it. Otherwise, we'll be fighting battles all over moment's codebase to compensate for the fact that date() just doesn't work like it's supposed to, like those startOf/endOf fixes above.

My proposal is to change updateOffset so that if we're changing a date or month, just to change the offset without touching the time. Just spitballing, but something like:

moment.updateOffset = function (mom, dontAdjustHours) {
    var offset;
    if (mom._z) {
        offset = mom._z.offset(mom);
        if (dontAdjustHours) {
            mom._offset = offset;
        }
        else {
        if (Math.abs(offset) < 16) {
            offset = offset / 60;
        }
        mom.zone(offset);
        }
    }
};

And then pass in that variable from moment where appropriate. I haven't tested any of that, but that's the sort of place the fix should be.

[zgmnkv]

@joshbambrick, you're right, thanks for the correction.
I agree with @icambron, this solution looks good. Btw, shouldn't there be offset = offset * 60; instead of offset = offset / 60;? Or maybe we can remove this block because moment.js interprets offset as hours if it is < 16 and > -16.

[icambron]

@zgmnkv I just pasted that from the current code. But I agree it looks wrong and that moment's zone() handles the < 16 case anyway.

[joshuabambrick]

So have you reached a formal conclusion to this? This is quite a big issue, and it would be good to see some changes to the actual source code.

[icambron]

@joshbambrick - I think the next step is a pull request. Since I don't actually use the moment-timezone project (I'm just here to help a bit by virtue of my involvement with Moment itself), it's not going to be me.

[kirknorthrop]

Just for reference (as I have just discovered the same problem) this also happens with the isBefore/isSame/isAfter trio.

[bantic]

I noticed an issue with startOf and endOf that is related to this.
Asking for endOf('day') for the date Sun Mar 10 2013 23:59:59 GMT-0400 (EDT) gives 22:59:59 of the following day (23 hours later than expected). And asking for startOf('day') for the same date gives 23:00:00 of the previous day (1 hour earlier than expected).

More detail in this gist: https://gist.github.com/bantic/a0801aa2f5fccb9bb8e0

[eipark]

Not positive, but I believe this is related:

# Trying to get the timestamp for the beginning of 3/9/14, the DST day in Boston
moment().tz("US/Eastern").year(2014).month(2).date(9).hour(0).minute(0).second(0).millisecond(0).format() 
# output => "2014-03-08T23:00:00-05:00"

US/Eastern changes from UTC -5 to UTC -4 at 2am on 3/9, but I'd expect this to still give me 2014-03-09T00:00:00-05:00.

[jessehouchins]

I see a fix mentioned here is now in the dev branch... does that mean we can expect this out soon? Any idea when the next release will be?

[timrwood]

This is fixed in the 0.1.0 release.

var mom = moment('2012-10-28 00:00:00+01:00').tz('Europe/London');
console.log(mom.format()); // 2012-10-28T00:00:00+01:00
mom.add('days', 1);
console.log(mom.format()); // 2012-10-29T00:00:00+00:00

@eipark, your example is fixed as well.

moment().tz("US/Eastern").year(2014).month(2).date(9).hour(0).minute(0).second(0).millisecond(0).format();
// 2014-03-09T00:00:00-05:00