fix(timezone): stop inheriting host DST in instance .tz() offset - #3169
fix(timezone): stop inheriting host DST in instance .tz() offset#3169Moumouls wants to merge 2 commits into
Conversation
Use tzOffset (Intl formatToParts) in proto.tz instead of toLocaleString + new Date + getTimezoneOffset, so fixed-offset zones like America/Guadeloupe keep UTC-4 across host spring DST.
|
I can confirm this patch on our company monorepo fix the issue |
|
@iamkun would you be available to review this? (or would you pass it off to another maintainer for review?) When I speak with other engineers and our students, I always suggest avoiding Day.js, because of the surprising and severe time zone bugs. I would love to be able to start recommending the library again. |
|
@karlhorky i sent an email to @iamkun with some luck it could land asap and released |
|
Thanks a lot for tracking this down and fixing it — this is a real, long-standing pain point (see #1260 and the related issues), and the fix is clean: reusing the existing host-independent Before merging, could you add a few more regression cases to
Once these are in, I'm happy to merge — this is a great fix and I'd like to get it into a release soon. |
|
Thanks @iamkun i'll add this asap |
|
@iamkun I’m currently using GPT-5.6 Sol xhigh to battle-test the fix, add more coverage, and fix the TZ plugin as much as possible to minimize future maintenance and bug fixes. The PR will be slightly bigger |
Store timezone wall clocks in UTC and resolve DST gaps/overlaps like Moment so host timezone transitions no longer skip hours or pick the wrong offset.
|
@iamkun It’s ready. I made multiple passes to simplify the code as much as possible and maximize both code coverage and functional coverage with Sol xhigh. I believe the TZ extension is now almost bulletproof, and it should fix many of the TZ issues in Day.js. |
|
Sorry for the added lines ( mostly to coverage edge case, like a DST shift a sunday in 2050...) |
|
Thanks for the context. Just to make sure I understand correctly — you're planning to add more tests and battle-test the fix on top of the existing PR, right? The additional test coverage sounds great — thank you for that. One small ask though: if this PR also ends up touching refactoring or other fixes for the TZ plugin, would it be okay to move those to a separate PR? That would really help with keeping the review manageable. |
|
@iamkun final version pushed, tell me if it need a split, the only caveat, the approach is based on "moment feature parity" for coverage so if moment have a TZ bug, dayjs is likely to reproduce it. Tell me if you need a split here, but the internal refactor to fix properly that big, i think the review is manageable, tell me if you need something or if the review is too hard to manage on this final commit ? |
|
The refactor was indeed necessary, @iamkun. Without it, fixing all the tests properly made the code quite tricky at some point. Once all the tests were passing, I did four rounds of simplification with Sol 5.6 xhigh wherever possible. I also reviewed the lines directly in Cursor and read through the code myself. |
|
reported coverage on TZ plugin is 100% |
|
Thanks again for digging into this — and for the much deeper follow-up. A few thoughts on scope: 1. The original fix is good as-is. The minimal change to 2. The broader That said, we'd rather not solve it by repurposing The existing So my suggestion: let's merge the original minimal fix now (with the added tests) to close out the reported issue, and open a separate PR for the |
|
@iamkun To keep a full working reference, I suggest we close this PR. I’ll open a new one with a minimal fix, your suggestions, and as many passing tests as possible — without refactor or workarounds. Remaining failing tests will be skipped (but kept into the new PR) Once that PR is merged, a follow-up PR can do the deeper refactor and make the remaining tests pass, so we still reach the same end state as this PR (#3169). Reverting the commit here would lose the history and some fixes that could be useful later. Does that sound good and clear to you? |
|
Sounds good, thanks for the clarification. One adjustment: for the new minimal-fix PR, let's only include tests that pass. Any failing/edge-case tests (the DST scenarios not yet handled by the minimal fix) should be left out of this PR entirely, not committed-but-skipped — we'd rather every merged PR be fully green. We can track those as follow-up test cases for the deeper-refactor PR. For that follow-up, please also consider the approach we outlined above: implement the DST-correctness work for |
|
AI MESSAGE @iamkun following your review, I opened a minimal PR with only the original It includes the Guadeloupe regression, the DST-observing target-zone tests you asked for (NY/Paris desync, This PR (#3169) stays open as the working base for the follow-up ( |
…3174) * fix(timezone): compute .tz() offset without host DST Use tzOffset (Intl formatToParts) in proto.tz instead of toLocaleString + new Date + getTimezoneOffset, so fixed-offset zones like America/Guadeloupe keep UTC-4 across host spring DST. * test(timezone): cover DST-observing zones and host TZ matrix Add iamkun's requested conversion tests (NY/Paris desync, keepLocalTime, New_York/Auckland hosts) plus passing parse/convert cases from #3169.
* chore: update doc * chore: update doc * chore: update doc * fix(plugin): timezone compute instance .tz() offset without host DST (#3174) * fix(timezone): compute .tz() offset without host DST Use tzOffset (Intl formatToParts) in proto.tz instead of toLocaleString + new Date + getTimezoneOffset, so fixed-offset zones like America/Guadeloupe keep UTC-4 across host spring DST. * test(timezone): cover DST-observing zones and host TZ matrix Add iamkun's requested conversion tests (NY/Paris desync, keepLocalTime, New_York/Auckland hosts) plus passing parse/convert cases from #3169. --------- Co-authored-by: Antoine Cormouls <contact.antoine.cormouls@gmail.com>
Summary
Fixes incorrect timezone offsets when converting an instant with instance
.tz()on a host that observes EU-style daylight saving time.Bug:
America/Guadeloupeis a fixed UTC−4 zone (no DST). On hosts such asEurope/ParisorEurope/London, around the EU spring-forward Sunday,dayjs(instant).tz('America/Guadeloupe')could report −03:00 instead of −04:00. Adding one day and converting again could then land on the same calendar day.Root cause:
proto.tzderived the offset from:Both
new Date(target)andgetTimezoneOffset()depend on the host timezone. When the host has already switched to DST for that instant but the wall-clock string still parses on the standard-time side of the transition, the offset for a fixed zone is wrong by one hour.The static helper
dayjs.tz(string, zone)already usedtzOffsetviaIntl.DateTimeFormat#formatToParts(host-independent). Instance.tz()did not.Fix: reuse the existing
tzOffset(+date, timezone)helper inproto.tzfor the offset. Keep wall-clock construction viatoLocaleStringand the rest of the method unchanged (utcOffset,keepLocalTime,$timezone).Related: #1260
Test plan
America/Guadeloupearound EU spring DST (2050-03-27)TZ=Europe/Paris npm run test-tz-plugin— pass (including Guadeloupe cases)TZ=Europe/London npm run test-tz-plugin— passnpm run test-tzhost matrix still coverstest/timezone.testReproduce before the fix:
TZ=Europe/Paris npx jest test/plugin/timezone.test.js -t 'America/Guadeloupe' --coverage=falseCredits
This PR was prepared with Cursor and assistance from Grok 4.5 High.