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

fix: utcOffset when local in DST but date is not #1448

Merged
merged 3 commits into from
May 3, 2022

Conversation

aloisklink
Copy link
Contributor

@aloisklink aloisklink commented Apr 9, 2021

Fixes a bug in the valueOf() function after setting a utcOffset, when the the local timezone offset changes.

I added a test for it to test/timezone.test.js, so it should be reproducible on other people's computers as well.

Without this fix, when trying to run npm test in my timezone (British Summer Time - BST), there are quite a lot of off-by-one errors, e.g.

  ● DST, a time that never existed Fall Back › 2012-11-04 02:00:00

    expect(received).toBe(expected) // Object.is equality
    
    Expected value to be:
      1352012400000
    Received:
      1352016000000

      206 |       expect(d.format()).toBe('2012-11-04T02:00:00-05:00')
      207 |       expect(d.utcOffset()).toBe(-300)
    > 208 |       expect(d.valueOf()).toBe(1352012400000)
      209 |     })
      210 |   })
      211 | })
      
      at forEach (test/plugin/timezone.test.js:208:27)
          at Array.forEach (<anonymous>)
      at Object.<anonymous> (test/plugin/timezone.test.js:204:21)

  ● keepLocalTime › keepLocalTime

    expect(received).toBe(expected) // Object.is equality
    
    Expected value to be:
      "2013-11-18T17:55:00+01:00"
    Received:
      "2013-11-18T18:55:00+02:00"

      263 |   const base = dayjs.tz('2013-11-18 11:55', 'America/Toronto')
      264 |   it('keepLocalTime', () => {
    > 265 |     expect(base.tz('Europe/Berlin').format()).toBe('2013-11-18T17:55:00+01:00')
      266 |     expect(base.tz('Europe/Berlin', true).format()).toBe('2013-11-18T11:55:00+01:00')
      267 |   })
      268 | })
      
      at Object.<anonymous> (test/plugin/timezone.test.js:265:47)

Fixes a bug in the valueOf() function after setting a utcOffset,
when the the local timezone offset changes.

I added a test for it to test/timezone.test.js, so it should be
reproducible on other people's computers as well.
@iamkun
Copy link
Owner

iamkun commented May 26, 2021

I don't know why this PR shows "This branch has not been deployed".

Would you please close it and re-open a new one?

@aloisklink aloisklink closed this May 29, 2021
@aloisklink aloisklink reopened this May 29, 2021
@codecov
Copy link

codecov bot commented May 29, 2021

Codecov Report

Merging #1448 (c400f3c) into dev (9c20e77) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##              dev     #1448      +/-   ##
===========================================
+ Coverage   99.94%   100.00%   +0.05%     
===========================================
  Files         176       179       +3     
  Lines        1770      1996     +226     
  Branches      411       507      +96     
===========================================
+ Hits         1769      1996     +227     
+ Misses          1         0       -1     
Impacted Files Coverage Δ
src/plugin/utc/index.js 100.00% <100.00%> (+1.07%) ⬆️
src/index.js 100.00% <0.00%> (ø)
src/utils.js 100.00% <0.00%> (ø)
src/locale/bg.js 100.00% <0.00%> (ø)
src/locale/br.js 100.00% <0.00%> (ø)
src/locale/ca.js 100.00% <0.00%> (ø)
src/locale/cs.js 100.00% <0.00%> (ø)
src/locale/de.js 100.00% <0.00%> (ø)
src/locale/et.js 100.00% <0.00%> (ø)
src/locale/fi.js 100.00% <0.00%> (ø)
... and 52 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c20e77...c400f3c. Read the comment docs.

@pszymanski95
Copy link

Hello 👋, any updates on this PR? Looks like it can fix a problem mentioned lately in that issue #1437, which I also encountered in my project.

@aloisklink
Copy link
Contributor Author

I don't actually know if it will fix #1437, since I have only tested this for using utc offsets (not timezones), when you are running dayjs in a region that has daylight savings.

It does look like a similar issue though, so hopefully it does fix that issue, but unless somebody writes a test for #1437, we can't be sure.

@basuneko
Copy link

basuneko commented Jul 5, 2021

Thanks for the work on this @aloisklink.
We're in NZ +12/+13 timezone and we've also encountered #1437 in our project. This PR seems to resolve it (at the very least, the off-by-one test failures in dayjs seem to go away). Would be great to have it merged

@fenharel117
Copy link

This PR resolves off-by-one errors I've been encountering

