fix: keep startOf/endOf below a day inside the current UTC offset - #3176
Open
maximilliangrand wants to merge 1 commit into
Open
fix: keep startOf/endOf below a day inside the current UTC offset#3176maximilliangrand wants to merge 1 commit into
maximilliangrand wants to merge 1 commit into
Conversation
The Date setters startOf/endOf use for hour, minute and second convert a
local wall clock back to an instant. At a DST fall back that wall clock
occurs twice and the conversion always picks the first occurrence, so the
result lands a whole offset step before the instant it was derived from:
TZ=Europe/Berlin
const d = dayjs('2024-10-27T01:30:45.123Z') // 02:30:45.123 +01:00
d.startOf('second').toISOString() // 2024-10-27T00:30:45.000Z
d.valueOf() - d.startOf('second').valueOf() // 3600123, not 123
d.isSame(d, 'second') // false
d.isBefore(d, 'second') // true
Restore the offset the instance started in when the setter crossed the
transition and that offset is the one in force at the corrected instant,
which leaves a wall clock that never existed resolved as before. Units of
a day and above keep building their result from local calendar fields.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The contract that breaks
startOf(unit)clears the fields belowunitandendOf(unit)fills them. Three things follow, and Day.js depends on all three:t - t.startOf('second') === t.millisecond()— clearing milliseconds cannot move the clock.startOf(u) <= t <= endOf(u).isSame,isBeforeandisAfterare rule 2 —isSame(that, u)is literallythis.startOf(u) <= other && other <= this.endOf(u)(src/index.js).On any host in a DST timezone, for one hour a year, all three are false.
Reproduction, on 1.11.21, no plugins
endOf('second')lands before the instant it came from, so anything bucketing, deduplicating or cache-keying by "same minute / same second" is silently wrong for that hour. No error, no invalid date.Root cause —
src/index.js,startOf->instanceFactorySethour, minute and second are
setMinutes(0,0,0)/setSeconds(0,0)/setMilliseconds(0)on a localDate. Per ECMA-262 every local setter readsLocalTime(t), replaces fields, then maps back withUTC(t) = t - LocalTZA(t, false). That inverse is not injective across a fall back, and the spec resolves the repeated wall clock to the offset before the transition. So for an instant whose wall clock sits in the repeated window the setter hands back an instant one whole offset step earlier — includingsetMilliseconds(0), which is not supposed to move anything.Moment, which this library tracks and which these tests already use as the reference, sidesteps it by truncating the timestamp arithmetically —
time -= mod(time + utcOffset * MS_PER_MINUTE, MS_PER_HOUR)for hour, andtime -= mod(time, unitMs)for minute and second — never touching the ambiguous local->UTC inverse.startOf('day')is not affected and is deliberately left alone: the local day really does begin at the first midnight, which is exactly what the first-occurrence rule returns, and it matches moment'snew Date(y, m, d).Note on overlap: open PR #3040 restructures this same
instanceFactorySet(for the$offsetpath, and to expressendOfas start-of-next minus 1 ms). I checked out its head (e8a7d76) and ran the repro above against it — it still returns3600123withisSame(d, 'second') === false— so it does not cover this bug, but the two will conflict textually and I am happy to rebase on whichever lands first.The fix
instanceFactorySettakes akeepOffsetflag, passed only for hour/minute/second. It reads the local offset before and after the setter and, when they differ, restores the original one — but only after confirming that offset is genuinely in force at the corrected instant:That last clause is what separates a repeated wall clock (fall back — the corrected instant exists at the original offset, so restore it) from a nonexistent one (spring forward — it does not, so leave the runtime's resolution exactly as it was). UTC-mode instances use
setUTC*and have no ambiguity, so they take an early return onto the old path along with day/date — which meansstartOf('day')does not pay for any of this.oldOffset === newOffsetis the fast path: no extraDateis built on a non-DST host, or anywhere away from a transition.Evidence
Fail-then-pass on a rebuilt
origin/dev, then on the built bundle:Regression surface, differential against moment 2.29.2 plus offset-independent invariants — 17,904 instants per zone (3,000 pseudo-random over 1990-2036 plus a dense sweep around every host transition, at both round and 7,919 ms offsets) x 9 units x {local, utc, utcOffset(330), utcOffset(45), utcOffset(-60)} x {startOf, endOf}, across 18 host timezones:
360,075 cases corrected, zero new divergences. Included: Europe/Berlin, Europe/London, Europe/Dublin, America/New_York, America/Whitehorse, America/St_Johns, America/Havana, America/Santiago, America/Sao_Paulo, Australia/Sydney, Australia/Lord_Howe, Pacific/Auckland, Pacific/Chatham, Asia/Tehran, Asia/Gaza, Asia/Kolkata, Africa/Casablanca, UTC.
Separately, a containment sweep (
startOf(u) <= t <= endOf(u)andisSame(self, u)) at 1-minute granularity ±2 h around every host transition 1990-2036 goes to zero violations for second/minute/hour in Europe/Berlin, Europe/London, America/New_York, Pacific/Auckland and Australia/Lord_Howe, with no new violations in any zone.The
timezone-plugin path is untouched: withutc+timezoneloaded,isSame(self, u)violation counts under.tz(...)are identical between the baseline and patched builds.Cost: one extra
getTimezoneOffset()onstartOf/endOfof hour/minute/second, about 20 ns per call on operations already costing ~330 ns on my machine. I measured this several ways and the percentage is not stable enough to quote precisely — well-controlled isolated runs put it around 6%, but it moves a lot with machine load, so treat it as "one extragetTimezoneOffset()" rather than a firm number.day/date/monthand UTC-mode instances take the early return and measure flat. Bundle 2.78 -> 2.86 KB gzipped, against the repo's 2.99 KB size-limit budget.Test
One case in
test/timezone.test.js, the filenpm testalready re-runs under Pacific/Auckland, Europe/London and America/Whitehorse. It finds the host's own one-hour fall backs fromgetTimezoneOffsetbetween 2015 and 2030, samples an instant inside each repeated hour, and asserts agreement with moment plusisSame(self, unit)plus the truncation identities. Host-agnostic by construction: it exercises the bug on the DST legs and finds no transitions (so passes trivially) on a UTC runner. It also passes on the other zones I tried by hand, including Pacific/Chatham, Australia/Lord_Howe, America/Havana, Asia/Tehran, Africa/Casablanca and Pacific/Apia.npm run lintclean.npm testgreen and exits 0: 93 suites, 774 tests (773 before).srcline coverage stays 100% under both a DST host and TZ=UTC, so thelines: 100threshold holds.What I could not verify
startOf('hour')can still land after the instant, soisSame(self, 'hour')can still be false there. Concretely, underTZ=Pacific/Chatham,2035-09-29T14:00:00Zis2035-09-30 03:45 +13:45;startOf('hour')returns04:00andisSame(self, 'hour')isfalse— on both the baseline and this patch (moment returns02:00). This is pre-existing, not introduced: in the containment sweep the patch takes Chatham's hour violations from 1380 to 690, St_Johns' from 4037 to 1298, Gaza's from 2878 to 118, Lord_Howe's to zero, and adds none anywhere. But it does not close that edge — doing so would mean replacinginstanceFactorySetwith moment's timestamp arithmetic, which also moves the day/date and$offsetpaths and is a much larger, riskier patch. The new test samples only one-hour fall backs, so it does not assert on this.minuteandseconddo go to zero violations in every zone I measured, including these.day/date/week/monthcontainment violations (e.g. St_Johns 1239 forday, unchanged at 1239). Those come from the separateinstanceFactorymidnight path, which this patch does not touch..utcOffset(n)instances disagree with moment for every unit, including year and month, on any DST host — and also on non-DST hosts. Pre-existing and untouched (this patch happens to fix 630 of those cases and breaks none); it looks like a separate issue in the utc plugin.srcunderTZ=UTCdips from 99.51% to 98.60%, because the corrective path is by definition unreachable without DST. Line coverage — the enforced threshold — stays at 100% andnpm testexits 0. If a codecov status check is configured on branches rather than lines, this may flag.DST, a time that never existed,DayOfYear set,Should not interpolate characters inside square brackets, andUTC and utcOffseton half-hour-offset hosts. All reproduce identically on untoucheddev(Whitehorse: 6 failed on both, same six names), andnpm testonly runs the full suite under the host default TZ, so CI is unaffected.getTimezoneOffset,new Date(number), arithmetic.