[temporal adapter date-fns] Fix date-only string parsing and setTimezone duck typing#4561
[temporal adapter date-fns] Fix date-only string parsing and setTimezone duck typing#4561rita-codes wants to merge 2 commits intomui:masterfrom
Conversation
rita-codes
requested review from
LukasTy,
atomiks,
colmtuite,
flaviendelangle and
michaldudak
as code owners
Bundle size report
Check out the code infra dashboard for more information about this PR. |
commit: |
rita-codes
added
the
internal
✅ Deploy Preview for base-ui ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for base-ui ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
LukasTy
left a comment
LukasTy
left a comment
There was a problem hiding this comment.
LGTM, but I'd suggest waiting for a second pair of eyes from Flavien.
Claude Opus 4.6 review
Author: @rita-codes | Files changed: 2 (+78 / -15) | CI: All 23 checks passing
Summary
Two targeted bug-fixes in TemporalAdapterDateFns:
-
Date-only string parsing —
"2026-04-06"is parsed by JS as UTC midnight, but the adapter previously extracted components with local getters (getDate(), etc.), causing the date to roll back a day in negative-UTC timezones. The fix detects date-only strings (noTin the input) and switches to UTC getters (getUTCDate(), etc.). -
setTimezoneduck typing — Replacesinstanceof TZDatewithtypeof value.withTimeZone === 'function', which is resilient to duplicate copies of@date-fns/tzin the bundle.
Detailed Analysis
Fix 1: Date-only UTC parsing
| Aspect | Assessment |
|---|---|
| Correctness | The heuristic !value.includes('T') accurately targets ECMA-262 date-only forms (YYYY-MM-DD), which are indeed parsed as UTC. Datetime strings with T are parsed as local time. This is correct per spec. |
| Edge cases — date-only with timezone suffix | A string like "2026-04-06Z" or "2026-04-06+05:30" has no T, would be classified as "date-only", and UTC getters would be used. This is actually correct since those are also UTC midnight per spec (date-only + zone offset). |
| Edge cases — non-ISO strings | Strings like "April 6, 2026" have no T and would be treated as date-only, using UTC getters. The behavior of new Date("April 6, 2026") is implementation-defined (not ISO 8601). However, the date() method is documented to accept ISO strings, so this is acceptable. |
system/default timezone path |
For date-only strings, a new Date(year, month, date) is constructed, which creates a local-time midnight — logically correct: the user said "April 6th", they get April 6th at local midnight. |
| Named timezone path | UTC getters are used to construct TZDate(y, m, d, h, min, s, ms, tz) — correct: extracts the intended face-value date, then anchors it in the target timezone. |
Verdict: Sound fix. The heuristic is aligned with the JS specification.
Fix 2: setTimezone duck typing
| Aspect | Assessment |
|---|---|
| Correctness | typeof value?.withTimeZone === 'function' is a reasonable duck-type check for TZDate objects. It avoids the classic cross-realm / duplicate-package instanceof failure. |
| Behavioral change | The old code had a special path: if value instanceof TZDate && isSystemTimezone, it called this.toJsDate(value). If !(value instanceof TZDate) && isSystemTimezone, it returned value as-is. The new code always calls this.toJsDate(value) when isSystemTimezone, even for plain Date objects. toJsDate on a plain Date simply returns it (if (value instanceof TZDate) ... else return value), so this is functionally equivalent. |
| Risk of false positives | Only objects with a .withTimeZone function will trigger the duck-type path. This is a very specific method name; false positives are negligible in practice. |
Verdict: Clean simplification with correct behavior preservation.
Tests
- 4 new test cases covering: date-only in negative UTC offset, date-only in named timezone, datetime strings still using local getters, and
setTimezoneduck typing with a fakeTZDate-like object. - Tests manipulate
process.env.TZwith properafterEachcleanup. - Coverage is adequate for the changes.
Comment typo fix
The comment in date() was corrected from "whereas we want to create that represents" → "whereas we want to create a date that represents". Note: the same typo still exists in the parse() method in the base-ui PR (line visible in the diff context). This could be flagged as a nit.
Cross-reference with MUI X local copy
The internal copy at packages/x-scheduler-headless/.../TemporalAdapterDateFns.ts already contains both fixes (date-only UTC parsing and duck-typing setTimezone). The PR is upstreaming these proven changes back to base-ui. This is a positive signal — the fixes have been battle-tested in the scheduler package.
One minor discrepancy: the local copy's parse() method at line 177 still has the uncorrected typo "whereas we want to create that represents" — same as the base-ui PR, which only fixes the typo in date() but not in parse().
Potential Concerns
-
Minor: The
isDateOnlyheuristic relies onvaluebeing a string, which is guaranteed by the method signature (value: T extends string | null), so no issue. However, if someone passed a numeric timestamp coerced to string (e.g."1680739200000"), it has noT, would be treated as date-only, and UTC getters would be used. This is an extremely unlikely edge case and the adapter documents ISO string input, so this is acceptable. -
Minor nit: The typo in the
parse()method comment ("whereas we want to create that represents") could be fixed in the same PR for consistency, since thedate()method comment was already corrected. -
No breaking changes — The public API surface is unchanged. Both fixes are purely behavioral corrections.
Merge Readiness Score
| Category | Score |
|---|---|
| Correctness | 9/10 — Both fixes are spec-aligned and logically sound |
| Test coverage | 8/10 — Good coverage; could add a positive-offset timezone test for completeness |
| Code quality | 9/10 — Clean, well-commented, minimal diff |
| Risk | 9/10 — Low risk; fixes proven in x-scheduler first |
| Documentation | 8/10 — Excellent PR description; minor typo in parse() comment left unfixed |
Overall: 8.5/10 — Ready to merge
The PR fixes two real bugs with correct, minimal, well-tested changes that have already been validated in the MUI X scheduler. The only actionable suggestion is to fix the remaining comment typo in parse() while in the neighborhood.
I think we can disreguard that one, because |
4 participants
Fix two issues in
TemporalAdapterDateFns:Date-only string parsing
Date-only strings (e.g.
"2026-04-06") are parsed as UTC midnight by the JS spec. The adapter was using local getters (getDate(),getHours(), etc.) to extract the components, which causes the day to roll back by one in timezones behind UTC (e.g. America/Sao_Paulo).The fix uses UTC getters for date-only strings (no
Tin the input) and keeps local getters for datetime strings (which JS parses as local time).setTimezoneduck typingsetTimezoneusedinstanceof TZDateto detect timezone-capable dates. This fails when multiple copies of@date-fns/tzexist in the bundle (e.g. different versions resolved by the package manager), because the object is a TZDate but from a different constructor.The fix uses duck typing (
typeof value.withTimeZone === 'function') instead ofinstanceof.Tests added
setTimezonewith a non-TZDate object that supportswithTimeZoneNote
These fixes were already applied and validated in the internal copy of this adapter used by
@mui/x-scheduler.