Repository navigation
fix(sheets): read date-only strings as local calendar dates - #136
mariyadavydova merged 5 commits into
Conversation
parseDate used new Date(text), which parses a date-only ISO string such
as "2024-03-15" as UTC midnight, while the date functions read the
result with local getters and format it with formatDate. West of UTC
every such date moved back a day: DAY("2024-03-15") returned 14,
WEEKDAY, EDATE, DATEVALUE, NETWORKDAYS, WORKDAY and DATE() round trips
were off by one, and eight existing formula tests fail under
TZ=America/New_York.
Date-only strings are now built with the local Date constructor. The
holiday sets of NETWORKDAYS.INTL and WORKDAY.INTL are keyed by the local
calendar date, YEARFRAC reads local fields, WEEKNUM counts whole days so
a DST change cannot shift the week, and the financial yearFrac rounds
its day count. A new test runs these in zones on both sides of UTC.
|
Thanks for the contribution! The original off-by-one problem is reproducible, and this fixes the common cases. The added time-zone tests are helpful too. I tested the branch with main merged locally and found a few things to address before merging:
The existing formula suite passes across the eight time zones I tested; these cases seem to fall outside its current coverage. Thanks again for tracking down the original issue and including a clear reproduction! |
- NETWORKDAYS and NETWORKDAYS.INTL compare calendar days, so a day with no local midnight (America/Santiago, 2019-09-08) no longer drops the last day of the range. - Financial day counts round only between two date-only values; a timestamp keeps its time of day as before. - YEARFRAC bases 0 and 4 read a date-only value by its calendar day and a timestamp by its UTC day as before, so an explicit offset gives the same result in every zone. - parseDateString is module-private again (lint:unused). - A date-only value sets year, month and day together, so 0000-02-29 keeps its leap day. The time-zone tests now also run in UTC, America/Santiago and Asia/Kolkata and cover each case.
|
Thanks for the careful review, @mariyadavydova. All five are fixed in dc95cca, on top of a merge of current main. Each case is a new test in
What I ran:
|
|
Thank you, @hey-research-lab ! |
|
Thanks for the quick review and merge, @mariyadavydova! We're a small team building HEY (heyresearch.xyz), a public record of who's actually shipping on Robinhood Chain, and we found this while reading Nautilo's code. If the team is open to a quick mention from @nautiloapp on X, it would mean a lot to us. Either way, happy to help with more fixes. |
Contribution track
What was wrong
parseDateandparseDateTimeinpackages/office-sheets/src/formula/functions-date.ts(around lines 893-944 onmain) callnew Date(str.v). JavaScript parses a date-only ISO string such as"2024-03-15"as UTC midnight. The date functions then read that value with local getters (getDate,getDay,getMonth) and write results withformatDate, which also uses local getters.DATE()returnsYYYY-MM-DDstrings in the same form, so every date that flows through a formula gets parsed this way.In any time zone west of UTC, each such date ends up one day early:
=DAY("2024-03-15")returns14=WEEKDAY("2024-03-17")returns7, not1=EDATE("2024-03-15",-1)returns2024-02-14=DATEVALUE("2024-03-15")returns2024-03-14=DAY(DATE(2024,3,15))returns14The existing suite catches this, but only when the zone is set. With
TZ=America/New_YorkorTZ=America/Los_Angeles, 8 tests intest/formula/formula.test.tsfail (DAY, WEEKDAY, EDATE, NETWORKDAYS, DATEVALUE, WEEKNUM, ISOWEEKNUM, WORKDAY). They pass under UTC and east of it, which is likely why the bug went unnoticed.Three related spots had the same mixed UTC/local handling:
NETWORKDAYS.INTLandWORKDAY.INTLholiday sets were keyed withtoISOString(), the UTC date.YEARFRACbases 0 and 4 readgetUTC*fields.WEEKNUMdivided a millisecond difference by a 7-day week. Across a daylight-saving change that span is an hour short, so=WEEKNUM("2024-03-17")returned11in New York instead of12.How it was verified
Formula tests (
vitest --run test/formula), before and after:The new
test/formula/date-timezone.test.tssets the zone itself withvi.stubEnv('TZ', ...), so it fails on any machine without the fix:8 failed | 4 passedbefore (New York and Los Angeles),12 passedafter.The whole
@nautilo/office-sheetssuite was also run under UTC, New York and Auckland, before and after the change. No test fails after the change that passed before. The remaining failures in this environment are files that need workspace dependencies I didn't install (importers, view tests, package-entry tests), and they're identical in both runs.tsc --noEmit -p tsconfig.jsonshows no errors in the changed files. The error count is the same before and after, and all of those errors come from the uninstalled dependencies.What changed
parseDateString(text). It builds aYYYY-MM-DDstring with the localDateconstructor and passes any other string tonew Date(text)as before. A month outside 1-12 or a day outside 1-31 is still invalid, and day overflow (2024-02-30) still rolls over, as it did before.parseDateandparseDateTimeuse it.NETWORKDAYS.INTLandWORKDAY.INTLholiday sets parse withparseDateStringand key byformatDate(the local calendar date), and the loops compare the same way.YEARFRACbases 0 and 4 read local fields, matching the other date functions.WEEKNUMcounts whole calendar days from 1 January usingDate.UTCon the local fields, so a DST change can't shift the week.yearFrachelper rounds its day count, the same way the other financial day counts already do.docs/office-engines/CHANGES.mdhas a short entry for the change.Trust and compatibility
No change to identity, permissions, data movement, providers or APIs. Formula results change only where they were off by a day in zones west of UTC, or across a DST boundary in
WEEKNUM.Author checks