Skip to content

Bump WebKit: omit dayPeriod from Intl.DateTimeFormat resolvedOptions for bare 12-hour patterns #36361

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 1 commit into
base: main
Choose a base branch
from
+38 −1
Open

Bump WebKit: omit dayPeriod from Intl.DateTimeFormat resolvedOptions for bare 12-hour patterns #36361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion scripts/build/deps/webkit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* for local mode. Override via `--webkit-version=<hash>` to test a branch.
* From https://github.com/oven-sh/WebKit releases.
*/
export const WEBKIT_VERSION = "34c01d13391e00c06862a3d2c5b7fff350ac87e0";
export const WEBKIT_VERSION = "autobuild-preview-pr-373-0c1a9be2";

/**
* WebKit (JavaScriptCore) — the JS engine.
Expand Down
37 changes: 37 additions & 0 deletions test/js/web/intl/intl.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,43 @@
// ---------------------------------------------------------------------------

describe("Intl.DateTimeFormat", () => {
test("resolvedOptions() omits dayPeriod for a bare 12-hour pattern", () => {
// The AM/PM marker ('a' in the ICU pattern) is governed by hourCycle/hour12,
// not the dayPeriod option. A 12-hour formatter that never asked for dayPeriod
// must not report one, and round-tripping resolvedOptions() must reproduce the
// same output. https://tc39.es/ecma402/#sec-initializedatetimeformat
const t = Date.UTC(2026, 0, 1, 10, 5);
const opts = { timeZone: "UTC", hour: "numeric", minute: "2-digit" } as const;
const a = new Intl.DateTimeFormat("en-US", opts);
const ro = a.resolvedOptions();
const b = new Intl.DateTimeFormat("en-US", ro);
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");

Check warning on line 123 in test/js/web/intl/intl.test.ts

View check run for this annotation

Claude / Claude Code Review

Test asserts CLDR-dependent literal output on all platforms 

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.
Comment on lines +99 to +123

@claude claude Bot Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

  1. On macOS, JSC links whatever libicucore the OS ships — its CLDR data version is not pinned by Bun and drifts per macOS release.
  2. 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.
  3. 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.
  4. Similarly, .find(p => p.value === "AM") returns undefined if the day-period value string differs at all (e.g. lowercase, or with a trailing marker), so undefined?.type yields undefined and .toBe("dayPeriod") fails — with a much less informative message than finding by type would give.
  5. 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.

});

snapshotIf("default", () => {
const out: Record<string, string> = {};
for (const loc of LOCALES) out[loc] = new Intl.DateTimeFormat(loc, { timeZone: "UTC" }).format(0);
Expand Down
Loading