marnusw / date-fns-tz

Complementary library for date-fns v2 adding IANA time zone support
MIT License
1.08k stars 116 forks source link

getTimezoneOffset appears to incorrect calculate daylight savings time #227

Open douglas-linder-hireup opened 1 year ago

douglas-linder-hireup commented 1 year ago

There are so many tickets on this issue, I've tried to consolidate my findings into the 'tldr' section at the bottom.

However, in detail... the documentation on this function says:

For time zones where daylight savings time is applicable a Date should be passed on the second parameter to ensure the offset correctly accounts for DST at that time of year.

However, and this is critically important, what it doesn't say is what the date should be.

What should the date be?

It turns out some time zones do strange things like put the transition between DST and non-DST on odd hours in the morning, like 2am in the hopes that won't impact too many people.

So, given that a specific day has the clocks turned back to 2am when you hit 3am, and you want to know what your offset is at 2am, what date should you pass to this function?

Local time? You can't. It's ambiguous. There is no UTC moment that matches '2am' local time. There are two moments, with two different offsets for that local time.

We therefore conclude that is it incorrect to pass a local time date into getTimezoneOffset

The implementation assumes the opposite. This seems a) wrong, and b) trivial to fix.

Let's move on...

Tests

Specifically, these tests: https://github.com/marnusw/date-fns-tz/blob/master/src/getTimezoneOffset/test.js#L49

The code:

var date = new Date('2020-10-04T00:45:00.000Z') 

Returns the UTC timestamp 1601775900000, which is Sunday, October 4, 2020 1:45:00 AM GMT <---

However, this test is supposed to be asserting that when the local time ticks over, the daylight savings is applied.

As previously discussed, using the local time for this cannot be correct. These tests should be using UTC time.

If we substitute the utterly unambiguous exact UTC moment:

    it('15 minutes before', function () {
      var date = new Date(1601739900000) 
      assert.equal(getTimezoneOffset('Australia/Melbourne', date), 10 * hours)
    })

    it('15 minutes after', function () {
      var date = new Date(1601741700000)
      assert.equal(getTimezoneOffset('Australia/Melbourne', date), 11 * hours)
    })

Then the test suite fails.

By adding additional tests, you can see that the conversion is in fact running at the GMT time equivalent.

In fact, this test passes:

   it('does DST at GMT time incorrectly', function () {
      var date = new Date(1601776800000) // Sunday, October 4, 2020 2:00:00 AM GMT. Wrong!
      assert.equal(getTimezoneOffset('Australia/Melbourne', date), 11 * hours)
    })

What?

So what's happening?

1601776800000 -> Sunday, October 4, 2020 2:00:00 AM GMT <--- DST is applied at this moment

1601740800000 -> Sunday, October 4, 2020 2:00:00 AM Australia/Melbourne <-- is should be applied here

The DST is being applied to local times.

Why?

It's pretty straight forward:

https://github.com/marnusw/date-fns-tz/blob/master/src/getTimezoneOffset/index.js#L31

return -tzParseTimezone(timeZone, date)

but:

https://github.com/marnusw/date-fns-tz/blob/master/src/_lib/tzParseTimezone/index.js#L15

export default function tzParseTimezone(timezoneString, date, isUtcDate) { <---- !!!!

tzParseTimezone is being invoked with isUtcDate undefined, and therefore false.

Therefore, it converts the incoming date object to UTC.

To be fair, in when this was added (https://github.com/marnusw/date-fns-tz/commit/dd83ed0618be6737dab67145c3b4bb610c06c7db), that parameter didn't exist; it was added to fix a different DST issue in https://github.com/marnusw/date-fns-tz/commit/e97b76c41a772342149394f0cbe8b9a0042c553f

TLDR

There's a bug.

It's here: https://github.com/marnusw/date-fns-tz/blob/master/src/getTimezoneOffset/index.js#L31

It should read:

return -tzParseTimezone(timeZone, date, true)

The question is: should it be fixed? Or is that cool, or, because it fundamentally changes the API behaviour, a breaking change?

Either way, I just want to call out, again, that this isn't a documentation issue.

Passing a local time into this function is fundamentally wrong and cannot produce unambiguous results.

TranVanTinhUIT commented 1 year ago

I had a similar problem and I solved it as follows:

function getTzOffset(timeZone: string, strISODate: string) {
  const zoneDate = utcToZonedTime(new Date(strISODate), timeZone);
  const zoneDateStr = format(zoneDate, 'yyyy-MM-dd HH:mm:ss');
  const utc = new Date(strISODate);
  const msOffset = utc.getTime() - new Date(zoneDateStr + 'Z').getTime();
  return msOffset / (1000 * 60);
}
tomfa commented 1 year ago

Here's a test we have that highlights this bug (if I read this issue right):

// 3:30 AM in CEST (UTC+2)
const immediatelyAfterDstChange = new Date('2022-03-27T01:30:00.000Z'); 

// Fails, returns 3600 * 1000
expect(getTimezoneOffset('Europe/Oslo', immediatelyAfterDstChange)).toBe(7200 * 1000)
bferreira commented 11 months ago

We are also experience this issues when using getTimezoneOffset together with Highcharts: image This should be twice 2:00

dereekb commented 8 months ago

I've run into this right now where getTimezoneOffset() returns a -5 hour offset for 12AM for America/Chicago instead of -6 up until 2/3AM when it actually becomes Central Daylight Time.

Thanks @TranVanTinhUIT for your workaround since that does return the proper offset for time before 2AM.

I did have to change it a little bit to match the same tzOffset return value (GMT-5 should be -5 hours in milliseconds):

const tzOffset = getTimezoneOffset(timezone, date);

is now:

  const inputTimeDate = setDate(date, { seconds: 0, milliseconds: 0 });
  const inputTime = inputTimeDate.getTime();
  const zoneDate = utcToZonedTime(inputTimeDate, timezone);
  const zoneDateStr = formatDate(zoneDate, 'yyyy-MM-dd HH:mm');   // ignore seconds, etc.
  const zoneDateTime = new Date(zoneDateStr + 'Z').getTime();
  const inputTime = setDate(date, { seconds: 0, milliseconds: 0 }).getTime(); // use date-fns set() function to clear ms/seconds since offset should only be hours/minutes
  const tzOffset = zoneDateTime - inputTime;