Bump WebKit: omit dayPeriod from Intl.DateTimeFormat resolvedOptions for bare 12-hour patterns - #36361
Bump WebKit: omit dayPeriod from Intl.DateTimeFormat resolvedOptions for bare 12-hour patterns#36361robobun wants to merge 1 commit into
Conversation
…terns Picks up oven-sh/WebKit#373. Any Intl.DateTimeFormat that resolves to a 12-hour cycle (hour with h11/h12, hour12: true, or a 12-hour locale default) was reporting resolvedOptions().dayPeriod === "short" even though no dayPeriod option was passed and the output contains no flexible day period. Feeding resolvedOptions() back into the constructor then requests dayPeriod for real, so the round-trip changes "10:05 AM" to "10:05 in the morning". setFormatsFromPattern in JSC was mapping the AM/PM pattern field 'a' into m_dayPeriod alongside the real flexible-day-period fields 'b'/'B'; 'a' is now left to hourCycle/hour12. formatToParts is unaffected.
|
Status: waiting on the oven-sh/WebKit#373 preview build ( |
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 5:04 AM PT - Jul 29th, 2026
❌ @robobun, your commit 3631703 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 36361That installs a local version of the PR into your bun-36361 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I also checked that prebuiltUrl() and prebuiltDestDir() in scripts/build/deps/webkit.ts already special-case the autobuild- prefix, so the preview tag doesn't double-prefix the release URL or truncate to a colliding cache key. Note the PR description already flags that WEBKIT_VERSION must be switched from the preview tag to the merged sha before this lands.
Extended reasoning...
Verified the non-sha WEBKIT_VERSION value flows correctly through the two consumers in webkit.ts: prebuiltUrl guards startsWith("autobuild-") before prefixing, and prebuiltDestDir strips the prefix rather than slicing to 16 chars, so webkit-preview-pr-373-0c1a9be2{suffix} is a distinct cache dir. No other consumers of cfg.webkitVersion assume a 40-hex sha. This is a WebKit dependency bump with an explicit pre-merge TODO in the description, so leaving for human review.
| expect({ | ||
| dayPeriod: ro.dayPeriod, | ||
| hourCycle: ro.hourCycle, | ||
| original: a.format(t), | ||
| roundTrip: b.format(t), | ||
| }).toEqual({ | ||
| dayPeriod: undefined, | ||
| hourCycle: "h12", | ||
| original: "10:05 AM", | ||
| roundTrip: "10:05 AM", | ||
| }); | ||
|
|
||
| expect( | ||
| new Intl.DateTimeFormat("en-US", { timeZone: "UTC", hour: "2-digit", hourCycle: "h11" }).resolvedOptions() | ||
| .dayPeriod, | ||
| ).toBeUndefined(); | ||
|
|
||
| // When dayPeriod *is* requested it must still be reported. | ||
| expect( | ||
| new Intl.DateTimeFormat("en-US", { timeZone: "UTC", hour: "numeric", dayPeriod: "long" }).resolvedOptions() | ||
| .dayPeriod, | ||
| ).toBe("long"); | ||
|
|
||
| // formatToParts still labels the AM/PM marker as type "dayPeriod" (ECMA-402 Table 16). | ||
| expect(a.formatToParts(t).find(p => p.value === "AM")?.type).toBe("dayPeriod"); |
There was a problem hiding this comment.
🟡 nit: This test hardcodes the CLDR-derived literals "10:05 AM" and p.value === "AM" under plain test(), while every other exact-output check in this file is gated by snapshotIf per the file header (macOS links Apple's libicucore, Windows a different ICU build). The invariants actually under test are CLDR-independent — consider asserting roundTrip: a.format(t) (equality, not a literal) and locating the formatToParts entry by p.type === "dayPeriod" rather than by value.
Extended reasoning...
What this is
The new test at test/js/web/intl/intl.test.ts:99-123 runs on every platform (plain test()) and asserts two CLDR-derived literals: original: "10:05 AM" / roundTrip: "10:05 AM" in the .toEqual object, and .find(p => p.value === "AM") in the formatToParts check. This file's own header states that exact Intl output is CLDR-version-specific — "macOS uses Apple's libicucore and Windows is on a different ICU build, so snapshot diffs there are expected and not a regression" — and scripts/build/deps/webkit.ts confirms darwin uses system ICU (prebuiltIcuLibs returns [] for darwin, commented // darwin: system ICU). Every other exact-format-string check in this file is accordingly gated behind snapshotIf (Linux + process.versions.icu === "75.1" only); this test bypasses that gate.
Step-by-step: how a divergence would manifest
- On macOS, JSC links whatever
libicucorethe OS ships — its CLDR data version is not pinned by Bun and drifts per macOS release. Intl.DateTimeFormat("en-US", { hour: "numeric", minute: "2-digit" })resolves its output pattern from that CLDR data. The en time patterns have changed across ICU releases — most notably CLDR 42 (ICU 72) switched the space before the AM/PM marker to U+202F NARROW NO-BREAK SPACE, and CLDR 43 reverted it.- If a macOS lane's ICU emits anything other than exactly
10:05+ U+0020 +AM, the.toEqual({ ..., original: "10:05 AM", roundTrip: "10:05 AM" })assertion fails. - Similarly,
.find(p => p.value === "AM")returnsundefinedif the day-period value string differs at all (e.g. lowercase, or with a trailing marker), soundefined?.typeyieldsundefinedand.toBe("dayPeriod")fails — with a much less informative message than finding by type would give. - In all of these cases the WebKit fix under test (dayPeriod omitted, round-trip equality preserved) is still correct — only the CLDR literal diverged.
Why this is a nit, not a blocker
Two mitigations make current-CI failure unlikely. First, JSC's IntlDateTimeFormat normalizes U+202F → U+0020 in its output (replaceNarrowNoBreakSpaces, a WebKit web-compat change) — verified empirically that Bun on Linux ICU 75.1 emits codepoints [31 30 3a 30 35 20 41 4d] even though raw CLDR 45 data uses NNBSP, and that normalization is C++-side so it applies on macOS too. Second, en-US "AM" is among the most stable strings in CLDR history. There is also precedent in this file for ungated test() with ICU-backed exact strings (the tr dotted-i case and getCanonicalLocales), so snapshotIf is a convention for multi-locale format sweeps rather than a hard rule. Net: fragile to future macOS ICU drift and inconsistent with the file's documented convention (REVIEW.md: "Copy harness conventions exactly"), but not demonstrably failing today.
Suggested fix
The invariants the WebKit change guarantees — ro.dayPeriod === undefined, ro.hourCycle === "h12", a.format(t) === b.format(t), and the AM/PM part's .type === "dayPeriod" — are all CLDR-independent. Rewriting the two fragile spots removes the uncontrolled-ICU dependency without weakening what's tested:
expect({
dayPeriod: ro.dayPeriod,
hourCycle: ro.hourCycle,
original: a.format(t),
roundTrip: b.format(t),
}).toEqual({
dayPeriod: undefined,
hourCycle: "h12",
original: a.format(t), // non-empty; the literal isn't what's under test
roundTrip: a.format(t), // round-trip EQUALITY is the invariant
});
// find by type, assert the value is present
const dp = a.formatToParts(t).find(p => p.type === "dayPeriod");
expect(dp?.value).toBeTruthy();Alternatively, keep the "10:05 AM" literal but wrap that one assertion in snapshotIf like the rest of the file.
2 participants
Bumps
WEBKIT_VERSIONto pick up oven-sh/WebKit#373.Problem
Any
Intl.DateTimeFormatcomponent formatter that resolves to a 12-hour cycle (hourwith h11/h12,hour12: true, or a 12-hour locale default) reportsresolvedOptions().dayPeriod === "short"even though nodayPeriodoption was passed and the output contains no flexible day period. FeedingresolvedOptions()back into the constructor (the ECMA-402 round-trip idiom) then genuinely requestsdayPeriod, so the same time formats differently:h23/h24 and the
timeStylepath are unaffected.Cause
IntlDateTimeFormat::setFormatsFromPatternin JSC maps the resolved ICU pattern back into the fields thatresolvedOptions()reports. It was treating'a'(the AM/PM marker, emitted whenever the resolved hour cycle is h11/h12) the same as'b'/'B'(the flexible day-period fields that the ECMA-402dayPeriodoption actually requests).buildSkeletonmapsdayPeriodback to'B', so the round-trip swaps the AM/PM marker for a flexible day period.Fix
'a'no longer populatesm_dayPeriod; only'b'/'B'do.m_dayPeriodis read exclusively byresolvedOptions(), soformatToParts(which mapsUDAT_AM_PM_FIELDto the"dayPeriod"part type independently) is unaffected. This matches V8.Verification
New test in
test/js/web/intl/intl.test.tscovers the undefineddayPeriod, the round-trip equality, the explicit-dayPeriodcase still being reported, and theformatToPartspart type.Also in this range (oven-sh/WebKit 34c01d1339..)
Nothing else; oven-sh/WebKit#373 is directly on top of the current
WEBKIT_VERSION.Note
WEBKIT_VERSIONcurrently points at the preview tagautobuild-preview-pr-373-0c1a9be2while oven-sh/WebKit#373 is open. It will be switched to the merged main sha before this lands.no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.