Converting dates (from JS) gets incorrect results if crossing DST boundary #1332

Closed
opened 2018-10-30 09:33:11 +00:00 by lalomartins · 7 comments
lalomartins commented 2018-10-30 09:33:11 +00:00 (Migrated from github.com)

(Actually also with different timezones, but you state in the README you make a choice of not handling timezones at all, so let's focus on the DST issue.)

This is a little difficult to explain though. Let's say we ended Daylight Saving Time last Sunday. Let d1 be a date today, and d2 be a date exactly a week ago.

To test more easily, let's say we set dateNF: 'yyyy-mm-dd"T"hh:MM:ss'. Then put d1 and d2 in two columns in the same row and export a csv. (The bug is not actually related to csv, it's just easier to test if we convert to string.)

Expected: the time part (after T) is identical for both.

Actual: the time part for d2 will be off by one hour (e.g. 10:28 instead of 11:28).

The reason for this is the dnthresh constant is calculated based on the timezone offset of the current time as of when the library is loaded. So dates with any other offset (including same timezone but different DST status) will break the calculation.

PR coming up in a few minutes.

(Actually also with different timezones, but you state in the README you make a choice of not handling timezones at all, so let's focus on the DST issue.) This is a little difficult to explain though. Let's say we ended Daylight Saving Time last Sunday. Let d1 be a date today, and d2 be a date exactly a week ago. To test more easily, let's say we set `dateNF: 'yyyy-mm-dd"T"hh:MM:ss'`. Then put d1 and d2 in two columns in the same row and export a csv. (The bug is not actually related to csv, it's just easier to test if we convert to string.) Expected: the time part (after T) is identical for both. Actual: the time part for d2 will be off by one hour (e.g. 10:28 instead of 11:28). The reason for this is [the dnthresh constant](https://github.com/SheetJS/js-xlsx/blob/9866dfc010338394e4cfcd33a9fbc15dae017ee5/bits/20_jsutils.js#L35) is calculated based on the timezone offset of the current time as of when the library is loaded. So dates with any other offset (including same timezone but different DST status) will break the calculation. PR coming up in a few minutes.
SheetJSDev commented 2018-10-30 19:36:37 +00:00 (Migrated from github.com)

Thanks for looking into this @lalomartins !

The overarching problem is that Excel doesn't actually have a proper concept of time. It's shoehorned into the numeric model in a very simplistic way, where each calendar day has 86400 equally-spaced seconds and midnight is pinned to 0. This very clearly creates problems around DST.

Thanks for looking into this @lalomartins ! The overarching problem is that Excel doesn't actually have a proper concept of time. It's shoehorned into the numeric model in a very simplistic way, where each calendar day has 86400 equally-spaced seconds and midnight is pinned to 0. This very clearly creates problems around DST.
lalomartins commented 2018-11-05 14:15:30 +00:00 (Migrated from github.com)

In our case it was an actual bug in our app (GrabCAD Print). Users noticed times on the CSV and XSLX exports were inconsistent with what they would see in the GUI and we tracked it down to DST shift. And the fix is relatively simple, so I got an OK from the company to contribute a PR.

In our case it was an actual bug in our app (GrabCAD Print). Users noticed times on the CSV and XSLX exports were inconsistent with what they would see in the GUI and we tracked it down to DST shift. And the fix is relatively simple, so I got an OK from the company to contribute a PR.
cemremengu commented 2018-11-15 21:38:15 +00:00 (Migrated from github.com)

Spent hours trying to figure out why our tests were failing 😄

Any ETA for the patch release?

Spent hours trying to figure out why our tests were failing :smile: Any ETA for the patch release?
phani17c commented 2018-12-29 00:26:57 +00:00 (Migrated from github.com)

Do we have this fix in latest release?

Do we have this fix in latest release?
guyko81 commented 2019-12-06 12:03:30 +00:00 (Migrated from github.com)

I have issue with DST but in a more serious way: when importing pure datepart (no hours) from CSV the date jumps 1 full day when we move over DST boundary.
It's a rounding issue, but don't know where to fix in the code.

Here is the actual date and the problem (date format | number format):

Expected:
2013.10.28 | 41575
2013.10.27 | 41574
2013.10.26 | 41573

Actual
2013.10.28 | 41575
2013.10.26 | 41573.95833
2013.10.25 | 41572.95833

Excel does the same formatting (the actual formatted value is 2013.10.26 23:31:12), so the logic is understandable, however the original data in the CSV didn't contain HH:MM:SS, so the DST logic shouldn't effect the imported value.

I have issue with DST but in a more serious way: when importing pure datepart (no hours) from CSV the date jumps 1 full day when we move over DST boundary. It's a rounding issue, but don't know where to fix in the code. Here is the actual date and the problem (date format | number format): Expected: 2013.10.28 | 41575 2013.10.27 | 41574 2013.10.26 | 41573 Actual 2013.10.28 | 41575 2013.10.26 | 41573.95833 2013.10.25 | 41572.95833 Excel does the same formatting (the actual formatted value is 2013.10.26 23:31:12), so the logic is understandable, however the original data in the CSV didn't contain HH:MM:SS, so the DST logic shouldn't effect the imported value.
DavidKHahn commented 2020-03-26 01:17:26 +00:00 (Migrated from github.com)

@guyko81 Hello this might sound like a simple solution but do you know how I can convert 41575 to date format?

@guyko81 Hello this might sound like a simple solution but do you know how I can convert 41575 to date format?
SheetJSDev commented 2020-03-26 01:25:12 +00:00 (Migrated from github.com)

41575 is midnight of 2013-10-28 in the timezone you are currently working in (not UTC).

This can be determined by

new Date(1904,0,41575-1461)
41575 is midnight of 2013-10-28 in the timezone you are currently working in (not UTC). This can be determined by ```js new Date(1904,0,41575-1461) ```
Sign in to join this conversation.
No Milestone
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: sheetjs/sheetjs#1332
No description provided.