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

Timezone startOf DST off-by-one #1437

Open
ashconnell opened this issue Mar 31, 2021 · 16 comments
Open

Timezone startOf DST off-by-one #1437

ashconnell opened this issue Mar 31, 2021 · 16 comments

Comments

@ashconnell
Copy link

In Australia DST ends on the 4th of April 2021.

Using the timezone plugin and v1.10.4 (latest):

  • Getting the start of the day before this date (in DST) returns the correct value
  • Getting the start of the day after this date (outside DST) returns an incorrect value.
dayjs.tz('2021-04-01 08:00', 'Australia/Melbourne').startOf('day').format()
// 2021-04-01T00:00:00+11:00 (correct)

dayjs.tz('2021-04-15 08:00', 'Australia/Melbourne').startOf('day').format()
// 2021-04-14T23:00:00+11:00 (incorrect)

Curiously, the changelog mentions this issue was fixed in v1.9.7, so I switched to 1.9.6 to test this and it seems to be working correctly:

dayjs.tz('2021-04-01 08:00', 'Australia/Melbourne').startOf('day').format()
// 2021-04-01T00:00:00+11:00 (correct)

dayjs.tz('2021-04-15 08:00', 'Australia/Melbourne').startOf('day').format()
// 2021-04-15T00:00:00+11:00 (correct)

Is anyone up for fixing this? I'll throw a $100 bounty down. Surely there are some others who also need this fixed and want to add to the pool!

@ashconnell
Copy link
Author

I tried going back to 1.9.6 and just found a bunch of other issues with timezones.

If anyone is using the timezone plugin in production they're in for a ride!

@addisonElliott
Copy link

Responding to this comment on the PR here: #1352

I think this commit has just made things worse.

Not sure I'd use the terminology "worse". The PR did fix a bug when working with dates that have different offsets (i.e. DST ended or began since the current date and the specified date). That much is confirmed and there are a few issues that it resolved.

Now, you may be right that it introduced a different bug.

dayjs.tz('2021-04-01 08:00', 'Australia/Melbourne').startOf('day').format()
// 2021-04-01T00:00:00+11:00 (correct)

dayjs.tz('2021-04-15 08:00', 'Australia/Melbourne').startOf('day').format()
// 2021-04-14T23:00:00+11:00 (incorrect)

