moment / moment-timezone

Timezone support for moment.js
momentjs.com/timezone
MIT License
3.82k stars 835 forks source link

Explicit time zones break utcOffset(..., true) #187

Open petebrowne opened 9 years ago

petebrowne commented 9 years ago

I'm trying to use the utcOffset with the keepLocalTime parameter as true. It looks like if you use an explicit time zone when creating the object, it sets the the internal property _isUTC to true, which breaks the utcOffset.

var m1 = moment('2014-11-21T10:30:00-06:00');
moment(m1).utcOffset('UTC', true).format(); // 2014-11-21T10:30:00+00:00
m1._isUTC; // false

var m2 = moment.tz('2014-11-21T10:30:00', 'America/Chicago');
moment(m2).utcOffset('UTC', true).format(); // 2014-11-21T16:30:00+00:00
m2._isUTC; // true ??
mattjohnsonpint commented 9 years ago

The isUtc() function still returns the correct result. The general guidance is to not use the underscore-prefixed properties externally, as many of the moment functions combine multiple properties to achieve the correct result.

Also, by wrapping the moment in another moment, you're cloning it, so the second line of each block isn't manipulating anything other than the moment used by the format call.

That said, I actually don't get the same results as you showed. In the second example, I get "2014-11-21T04:30:00-06:00" - which kept the original offset and did shift the time. So I think there's still a bug here.

@timrwood any ideas?

petebrowne commented 9 years ago

Yeah I get 2014-11-21T04:30:00-06:00 now, very odd...could DST changes affect this as well?

Just to clarify: I'm not using _isUTC, I just noticed the flag was different while debugging

timrwood commented 9 years ago

The value passed to utcOffset should be a number or offset string, not a timezone name. utcOffset(0, true) or utcOffset('+00:00', true).

petebrowne commented 9 years ago

I still get inconsistent results with 0 instead of 'UTC'

var m1 = moment('2014-11-21T10:30:00-06:00');
m1.utcOffset(0, true).format();
// "2014-11-21T10:30:00+00:00"

var m2 = moment.tz('2014-11-21T10:30:00', 'America/Chicago');
m2.utcOffset(0, true).format();
// "2014-11-21T04:30:00-06:00"
timrwood commented 9 years ago

Ok, I think I see what is going on here.

While utcOffset is being called, it calls moment.updateOffset to keep the offset in sync over dst changes. Because the moment is still in America/Chicago it's updating its offset back to Chicago time.

What you should do is remove the timezone from the moment and then set the offset. Right now there is no official way to remove a timezone, but you could do m2._z = null; m2.utcOffset(0, true).format();.

It is kinda weird to parse into a timezone, then switch to another offset while keeping the same time. Why not just do moment.utc('2014-11-21T10:30:00') instead?

petebrowne commented 9 years ago

I'm receiving a datetime from an API in UTC and converting it to a user's timezone for display. That seems like a pretty valid use-case?

timrwood commented 9 years ago

If you want to display a utc datetime in local time you can do this.

moment.utc('2014-11-21T10:30:00').local().format() // 2014-11-21T04:30:00-06:00

If you want to display a utc datetime in a specific timezone you can do this.

moment.utc('2014-11-21T10:30:00').tz('America/Los_Angeles').format() // 2014-11-21T02:30:00-08:00

Maybe I'm missing the use case, but do those help?

petebrowne commented 9 years ago

The problem I have is that I can't guarantee the original timestamp is UTC, which is why I'm initializing with moment.tz instead of moment.utc

ashleydw commented 8 years ago

I think I have the same problem here. I am creating a moment object from a utc timestamp, and setting the timezone of the object. However, when returning utcOffset() the offset is always my local offset, not the parsed timezone.

// 1441955500 = 2015-09-11T07:11:40
// Melbourne is +10 hrs
var mo = moment.utc(1441955500, 'X');
mo.tz('Australia/Melbourne');
console.log(mo.format(), mo.format('Z'), mo.utcOffset());

Gives: 2015-09-11T17:11:40+10:00 +10:00 600

So the timezone is set correctly, but utcOffset() returns 600 - my local offset.

mattjohnsonpint commented 8 years ago

@ashleydw - utcOffset() returns a number in terms of minutes East of UTC. 60 minutes in an hour, so 600 / 60 = 10 hours East of UTC, which we show in string format as +10:00.

ashleydw commented 8 years ago

sorry my fault! I was reading it incorrectly

wyefei commented 7 years ago

moment('2017-08-18T20:30:00-04:00').utcOffset() returns -420. Not sure whether this is a bug or I am misunderstanding it. Shouldn't it return -240 instead?

mattjohnsonpint commented 6 years ago

@wyefei - in the example you gave, the offset is your local offset, not the one passed in. You'd have to use moment.parseZone(...) to retain the offset passed in.

mattjohnsonpint commented 6 years ago

To be clear, the bug being tracked in this issue is related to calling utcOffset with the true second parameter (to keep the local time) as shown here, should clear any time zone set on the moment object, and it is not. If you're not passing true, or your not using moment-timezone, this is not the bug for you. 😄

This should be relatively easy to fix, so marking up for grabs.

jonseymour commented 5 years ago

For reference, here is a similar example, although with the false parameter.

var moment = require('moment-timezone');
m0 = moment.tz("2019-01-08 11:55", "Australia/Sydney");
m1 = m0.clone().add(6, 'months')
m2 = m0.clone().utcOffset(m1.utcOffset(), false)

m3 = moment("2019-01-08 11:55:00+11:00")
m4 = m3.clone().add(6, 'months')
m5 = m3.clone().utcOffset(m4.utcOffset(), false)

m6 = m2.clone().utc().utcOffset(m1.utcOffset(), false)

vars = [ m0, m1, m2, m3, m4, m5, m6 ]
for (var i in vars) {
    m = vars[i]
    console.log(i, ":", m.toString(), m.utcOffset(), m.isDST(), m.unix())
}
if (m5.toString() != m2.toString()) {
    console.log("assertion failed: display string (2) is showing wrong offset compared to (5)")
}

which results in this output:

0 : Tue Jan 08 2019 11:55:00 GMT+1100 660 true 1546908900
1 : Mon Jul 08 2019 11:55:00 GMT+1000 600 false 1562550900
2 : Tue Jan 08 2019 11:55:00 GMT+1100 600 false 1546908900
3 : Tue Jan 08 2019 11:55:00 GMT+1100 660 true 1546908900
4 : Mon Jul 08 2019 11:55:00 GMT+1000 600 false 1562550900
5 : Tue Jan 08 2019 10:55:00 GMT+1000 600 false 1546908900
6 : Tue Jan 08 2019 10:55:00 GMT+1000 600 false 1546908900

assertion failed: display string (2) is showing wrong offset compared to (5)

The expectation is that the output object 2 and object 5 would be the same. The workaround appears to be converting the input moment object to the UTC timezone (to remove the association with a DST timezone) and then adjust the utcOffset (see m6 above)

mattshin64 commented 4 years ago

Is this still a bug, if not would love to work on it.