@basuneko
Copy link

There have been a few issues that are probably related to this (#1635, #1622), and more are coming as we are nearing the DST change (last sunday of September here in NZ).
@iamkun could you please give an indication of when (or if?) this fix is going to be merged and released?
Thank you for the awesome work ❤️

@steponas
Copy link

On my machine (EEST, UTC+3) there are 7 test failures on vanilla dev/master branches.
When applying this change, all failures are fixed.

@iamkun is there any blocking reason why this isn't merged yet? If there is anything changeable - I can help.

@fredzv
Copy link

fredzv commented Sep 23, 2021

I actually applied this fix to utc.js in my node_modules folder. It seems to have fixed the issue when converting timezones .tz("some/timezone", true) when daylight savings is active. It now outputs the same result as moment.js (and the result that I would expect).

@jamieparkinson
Copy link

For those who are waiting on this PR, you can apply this fix in your application by means of a plugin, like we've done here https://github.com/wellcomecollection/wellcomecollection.org/blob/32af11a4169a0a91ff3e1efd2cf2bf919d61fb90/common/utils/dates.ts#L37-L45

@mr-short
Copy link

Bump for asking why this is not being resolved? Seems to be effecting a lot of timezone/utc issues people are having

@xvaara
Copy link
Contributor

xvaara commented Oct 21, 2021

@iamkun any change to get this in?

@danielmalmros
Copy link

danielmalmros commented Nov 4, 2021

bump - @iamkun we are experience a similar issue in our production env regarding timezone/offset from UTC, so if the PR is ready, what is waiting?

Currently we experience that Europe/Copenhagen timezone shows wrong offset '$offset': 120, - it should be '$offset': 60 due to european winter time :)

Using version: 1.10.6

@Mike-Becatti
Copy link

@msal4 - Any idea when this can get pulled?

@xvaara
Copy link
Contributor

xvaara commented Nov 8, 2021

bump - @iamkun we are experience a similar issue in our production env regarding timezone/offset from UTC, so if the PR is ready, what is waiting?

Currently we experience that Europe/Copenhagen timezone shows wrong offset '$offset': 120, - it should be '$offset': 60 due to european winter time :)

the fix few comments up works fine. Use that. Seems weird that there is no reaction to this, since dayjs has the wrong time for most of europe, maybe every DST region...

@dgrelaud
Copy link

dgrelaud commented Dec 8, 2021

This PR did not work in my case so I have created an alternative DaysJS plugin for timezone.
https://www.npmjs.com/package/dayjs-timezone-iana-plugin

cf. my comment: #1271 (comment)

@gl-aagostino
Copy link

I don't know how this is supposed to work, but if I do something like

const monthValue = date.get('month');
const month = dayjs
        .utc(date)
        .set('month', monthValue)
        .tz("America/Los_Angeles")
        .startOf('month')
        .utc();

and move through the months with a date picker, in March, start of month becomes -6:00GMT instead of -7:00GMT, and once I hit December it goes back to -8:00 GMT. Why does it change by 2 hours?

iamkun pushed a commit that referenced this pull request May 6, 2022
## [1.11.2](v1.11.1...v1.11.2) (2022-05-06)

### Bug Fixes

* add OpUnitType (week) to quarterOfYear startOf/endOf types ([#1865](#1865)) ([400bc3e](400bc3e))
* Fix type issue with ManipulateType ([#1864](#1864)) ([d033dfc](d033dfc))
* fix UTC plugin .valueOf not taking DST into account  ([#1448](#1448)) ([27d1c50](27d1c50))
@iamkun
Copy link
Owner

iamkun commented May 6, 2022

🎉 This PR is included in version 1.11.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@iamkun iamkun added the released label May 6, 2022
splashwizard pushed a commit to splashwizard/tracking-time that referenced this pull request Oct 21, 2024
## [1.11.2](iamkun/dayjs@v1.11.1...v1.11.2) (2022-05-06)

### Bug Fixes

* add OpUnitType (week) to quarterOfYear startOf/endOf types ([#1865](iamkun/dayjs#1865)) ([400bc3e](iamkun/dayjs@400bc3e))
* Fix type issue with ManipulateType ([#1864](iamkun/dayjs#1864)) ([d033dfc](iamkun/dayjs@d033dfc))
* fix UTC plugin .valueOf not taking DST into account  ([#1448](iamkun/dayjs#1448)) ([27d1c50](iamkun/dayjs@27d1c50))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.