Looking at your first block of code above (I've quoted it for reference), I tried to reproduce it using the same version dayjs.

I'm not getting the same results. I've posted the code I'm using below.

The April 1st date is fine, I get the same result. But for April 15th, I get 2021-04-15T00:00:00+10:00. I think this also matches the result you're getting for the example you gave in the PR. As you said, DST ends April 4th in Australia and so the UTC offset goes from +11 to +10 during that time. So that's why the offset is +10 and not +11.

So, the result appears to be correct to me. Can you explain why you think that's incorrect?

Now, if you can give a snippet of code where you get 2021-04-14T23:00:00+11:00 as the result, then that definitely seems like a bug to me.

const dayjs = require('dayjs');
dayjs.extend(require('dayjs/plugin/utc'));
dayjs.extend(require('dayjs/plugin/timezone'));

dayjs.tz('2021-04-01 08:00', 'Australia/Melbourne').format();
// 2021-04-01T08:00:00+11:00

dayjs.tz('2021-04-01 08:00', 'Australia/Melbourne').startOf('day').format();
// 2021-04-01T00:00:00+11:00

dayjs.tz('2021-04-15 08:00', 'Australia/Melbourne').format();
// 2021-04-15T08:00:00+10:00

dayjs.tz('2021-04-15 08:00', 'Australia/Melbourne').startOf('day').format();
// 2021-04-15T00:00:00+10:00

@ashconnell
Copy link
Author

ashconnell commented Mar 31, 2021

@addisonElliott here's a codesandbox that reproduces what i'm seeing with 1.10.4:

https://codesandbox.io/s/sad-wiles-mx6do?file=/src/App.js

I very much wish I was getting the results you are :P

Edit: here's a screenshot in case the output is different to you somehow

image

@addisonElliott
Copy link

addisonElliott commented Mar 31, 2021

Can you save your changes? I'm not seeing the content in the div. It's just the regular "Hello CodeSandbox"

Disregard. Had to refresh my browser.

I'm not getting the same results. I'm going to try changing my timezone to yours. I think that's the issue.

@ashconnell
Copy link
Author

Very interesting. I'm in the same timezone as the example (Australia/Melbourne).

I just tried it again with a few different timezones by changing the Sensor Location in Chrome DevTools and always get this same result.

@ashconnell
Copy link
Author

Ok. Switching to US/Central (en_US) gives me the same results as you:

image

@ashconnell
Copy link
Author

ashconnell commented Mar 31, 2021

@addisonElliott I've just realised, you are correct about #1352 (comment) as +10 is expected outside of DST.

Let me run this dev version against our own apps tests. I noticed a few other things happening but now i'm second guessing myself!

@addisonElliott
Copy link

addisonElliott commented Mar 31, 2021

👍 That works.

Here's what I've found so far. If I change my local timezone to Australia/Melbourne, the issue persists. Even when I changed my timezone to UTC it persists. But when I'm on my regular timezone of America/Chicago, the problem isn't there.

I'm not sure what Chrome devtools does under the hood, but I'm not sure if that's enough to reproduce the issue. You might have to change your actual clock time on your machine.

Anyway, while on the Melbourne timezone, I did some debugging to see what the contents of the date are.

Finding 1

This is using April 1st to show what the date looks like. Offset and local offset are correct at +11. Date itself appears correct too.

> dayjs.tz('2021-04-01 08:00', 'Australia/Melbourne');
d {
  '$L': 'en',
  '$d': 2021-03-31T21:00:00.000Z,
  '$x': { '$localOffset': -660, '$timezone': 'Australia/Melbourne' },
  '$y': 2021,
  '$M': 3,
  '$D': 1,
  '$W': 4,
  '$H': 8,
  '$m': 0,
  '$s': 0,
  '$ms': 0,
  '$offset': 660
}
> dayjs.tz('2021-04-01 08:00', 'Australia/Melbourne').format();
'2021-04-01T08:00:00+11:00'

Finding 2

Now doing this at April 15th. Everything still is correct. Offset is +10.

> dayjs.tz('2021-04-15 08:00', 'Australia/Melbourne');
d {
  '$L': 'en',
  '$d': 2021-04-14T22:00:00.000Z,
  '$x': { '$localOffset': -600, '$timezone': 'Australia/Melbourne' },
  '$y': 2021,
  '$M': 3,
  '$D': 15,
  '$W': 4,
  '$H': 8,
  '$m': 0,
  '$s': 0,
  '$ms': 0,
  '$offset': 600
> dayjs.tz('2021-04-15 08:00', 'Australia/Melbourne').format();
'2021-04-15T08:00:00+10:00'

Finding 3

Here I did startOf for April 15th and the problem was introduced. There was a change a few releases back regarding startOf,endOf, and I wonder if that's when this bug was introduced.

Here's what I notice that's wrong:

  • No $localOffset in $x
  • Offset is wrong, should be +10 not +11.
> dayjs.tz('2021-04-15 08:00', 'Australia/Melbourne').startOf('day');
d {
  '$L': 'en',
  '$offset': 660,
  '$d': 2021-04-14T13:00:00.000Z,
  '$x': { '$timezone': 'Australia/Melbourne' },
  '$y': 2021,
  '$M': 3,
  '$D': 14,
  '$W': 3,
  '$H': 23,
  '$m': 0,
  '$s': 0,
  '$ms': 0
}
> dayjs.tz('2021-04-15 08:00', 'Australia/Melbourne').startOf('day').format();
'2021-04-14T23:00:00+11:00'

Summary

That's as far as I got debugging. Something is up with the startOf function causing this problem.

I don't have any time left today to debug this, but hopefully that gets you on the right track.

@ashconnell
Copy link
Author

ashconnell commented Mar 31, 2021

I'm now almost certain the issues i've been seeing today disappear on dev thanks to this PR #1352

Our test suite runs a bunch of bookings across different timezones and they're all green.

Looking forward to the next release, and thanks for your help. Where do I send money to support this lib?

@addisonElliott
Copy link

That makes sense. I didn't make the connection that the PR hasn't been released yet.

🤷‍♂️ Not sure where to send it. I just follow a few issues that I'm familiar with and try to help if there's any issues.

@ashconnell
Copy link
Author

@iamkun do you have any ETA plans for a new release? The dev branch contains a commit that fixes a pretty bad timezone issue with DST. AFAICT there's no historical release that has stable timezones except the upcoming one.

@nemphys
Copy link

nemphys commented Apr 7, 2021

@iamkun do you have any ETA plans for a new release? The dev branch contains a commit that fixes a pretty bad timezone issue with DST. AFAICT there's no historical release that has stable timezones except the upcoming one.

+1 on this, would have to have to revert to moment.js because of this!

@apm78
Copy link

apm78 commented May 19, 2021

dayjs.utc('2021-02-01T12:00:00.000Z').tz('Europe/Berlin').format() evaluates to 2021-02-01T13:00:00+02:00 but should be 2021-02-01T13:00:00+01:00. The time is correct, but the offset is not.
Looking at the PR 1352 that was mentioned. I think this is due to the fact that the offset of the current local time is used (executing the code while having DST) not the one of the given date (no DST).

Unfortunately, as long as this error is in place I have to postpone the migration from moment.js

@apm78
Copy link

apm78 commented May 27, 2021

I updated to the version 1.10.5. It seems though that the problems with the timezones and DST are only partly solved.
Please see this code sandbox for yourself.
E.g.
dayjs.utc("2021-01-11T20:00:00.000Z").tz("Europe/Berlin").toJSON() returns 2021-01-11T21:00:00.000Z but should be 2021-01-11T20:00:00.000Z.

@ashconnell
Copy link
Author

@apm78 day-js timezone support is insanely broken.

The docs make it seem like timezone support is production ready but it's literally a single ~80 line file that barely covers anything.

I'm not very knowledgeable with timezones so it's hard for me to contribute, but strategically I think this repo should:

  1. Make it blatantly obvious that timezone support isnt ready to be used
  2. Build a test suite that cross checks results with something that is production ready, eg Moment.js

I spent literally weeks adding day-js to a new platform before realising these issues existed and then spent literally days swapping it back out for Moment.js

@addisonElliott
Copy link

I don't use the timezone functionality extensively. Primarily I utilize UTC timestamps for timestamping things, so I haven't noticed any issues.

I'm not sure I'd agree with it being "insanely broken", but broken nonetheless. My understanding there are a few outstanding bugs regarding timezones that are causing some headaches. These are likely small & simple fixes waiting to be investigated.

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