bun-types: accept the { name, cause } options object in the DOMException constructor - #39323
bun-types: accept the { name, cause } options object in the DOMException constructor#39323robobun wants to merge 1 commit into
Conversation
…ion constructor The runtime (JSDOMException.cpp) and Node both accept an options object as the second argument, but the declaration only allowed a name string. The var is now declared through UseLibDomIfAvailable so the extra overload does not conflict with lib.dom.d.ts when that is loaded.
|
Warning Review limit reached
Next review available in: 3 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. 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 (3)
Comment |
|
Status: ready for review at c0e8125. Types and test changes only; no Reproduced by type-checking |
There was a problem hiding this comment.
I reviewed this and found no issues. Deferring approval since packages/bun-types/ has a CODEOWNER.
What was reviewed:
- The new overload matches the runtime (
JSDOMExceptionDOMConstructor::constructreadsname/causefrom an object arg) and@types/node's shape. - Wrapping the
varinUseLibDomIfAvailablefollows the same pattern as the neighboring web globals; confirmed it avoids the duplicate-var-declaration conflict with lib.dom. - Fixture line numbers (globals.ts:340-342, col 37) line up with the three new expected diagnostics in the lib.dom case, and
erris in scope from the earlierconst err = new Error(...). - The
@ts-expect-erroronnew DOMException("asdf", 20)errors under both overload sets, so it holds with and without lib.dom.
Extended reasoning...
Overview
This PR adds a second constructor overload to the DOMException type declaration in packages/bun-types/globals.d.ts so that new DOMException(msg, { name, cause }) type-checks. The declaration is also switched from a bare declare var to the Bun.__internal.UseLibDomIfAvailable<"DOMException", ...> wrapper, matching the pattern used by every other web global in the file. The rest of the diff is test coverage: six new assertions in fixture/globals.ts, three new expected-error entries in the checks with lib.dom.d.ts list of bun-types.test.ts (recording that lib.dom's own signature still rejects the options object when lib.dom is loaded), and one line added to the single-file tsc smoke check that runs on debug builds.
Security risks
None. This is a pure TypeScript declaration change with no runtime code; the tests only drive tsc over fixtures.
Level of scrutiny
Low. The one substantive line is a constructor overload that mirrors both the actual C++ implementation and @types/node. The UseLibDomIfAvailable wrapping is mechanical — the file already has ~30 declarations using the identical shape. I verified the fixture line/column numbers in the new expected-diagnostic entries match where the object literals land after the insertions, that err referenced on line 340 is the Error declared earlier in the fixture, and that the @ts-expect-error guard on new DOMException("asdf", 20) is satisfied under both the with-lib.dom and without-lib.dom configurations (a number matches neither string nor the options object).
Other factors
The PR description includes verification that the full test file passes (15 pass) and that it fails without the .d.ts change, satisfying the fails-for-the-right-reason requirement. packages/bun-types/ is CODEOWNER-gated, which is why I'm deferring rather than approving despite the change being straightforward.
Problem
bun-types,new DOMException("boom", { name: "AbortError", cause: err })fails to compile:error TS2345: Argument of type '{ name: string; cause: Error; }' is not assignable to parameter of type 'string'.The{ name }and{ cause }forms fail the same way.JSDOMExceptionDOMConstructor::construct(src/jsc/bindings/webcore/JSDOMException.cpp:136-153) readsnameandcausefrom an object second argument and falls back to treating the argument as the name string. Node accepts the same form (itstest/parallel/test-domexception-cause.jsruns in Bun's suite), and@types/nodedeclares it (web-globals/domexception.d.ts).new (message?: string, name?: string).Fix
new (message?: string, options?: { name?: string | undefined; cause?: unknown }): DOMExceptionto theDOMExceptiondeclaration. This is the fixing line; the rest of that hunk is re-indentation.Bun.__internal.UseLibDomIfAvailable<"DOMException", ...>, like the other web globals in the file. lib.dom.d.ts also declaresvar DOMException, and TypeScript requires repeatedvardeclarations to have identical types, which the old declaration met by being a copy of lib.dom's. With the wrapper, lib.dom's declaration is used when lib.dom is loaded, so in that configuration the options form stays rejected exactly as before (the three new entries in the "checks with lib.dom.d.ts" list record that), and the overload applies everywhere else.construct()accepts.nameis optional and an explicitundefinedis treated as absent (hencestring | undefined, which also matters underexactOptionalPropertyTypes);causeis stored as given, whatever its type, so it isunknown, matchingErrorOptions.cause. The two overloads are the same two@types/nodedeclares.@types/nodeitself is not affected: itsvar DOMExceptionis conditional ononmessageexisting onglobalThis, whichbun-typesdeclares (index.d.ts:32), so it takes whateverbun-typesdeclares.causetype and a rejected second argument. Without the.d.tschange thechecks without lib.dom.d.tsand tsgo cases fail with the threeTS2345errors above; with it the whole file passes (bun test test/integration/bun-types/bun-types.test.ts, 15 pass).{ name, cause }line and is renamed accordingly.bun bd teston the file: fails without the.d.tschange, 3 pass / 12 skip with it. If test(bun-types): replace the tsgo and Bun.mmap spawns with one whole-fixture tsc run, enforced by a lint #39270 lands first, that block goes away and the fixture lines are the coverage.tsc -p packages/bun-typesis clean.Background
bun-typesand lib.dom.d.ts both declare the web globals. Interfaces merge, but avardeclared twice must have the identical type, sobun-typesdeclares such variables asUseLibDomIfAvailable<Name, T>(bun.d.ts:58): when lib.dom is loaded (detected byonabortexisting onglobalThis) it resolves to lib.dom's type forName, otherwise toT. The cost is that Bun-only surface is unavailable when lib.dom is loaded; the "checks with lib.dom.d.ts" list in bun-types.test.ts is the record of that surface.@types/nodedeclares its web globals astypeof globalThis extends { onmessage: any; X: infer T } ? T : { ... }, deferring to a DOM-like environment when one is present.bun-typesdeclaresonmessageso that@types/nodedefers to it, which is why the declaration users see forDOMExceptionis the one in globals.d.ts and not@types/node's.packages/bun-types, installs it into a copy offixture/and type-checks the fixture with and without lib.dom. Most of those cases drive TypeScript in-process and are skipped on debug builds; the single-file tsc case is the one that runs under either build.Runtime probe
bun 1.4.0:
node 26.3.0 accepts the same inputs; the only difference is that it reports
nameas"undefined"when the options object has noname, which does not affect the declaration (nameis optional either way).