fix: account for daylight savings time [LIBS-490] #1345
Conversation
There was a problem hiding this comment.
Thanks for writing such an extensive explanation. I don't think I would have been able to evaluate this PR without it 👍.
I agree that the current implementation is good enough and that we should not spend more time on this. We can revisit this when we add some sort of Temporal based solution, which is probably going to be part of a bigger effort of adding some sort of date/time conversion/localisation provider to the app-shell.
There was a problem hiding this comment.
LGTM .. and the compromises are very sensible. One question is regarding the tests: are we mocking the client's timezone somewhere? otherwise I guess these tests could fall if I run them locally on a different timezone..
@kabaros: For these tests, we're setting the client time zone to be UTC in the test script with an environment variable (https://github.com/dhis2/app-runtime/blob/master/services/config/package.json#L37), so they should work as long as you run them with Ideally, when writing the tests, I was hoping that I could mock different client time zones within the tests, so that we could test a greater combination of client/server time zones. It doesn't seem, however, like jest or JavaScript will let one change the time zone after one has set it (I'm going off random blogposts, stack overflow posts, because jest documentation doesn't seem to mention time zones). There do seem to be some libraries (e.g. timezone-mock) that exist for helping with this, but then when I was reading up on approaches, it seems like most of them are mocking time zones by mocking the Date object implementation using offsets...which then made me wonder if it was worth it here given the logic of the hook is extending the Date object with offsets? I don't really have a sense either way; I guess my preference would be to merge this fix and then make another ticket to look into improving these tests more? |
|
I achieved something within the same lines by spying on Intl.DateTimeFormat here .. maybe that could work as an alternative for the env variable (not 100% sure if it let you change it after setting it up though, but I think it should be possible). In all cases, I think setting the env variable is good as well .. I was just curious to know how you were ensuring that the tests are deterministic. |
Got you. Thanks @kabaros ! That's a a good idea for spying on Intl.DateTimeFormat; mostly the implementation here doesn't expressly use information about the client time zone, but now it won't calculate an offset if the server and client time zones are the same, so I've added in an additional test (with the Intl.DateTimeFormat for spying) for ensuring no offset where both server/client time is the same. Going to go ahead and merge this PR by end of day. 🙏 |
|
🎉 This PR is included in version 3.9.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
# [3.10.0-alpha.3](v3.10.0-alpha.2...v3.10.0-alpha.3) (2023-08-22) ### Bug Fixes * **connection-status:** responsiveness to online events [LIBS-497] ([#1348](#1348)) ([91a3d4d](91a3d4d)) * **types:** add generic result type to oncomplete param ([#1350](#1350)) ([a069603](a069603)) * [DHIS2] Type generic T = QueryResult to useDataQuery ([#1297](#1297)) ([7c5c083](7c5c083)) * account for daylight savings time [LIBS-490] ([06eaa5d](06eaa5d)) * account for daylight savings time [LIBS-490] [#1345](#1345) ([fb00533](fb00533)) * add test for when time zones are the same [LIBS-490] ([7911f8b](7911f8b))
|
🎉 This PR is included in version 3.10.0-alpha.3 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Implements LIBS-490
Key features
Description
useTimeZoneConversion hook assumes a constant offset between client and server time zones but this offset will change throughout the year depending on whether either client or server time zone observes daylight savings time.
Checklist
Known issues
A) I have updated the hook to calculate the offset except when either a) the client and server time zones are the same or b) an initial attempt at calculating the offset fails (e.g. could be due to some problem with the time zone information). The calculation of the offset isn't expensive, but we could also check if either client of server time zone observes daylight savings time, and if not memoize the offset calculation. I haven't done this as it feels a bit hacky (e.g. see this suggested approach on StackOverflow for determining whether time zone uses daylight savings time)
B) Since JavaScript only allows us to initialize a date in the client time zone, I can't think of a way to (easily) calculate the offset as of a given server time. E.g. if we get the string
2020-10-01T12:00:00we can calculate the offset from that client time, but cannot calculate from that server time. This will lead to some inaccuracies around the point when time shifts from standard to summer time (or vice versa). For example, if we have server time zone: UTC, client time zone: Europe/Oslo and want to figure out the client equivalent of2023-03-23T01:00:00; the server offset at this point should be 2 hours, but if we calculate at the point it is 1am for the browser, the offset would be 1 hour (because at that point, summer time won't be implemented). So we end up with a couple of hours during which point the offset is incorrectly calculated.I thought to address this issue it might make sense to calculate the offset from a client perspective and then test that the resulting client date when put into server time zone string (with
toLocaleString) is correct. If not, the offset could be guessed at within ±2 hours (accounting for daylight savings time simultaneously occurring in both northern/southern hemispheres) until one presumably finds the correct one. I don't really know this would be worth it though (and also feels hacky)Another issue that occurs to me with this is that because we allow server time zone to be a local time zone that can observe daylight savings time, we can end up in situations in which our server time differential is not unambiguously mappable to UTC (or a server time). For example, if we have server time zone: Europe/Oslo and the timestamp
2023-10-29T02:30this could be either2023-10-29T01:30UTC or 2023-10-29T00:30` UTC (that is we have apparent repeat timestamps when moving from summer to standard time)As a whole, I think I lean to being okay with this few hours of non-exactness in our system around daylight savings time (until temporal standard is introduced which should fix this). I don't think we're using dates in such a mission critical way that this will be problematic? (This isn't obviously ideal, but it seems like even if we invest time to fixing the frontend with more logic, it will still be imperfect?)