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

Inconsistent behaviour for utcOffset. #1340

Closed
y-abarnikov opened this issue Jan 18, 2021 · 8 comments
Closed

Inconsistent behaviour for utcOffset. #1340

y-abarnikov opened this issue Jan 18, 2021 · 8 comments

Comments

@y-abarnikov
Copy link

y-abarnikov commented Jan 18, 2021

Describe the bug
Inconsistent behaviour for utcOffset. Probably issue with Daylight Saving Time.
image

Expected behavior
Should be -4 as in the row below.

Information

  • Day.js Version [1.10.3]
  • moment-timezone Version [0.5.32]
  • OS: [Mac OS Big Sur]
  • Interpreter Node.js [14.15.0]
  • Time zone: [GMT+2 (Eastern European Time)]
@y-abarnikov
Copy link
Author

@iamkun

@addisonElliott
Copy link

FYI, it seems the bug is in moment to me.

Right now the correct offset for America/New_York is -5.

See here: https://www.zeitverschiebung.net/en/timezone/america--new_york

@hansdaniels
Copy link
Contributor

hansdaniels commented Jan 21, 2021

DST is picked up by tz, but strangely, it is added to the hour only, not to the offset:

import dayjs from 'dayjs';
import tz from 'dayjs/plugin/timezone.js';
import utc from 'dayjs/plugin/utc.js';
dayjs.extend(tz);
dayjs.extend(utc);

console.log(dayjs.tz(new Date('2020-03-29T00:30:00Z').getTime(), 'Europe/Berlin').format(), 'is correct');
console.log(dayjs.tz(new Date('2020-03-29T01:30:00Z').getTime(), 'Europe/Berlin').format(), 'should be 2020-03-29T03:30:00+02:00, because DST kicked in half an hour before');

yields:

2020-03-29T01:30:00+01:00 is correct
2020-03-29T03:30:00+01:00 should be 2020-03-29T03:30:00+02:00, because DST kicked in half an hour before

@addisonElliott
Copy link

Not sure your bug is similar to the OPs, unless I'm missing the connection.

But I did confirm and figure out what the problem is with your case. It's actually a pretty easy fix (I think).

The problem

I'm in CST, which has an offset of -5/-6 (depending on DST). Let's use this as an example.

Given the input you gave: 2020-03-29T00:30:00Z

The code converts to local machines timezone (at this time it was -5 for me). Then it converts to the desired timezone. These are the results:
* 2020-03-28T20:30:00-05:00
* 2020-03-29T03:30:00+02:00

Then these two dates are subtracted (disregard the offset), giving -07:00. Next, we take local machine offset (-5) and subtract from -7. -5 - (-7) = +2. Correct answer.

That's how it should work. Let's look at the code and I'll show you the bug.

  proto.tz = function (timezone = defaultTimezone, keepLocalTime) {
    const oldOffset = this.utcOffset()
    const target = this.toDate().toLocaleString('en-US', { timeZone: timezone })
    const diff = Math.round((this.toDate() - new Date(target)) / 1000 / 60)
    let ins = d(target).$set(MS, this.$ms).utcOffset(localUtcOffset - diff, true)
    if (keepLocalTime) {
      const newOffset = ins.utcOffset()
      ins = ins.add(oldOffset - newOffset, MIN)
    }
    ins.$x.$timezone = timezone
    return ins
  }

Source: https://github.com/iamkun/dayjs/blob/dev/src/plugin/timezone/index.js#L101

Check out that it takes localUtcOffset and subtracts the diff. That localUtcOffset is obtained by doing the following. The problem is that DST is applied now so the offset for me is -06:00, giving the offset of +01:00 that we're seeing.

  const localUtcOffset = d().utcOffset()

Source: https://github.com/iamkun/dayjs/blob/dev/src/plugin/timezone/index.js#L39

The fix

Replace localUtcOffset with this.utcOffset() and that will return the UTC offset at that date

What you should do

I haven't tested this but I think it'll work.

If that works for you, I'd encourage you to submit a PR and fix this.

When was the bug introduced

Bug has existed ever since the timezone plugin was added.

https://github.com/iamkun/dayjs/pull/974/files

@hansdaniels
Copy link
Contributor

hansdaniels commented Jan 21, 2021 via email

@hansdaniels
Copy link
Contributor

The proposed works for utcOffset when starting from Z.
But when the input date already has a different timezone, tz is now broken.

 FAIL  test/plugin/timezone.test.js (5.826s)
  ● Parse › parse and convert between timezones
    Expected value to be:
      "2014-06-01T09:00:00-07:00"
    Received:
      "2014-06-01T09:00:00-11:00"

      57 |   it('parse and convert between timezones', () => {
      58 |     const newYork = dayjs.tz('2014-06-01 12:00', NY)
    > 59 |     expect(newYork.tz('America/Los_Angeles').format()).toBe('2014-06-01T09:00:00-07:00')
      60 |     expect(newYork.tz('Europe/London').format()).toBe('2014-06-01T17:00:00+01:00')

Have you meant changing

    let ins = d(target).$set(MS, this.$ms).utcOffset(localUtcOffset - diff, true)

to

let ins = d(target).$set(MS, this.$ms).utcOffset(this.utcOffset() - diff, true)

or have you had another line in mind?
This was the only reference to localUtcOffset.

Do you directly see the cause of this or would you have to dig deeper as well?

@addisonElliott
Copy link

No you changed the code correctly. Good catch.

I think I know the problem. Try the below snippet. Basically, the issue is that .utcOffset is overridden when a timezone is set, but we actually want the user's local timezone at that date. The reason is because new Date in Javascript without a timezone will assume local timezone and we need that offset.

Another thing is I'm not sure that keepLocalTime logic is valid anymore. Curious to see if tests fail for that too.

  proto.tz = function (timezone = defaultTimezone, keepLocalTime) {
    const oldOffset = this.utcOffset()
    const date = this.toDate();
    const target = date.toLocaleString('en-US', { timeZone: timezone })
    const diff = Math.round((date - new Date(target)) / 1000 / 60)
    let ins = d(target).$set(MS, this.$ms).utcOffset((-Math.round(date.getTimezoneOffset() / 15) * 15) - diff, true)
    if (keepLocalTime) {
      const newOffset = ins.utcOffset()
      ins = ins.add(oldOffset - newOffset, MIN)
    }
    ins.$x.$timezone = timezone
    return ins
  }

hansdaniels pushed a commit to hansdaniels/dayjs that referenced this issue Jan 22, 2021
hansdaniels pushed a commit to hansdaniels/dayjs that referenced this issue Jan 23, 2021
@iamkun
Copy link
Owner

iamkun commented Jan 28, 2021

fixed in #1352

@iamkun iamkun closed this as completed Jan 28, 2021
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