Intl.DateTimeFormat: keep timeStyle literal separators in formatRange for ja/ko/th - #36197
Intl.DateTimeFormat: keep timeStyle literal separators in formatRange for ja/ko/th#36197robobun wants to merge 4 commits into
Conversation
formatRange must render the same literal-separated time pattern as format() for locales like ja/ko/th whose timeFormats use word separators. Also cover that an explicit hourCycle is still honored by formatRange at midnight. The fix lives in JSC's IntlDateTimeFormat (oven-sh/WebKit#366): stop passing a redundant -u-hc- to udtitvfmt_open that trips ICU-22669 in ICU < 76.
WalkthroughChangesIntl formatRange tests
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/web/intl/intl.test.ts`:
- Around line 123-128: Replace the manual parameter loops in both affected tests
with independent test.each matrix cases, preserving the existing
locale/timeStyle and hour-cycle combinations and assertions so each matrix entry
runs and reports separately.
- Around line 130-142: Strengthen the assertions in the hms range tests around
hms, formatRange, formatToParts, and formatRangeToParts to validate both
endpoints: require format(b) to appear verbatim in the range and compare every
relevant hour-following literal (and hour-cycle component) from formatToParts(b)
against the corresponding formatRangeToParts result. Apply the same
complete-component validation to the hour-cycle test matrix rather than
inspecting only the first match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c42505c6-d113-4b45-907e-2ea6323a24f3
📒 Files selected for processing (1)
test/js/web/intl/intl.test.ts
…ints Each locale/timeStyle and hourCycle case now runs as its own test so a single failure doesn't short-circuit the rest. The style-pattern test asserts the full hour/minute/second-following separator list for both startRange and endRange against formatToParts, and the hourCycle test checks the hour field for both range endpoints.
There was a problem hiding this comment.
Test logic looks sound, but this PR is the test half of a two-repo change and doesn't yet include the WEBKIT_VERSION bump — the ja/ko/th formatRange separator tests will fail on Linux CI (bundled ICU 75.1) until oven-sh/WebKit#366 is published and pulled in. Deferring so a human can sequence the merge with the WebKit bump (or mark the first block test.todo in the interim).
What was reviewed:
fieldSeparators/hmshelpers correctly extract the literals following hour/minute/second and compareformatToPartsvsformatRangeToPartsper-endpoint — assertion is non-vacuous (fails on the pre-fix colon output).- The explicit-
hourCycleguard test uses a minute-only-differing range so ICU falls back to full-pattern-both-sides, giving distinctstartRange/endRangehour parts; this exercises pre-existing behavior and should already pass. - Not gated by
snapshotIf, so runs on macOS/Windows too — CLDR-agnostic invariant (formatRange separators == format separators) rather than exact strings, which is the right shape for cross-ICU portability.
Extended reasoning...
Overview
Test-only addition to test/js/web/intl/intl.test.ts: (1) a test.each matrix over ja-JP/ko-KR/th-TH × timeStyle: full|long asserting that formatRange's hour/minute/second separator literals match format's for both endpoints, and (2) a guard test.each over h11|h12|h23|h24 asserting explicit hourCycle is honored in formatRangeToParts at midnight. The actual behavioral fix lives in oven-sh/WebKit#366 (IntlDateTimeFormat::createDateIntervalFormatIfNecessary — only append -u-hc- when the hour cycle was explicitly specified).
Security risks
None. Pure Intl formatting assertions; no I/O, subprocesses, network, or untrusted input.
Level of scrutiny
Low for the test code itself — the assertions are well-constructed, CLDR-version-agnostic (compare format vs formatRange rather than hard-coding "時"/"분"/"นาฬิกา"), and CodeRabbit's earlier feedback (use test.each, assert both endpoints) was addressed in f4f96fb. Higher scrutiny is warranted for the merge sequencing: the PR description states the WEBKIT_VERSION bump "will follow", so as it stands the first test block is expected to fail on Linux CI where ICU 75.1 is bundled and the JSC-side fix isn't yet linked. Merging this without the bump would turn CI red.
Other factors
- The second test (explicit hourCycle) is a guard for behavior that already works today, so it doesn't depend on the WebKit change. Only the ja/ko/th separator block does.
- Tests are not
snapshotIf-gated, so they run on all platforms; the invariant chosen (separator-list equality betweenformatToPartsandformatRangeToParts) is deliberately robust to ICU/CLDR version differences on macOS/Windows, which is appropriate. - No prior claude[bot] review on this PR; CodeRabbit threads are all resolved.
Given the cross-repo dependency and the pending version bump, a human should decide the landing order rather than auto-approving a test-only PR that will fail CI in isolation.
|
Once oven-sh/WebKit#366 is merged to main and its The |
…formatRange fix) Picks up oven-sh/WebKit#366: only pass -u-hc- to UDateIntervalFormat when the hour cycle was explicitly requested, so formatRange keeps the locale's timeStyle literal separators (ja/ko/th) instead of falling back to H:mm:ss. One commit on top of 549170099226f816.
There was a problem hiding this comment.
I didn't find any bugs in the test additions, but this pins WEBKIT_VERSION to a preview tag (autobuild-preview-pr-366-d8348c3d) for an as-yet-unmerged oven-sh/WebKit#366 — the actual fix lives entirely in that PR, not in this diff. A maintainer should review/land the WebKit-side change and decide whether to merge with the preview tag or wait for the final autobuild-<sha>.
What was reviewed:
- New
formatRangetests: assertions are non-vacuous (both endpoints checked, separator lists compared againstformatToParts),test.eachmatrix, CLDR-version-agnostic invariants rather than hardcoded literals. hourCycleguard test: h11/h12/h23/h24 midnight values match ECMA-402 semantics; bothstartRangeandendRangehours asserted.webkit.ts: theautobuild-prefix path inprebuiltUrl/prebuiltDestDiralready handles non-SHA tags, so the pin is mechanically valid.
Extended reasoning...
Overview
This PR has two parts: (1) a one-line WEBKIT_VERSION bump in scripts/build/deps/webkit.ts from a merged commit SHA to autobuild-preview-pr-366-d8348c3d, and (2) ~70 lines of new Intl tests in test/js/web/intl/intl.test.ts covering formatRange/formatRangeToParts timeStyle-separator consistency for ja/ko/th and explicit hourCycle handling. The behavioral fix itself is entirely in oven-sh/WebKit#366 (JSC's IntlDateTimeFormat::createDateIntervalFormatIfNecessary); nothing in Bun's src/ changes.
Security risks
None. No runtime code is modified in this repo; the tests are pure Intl API calls with no I/O, network, or subprocess.
Level of scrutiny
High — not because the diff is complex, but because:
WEBKIT_VERSIONis a repo-wide dependency pin that determines which JSC binary every Bun build downloads. Bumping it to a preview tag for an unmerged upstream PR is a coordination decision (the author's own comment says the tag should be replaced once WebKit#366 lands). This falls squarely under the "Dependencies & vendoring" section of the review guidelines, which calls for maintainer sign-off.- The actual code change under review (the
m_explicitHourCyclelogic in IntlDateTimeFormat.cpp) is not in this diff and cannot be reviewed here.
Other factors
- The test additions themselves look solid: they use
test.eachper the earlier CodeRabbit feedback (now resolved), assert both range endpoints, and compare separator literals structurally rather than pinning CLDR-specific strings, so they should be stable across ICU versions and platforms (macOS system ICU included). - The
h23expected value"00"forhour: "numeric"was checked against ECMA-402 §11.5.7 (2-digit padding whenhourCycleis h23/h24) and matches V8/Node behavior. - No prior
claude[bot]reviews on this PR; all outstanding review threads are resolved.
Given the WebKit pin is temporary by the author's own account and the underlying JSC change needs its own review, deferring to a human.
Problem
Intl.DateTimeFormat.prototype.formatRange(andformatRangeToParts) withtimeStyle: "full"/"long"onja-JP,ko-KR,th-THrenders the time portion with colon separators instead of the locale's literal-separated style pattern, whileformat()on the same formatter renders correctly. The same formatter therefore renders the same instant two different ways depending on whether range endpoints are equal.Same shape for
ko-KR(오후 1시 4분 5초vs오후 1:04:05) andth-TH(13 นาฬิกา 04 นาที 05 วินาทีvs13:04:05). Locales whose time patterns are already colon-based (en/de/fr/zh/ru/he/ar/hi) are unaffected.Cause
IntlDateTimeFormat::createDateIntervalFormatIfNecessaryappends-u-hc-<m_hourCycle>to the locale passed toudtitvfmt_openwheneverm_hourCycle != None.m_hourCycleis derived from the generated pattern insetFormatsFromPattern, so it is always populated when an hour field is present, even when the user never specifiedhourCycle,hour12, or a-u-hc-locale extension. For ja-JP withtimeStyle: "full"the locale becomesja-u-hc-h23.In ICU < 76 (before ICU-22669),
DateTimePatternGenerator::addICUPatternsloads the locale's standard style patterns by creatingDateFormatinstances for each style, and when the locale carries anhourskeyword those instances go through a path that regenerates their patterns from skeletons. For locales whosetimeFormats/fullpattern uses literal separators the regenerated pattern falls back to the colon-separatedavailableFormatsentry, soDateIntervalFormat::initializePatternbuilds its fallback patterns withH:mm:sswhile the single-dateUDateFormat(opened without-u-hc-) still hasH時mm分ss秒.Confirmed against stock ICU 75.1:
Node v26 ships ICU 78 (post-ICU-22669), which is why it renders correctly.
Fix
The fix is entirely on the JSC side: oven-sh/WebKit#366. Store the hour cycle resolved from the locale's
-u-hc-extension and thehourCycleoption separately asm_explicitHourCycle(None when neither was specified), and use that instead of the pattern-derivedm_hourCyclewhen building theUDateIntervalFormatlocale. The skeleton already encodes the 12/24-hour choice, and the locale default covers the h11/h12 and h23/h24 preference within each group, so-u-hc-is only needed when the user explicitly overrode that preference.This PR adds:
format(a)produces for ja/ko/th withtimeStyleappear verbatim informatRange(a, b), and thatformatRangeToPartsagrees withformatToPartson the hour/minute separator literal;hourCycle: "h11"|"h12"|"h23"|"h24"is still honored byformatRangeat midnight (the case the-u-hc-extension exists for).The
WEBKIT_VERSIONbump will follow once oven-sh/WebKit#366 lands and its preview build is published.Verification
Built Bun against local WebKit at the PR branch with ICU 75.1:
test/js/web/intl/intl.test.tspass, including the new pair;hourCyclevariants still render the correct midnight hour informatRange.Note: the automated fail-before/fail-after gate does not apply here because the code change is in
vendor/WebKit/(a prebuilt dependency), notsrc/**; the WebKit autobuild bump is what makes the new test pass.no test proof · iteration 2 · Platform-specific test-only change; deferring to CI.