Intl.DateTimeFormat: accept legacy IANA primary zones (CET, CST6CDT, EET, EST5EDT, MET, MST7MDT, PST8PDT, WET) on Linux/Windows#30620
Conversation
|
Updated 1:06 AM PT - Jun 17th, 2026
❌ @robobun, your commit ba7936d has 1 failures in 🧪 To try this PR locally: bunx bun-pr 30620That installs a local version of the PR into your bun-30620 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughUpdates default WebKit build tag to ChangesWebKit and Intl Updates
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-datetimeformat.test.ts`:
- Around line 18-21: Replace the parameterized test using test.each with a
describe.each block: locate the block referencing legacyPrimaryZones and the
creation of fmt (new Intl.DateTimeFormat("en-US", { timeZone: zone })), change
test.each(legacyPrimaryZones) to describe.each(legacyPrimaryZones)("%s is a
valid IANA primary zone", zone => { and move the actual assertion
(expect(fmt.resolvedOptions().timeZone).toBe(zone)) into an inner test/it block
(e.g., test("resolves to the zone", () => { ... })) so the parameterization is
provided by describe.each while the assertion runs in a standard test.
🪄 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: 040cfd74-6f4a-4f3c-929a-f404a71b3edf
📒 Files selected for processing (2)
scripts/build/deps/webkit.tstest/js/web/intl-datetimeformat.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-datetimeformat.test.ts`:
- Around line 3-74: Add direct toLocaleString assertions covering the same
legacyPrimaryZones to mirror the Intl.DateTimeFormat checks: for each zone in
legacyPrimaryZones assert new
Date("2024-06-15T12:00:00Z").toLocaleString("en-US", { hour: "2-digit", minute:
"2-digit", hourCycle: "h23", timeZone: zone }) returns the same formatted times
used in the "legacy primary zones apply the expected offset + DST" test, and add
a small test that calling toLocaleString with an invalid zone (e.g. "BogusZone")
throws a RangeError; place these alongside the existing tests in the same
describe block so they exercise Date.prototype.toLocaleString in addition to
Intl.DateTimeFormat.
🪄 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: 4a227f0c-0a9c-4f25-aaa9-b15f2293b1bb
📒 Files selected for processing (1)
test/js/web/intl-datetimeformat.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| * From https://github.com/oven-sh/WebKit releases. | ||
| */ | ||
| export const WEBKIT_VERSION = "5488984d20e0dbfe4be2c3ba8fb18eb81a5e0e8b"; | ||
| export const WEBKIT_VERSION = "autobuild-preview-pr-227-65540d6a"; |
There was a problem hiding this comment.
🔴 WEBKIT_VERSION is set to autobuild-preview-pr-227-65540d6a — a CI preview-release tag for the not-yet-merged oven-sh/WebKit#227 — rather than a merged commit SHA. If this lands on main as-is, scripts/sync-webkit-source.ts will fail (it does git checkout ${WEBKIT_VERSION} after pulling main, and compares it against git rev-parse HEAD which is always a SHA), process.versions.webkit will be the tag string instead of the documented "clean commit hash", and main will depend on an ephemeral preview-release asset that may be GC'd once WebKit#227 merges. This should be swapped for the merged WebKit commit SHA before merge.
Extended reasoning...
What the bug is
The doc comment immediately above this constant says "WebKit commit — determines prebuilt download URL + what to checkout for local mode", and every prior value in git history has been a 40-char commit SHA (5488984d20e0…, etc.). This change sets it to autobuild-preview-pr-227-65540d6a, which is a GitHub Releases tag for a CI preview build of an unmerged WebKit PR — it is neither a commit SHA nor a ref that exists on main of oven-sh/WebKit. The PR description itself notes the fix lives in oven-sh/WebKit#227, so this is presumably a temporary value to let CI fetch the preview tarball while that PR is in flight; but nothing in the diff marks it as such, and merging it to main would break three downstream consumers of WEBKIT_VERSION.
Code paths that break
1. scripts/sync-webkit-source.ts (local-WebKit dev workflow):
const checkedOutCommit = (await Bun.$`git rev-parse HEAD`.text()).trim();
const { WEBKIT_VERSION: expectedCommit } = await import("./build/deps/webkit.ts");
if (checkedOutCommit == expectedCommit) { … } // (a)
…
await Bun.$`git checkout main`;
await Bun.$`git pull`;
await Bun.$`git checkout ${expectedCommit}`; // (b)(a) git rev-parse HEAD always returns a 40-char SHA, so comparing it to the string autobuild-preview-pr-227-65540d6a can never be true — even when the user is sitting at commit 65540d6a…, the script will think it needs to switch.
(b) After git checkout main && git pull, the only refs guaranteed to be present are main and whatever was already fetched. autobuild-preview-pr-227-65540d6a is a release tag pointing at a PR-branch commit; git pull on main does not fetch it, so git checkout autobuild-preview-pr-227-65540d6a fails with error: pathspec '…' did not match any file(s) known to git. Local-WebKit developers' sync is broken.
2. scripts/build/depVersionsHeader.ts:54-59:
// WebKit special case: its `prebuilt.identity` includes the suffix
// (e.g. "-debug-asan"), but process.versions should show the clean
// commit hash. Use cfg.webkitVersion instead …
if (dep.name === "WebKit") {
versions.push([dep.versionMacro, cfg.webkitVersion]);
}This emits cfg.webkitVersion verbatim into bun_dependency_versions.h, which surfaces as process.versions.webkit (and via src/js/internal/debugger.ts in the inspector's WebKit-Version field). The comment explicitly documents the contract as "clean commit hash"; with this change it becomes the preview-tag string.
3. Ephemeral release asset / degenerate cache key:
prebuiltUrl() does have a version.startsWith("autobuild-") branch, so the CI tarball download works today. But preview-PR releases on oven-sh/WebKit are temporary — once #227 merges and the real autobuild-<sha> release is cut, the preview asset is liable to be deleted, at which point every fresh build of bun main 404s on the WebKit tarball. Additionally, prebuiltDestDir() keys the cache on cfg.webkitVersion.slice(0, 16) → "autobuild-previe", a degenerate key that would collide with any other preview tag.
Why nothing guards against this
There is no format validation on WEBKIT_VERSION. The startsWith("autobuild-") branch in prebuiltUrl() exists to support the --webkit-version=<hash> CLI override for testing branches (per the doc comment on line 3), not for the committed default. git log -S autobuild-preview shows zero precedent — this is the first time a non-SHA has been committed here. .claude/commands/upgrade-webkit.md and CONTRIBUTING.md both instruct "Update WEBKIT_VERSION … to the commit SHA".
Step-by-step proof
- Merge this PR to
mainas-is. - A contributor with
vendor/WebKit/checked out at the previous SHA runsbun scripts/sync-webkit-source.ts. - Line 14:
checkedOutCommit = "5488984d20e0dbfe4be2c3ba8fb18eb81a5e0e8b". - Line 15:
expectedCommit = "autobuild-preview-pr-227-65540d6a". - Line 17:
"5488984d…" == "autobuild-preview-…"→ false. - Lines 21–22:
git checkout main && git pull— fetchesmainonly. - Line 24:
git checkout autobuild-preview-pr-227-65540d6a→ fails, the ref does not exist locally. - Separately, build bun and inspect
process.versions.webkit→"autobuild-preview-pr-227-65540d6a"instead of a commit hash.
Fix
Before merging this PR, land oven-sh/WebKit#227 first, then replace this line with the merged commit's 40-char SHA:
export const WEBKIT_VERSION = "<merged-sha-of-WebKit#227>";(Or mark this PR draft / DO-NOT-MERGE until the WebKit side lands.)
|
CI status: diff is green. The Windows 2019 lane hit the same unrelated flake twice — |
robobun
force-pushed
the
farm/eb3d4eac/intl-datetimeformat-legacy-zones
branch
from
622fc24 to
669bdc1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@scripts/build/deps/webkit.ts`:
- Line 6: The current WEBKIT_VERSION values like "autobuild-preview-..." collide
because prebuiltDestDir() truncates cfg.webkitVersion to 16 chars causing
different preview tags to map to the same cache dir; update prebuiltDestDir() to
use a longer prefix (or the full cfg.webkitVersion) when constructing the
destination directory so each unique cfg.webkitVersion (including
WEBKIT_VERSION) yields a distinct cache path — locate the usage in
prebuiltDestDir and replace the 16-char truncation logic on cfg.webkitVersion
with a safer approach (e.g., no truncation or a longer, collision-resistant
slice) so preview builds no longer reuse stale artifacts.
🪄 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: 41fa8722-19e0-4116-9c42-972f2deb726a
📒 Files selected for processing (1)
scripts/build/deps/webkit.ts
|
Latest CI (build 54356, retrigger pushed onto rebased branch) hit a different known flake — |
robobun
force-pushed
the
farm/eb3d4eac/intl-datetimeformat-legacy-zones
branch
from
669bdc1 to
12f3777
Compare
|
Rebased onto main (incl. ICU 75 → main's WebKit 3167a44). WebKit PR #227 rebased too (oven-sh/WebKit@4a8a597951); |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/build/deps/webkit.ts (1)
6-6:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid cache-dir collisions with
autobuild-preview-*version tags.This new tag format collides with the current
prebuiltDestDir()keying (slice(0, 16)), so different WebKit revisions can share the same extraction directory and reuse stale artifacts.Suggested fix
function prebuiltDestDir(cfg: Config): string { - const version16 = cfg.webkitVersion.slice(0, 16); + const versionKey = cfg.webkitVersion.replace(/[^A-Za-z0-9._-]/g, "_"); // Cross-compiled targets share a host (and cache dir) with native builds, // so include os+arch in the key — otherwise a FreeBSD/arm64 extraction // collides with a Linux/x64 one at the same WebKit version. const osKey = cfg.freebsd ? "-freebsd" : cfg.abi === "android" ? "-android" : ""; const archKey = cfg.arm64 ? "-arm64" : ""; - return resolve(cfg.cacheDir, `webkit-${version16}${osKey}${archKey}${prebuiltSuffix(cfg)}`); + return resolve(cfg.cacheDir, `webkit-${versionKey}${osKey}${archKey}${prebuiltSuffix(cfg)}`); }🤖 Prompt for 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. In `@scripts/build/deps/webkit.ts` at line 6, The current autobuild-preview-* WEBKIT_VERSION values collide because prebuiltDestDir() currently keys extraction dirs using version.slice(0, 16); update the keying so it cannot collide for long auto-build tags—either stop truncating (use the full WEBKIT_VERSION) or increase uniqueness (e.g., include a longer slice or a hash of WEBKIT_VERSION) in prebuiltDestDir(), and ensure any callers of prebuiltDestDir() continue to use the new key format. Use the symbols WEBKIT_VERSION and prebuiltDestDir() (and the existing slice(0, 16) logic) to locate the code to change.
🤖 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.
Duplicate comments:
In `@scripts/build/deps/webkit.ts`:
- Line 6: The current autobuild-preview-* WEBKIT_VERSION values collide because
prebuiltDestDir() currently keys extraction dirs using version.slice(0, 16);
update the keying so it cannot collide for long auto-build tags—either stop
truncating (use the full WEBKIT_VERSION) or increase uniqueness (e.g., include a
longer slice or a hash of WEBKIT_VERSION) in prebuiltDestDir(), and ensure any
callers of prebuiltDestDir() continue to use the new key format. Use the symbols
WEBKIT_VERSION and prebuiltDestDir() (and the existing slice(0, 16) logic) to
locate the code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c238c4d6-acc7-4a73-83f8-75f809fbd3e6
📒 Files selected for processing (1)
scripts/build/deps/webkit.ts
robobun
force-pushed
the
farm/eb3d4eac/intl-datetimeformat-legacy-zones
branch
2 times, most recently
from
548f326 to
6b96dca
Compare
|
CI status: build 55981 red on Darwin 26 aarch64 — |
robobun
force-pushed
the
farm/eb3d4eac/intl-datetimeformat-legacy-zones
branch
2 times, most recently
from
bea947a to
5d4bfb3
Compare
robobun
force-pushed
the
farm/eb3d4eac/intl-datetimeformat-legacy-zones
branch
from
d328534 to
f34c7b7
Compare
|
CI red on Windows 2019 test-bun for |
robobun
force-pushed
the
farm/eb3d4eac/intl-datetimeformat-legacy-zones
branch
from
f34c7b7 to
fbb7672
Compare
robobun
force-pushed
the
farm/eb3d4eac/intl-datetimeformat-legacy-zones
branch
from
fbb7672 to
97b7fbf
Compare
|
Rebased onto main again ( To land this cleanly: merge oven-sh/WebKit#227 (17-line |
robobun
force-pushed
the
farm/eb3d4eac/intl-datetimeformat-legacy-zones
branch
from
97b7fbf to
cb50d74
Compare
Bump WebKit to oven-sh/WebKit#227, which lets `Intl.DateTimeFormat`/`Date.prototype.toLocaleString` accept the 8 non-'/' IANA primary time zones: CET, CST6CDT, EET, EST5EDT, MET, MST7MDT, PST8PDT, WET These are first-class `Zone`s in the IANA `etcetera` file, so they are primary identifiers — but JSC's `isValidTimeZoneNameFromICUTimeZone` dropped every non-'/' name except `UTC`/`GMT`. On Linux/Windows (bundled ICU 75) they were therefore rejected with RangeError: invalid time zone: CET macOS was unaffected (system libicucore has a different canonical table). Per test262 `intl402/DateTimeFormat/timezone-case-insensitive.js`, each of these identifiers must round-trip to itself through `resolvedOptions().timeZone`; this also makes them appear in `Intl.supportedValuesOf('timeZone')`. Fixes #30618
macOS links against the system libicucore, whose canonical-zone table
does not list the 8 legacy primaries (CET, CST6CDT, ..., WET). They
*are* accepted as inputs and round-trip through
resolvedOptions().timeZone (the other tests already cover that), but
Intl.supportedValuesOf('timeZone') only enumerates them on the bundled
ICU shipped with Linux/Windows.
Keep the assertion where it has teeth; skip it on Darwin.
Per test/CLAUDE.md — add coverage to the existing module test file rather than a new top-level file. The zones are grouped under `Intl.DateTimeFormat timeZone option` immediately after the existing `Intl.DateTimeFormat` describe that covers snapshot + timeZoneName behaviour. Also switch the macOS guard to `isMacOS` from `harness` to match the file's existing `isLinux` import.
robobun
force-pushed
the
farm/eb3d4eac/intl-datetimeformat-legacy-zones
branch
from
cb50d74 to
ba7936d
Compare
1 participant
Repro
Same failure for
CST6CDT,EET,EST5EDT,MET,MST7MDT,PST8PDT,WET.Cause
These 8 identifiers are first-class
Zones in the IANAetceterafile — not backward links. They are primary IANA time zone identifiers. ICU agrees (isSystem=1,canonical(CET) = CET,iana(CET) = CET, etc.).But
isValidTimeZoneNameFromICUTimeZoneinSource/JavaScriptCore/runtime/IntlObject.cppdropped every non-/zone name exceptUTC/GMT. These 8 therefore never enteredintlAvailableTimeZones()(the primary list). They could not be found via the case-insensitiveintlAvailableTimeZoneIndexeither, becausetoPrimaryIanaTimeZoneIdentifier("CET")returns"CET"unchanged (no backward link) and"CET"is not in the primary table, so the index-building loop skips them.intlAvailableNamedTimeZonethen returnedstd::nulloptandIntlDateTimeFormat::initializeDateTimeFormatthrew theRangeError.EST,MST,HSThappened to work (but only through the index path) because ICU canonicalises them toEtc/GMT+5,Etc/GMT+7,Etc/GMT+10, all of which are primaries.macOS was unaffected because it links against the system
libicucore, which has a different canonical table.Fix
oven-sh/WebKit#227 whitelists the 8 names in
isValidTimeZoneNameFromICUTimeZone, so they enter the primary table and are found by the direct binary search in bothintlResolveTimeZoneIDandintlAvailableNamedTimeZone. Case-insensitive lookups still normalise to the canonical casing (cet→CET), and the 8 zones now appear inIntl.supportedValuesOf("timeZone")(total 445 → 453).Per test262
intl402/DateTimeFormat/timezone-case-insensitive.js, each of these identifiers is expected to round-trip to itself throughresolvedOptions().timeZone.Verification
With
USE_SYSTEM_BUN=1(pre-fix): 11 of 13 fail (RangeError: invalid time zone: CETetc.).Fixes #30618