bun-types: defer Event and EventTarget to lib.dom when it is loaded - #40576
Conversation
With lib.dom loaded, the bare global Event interface merged the Node-style composedPath(): [EventTarget?] with lib.dom's composedPath(): EventTarget[], which made the merged type unsatisfiable. Gate the Node-style members on LibDomIsLoaded, the same pattern used for ReadableStream and TextDecoder. Fixes #40574
|
Warning Review limit reached
On-demand reviews are free for the next 25 days. After that, they cost $0.25 per reviewed file. Or wait 2 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find any bugs — the change follows the existing LibEmptyOr* / LibDomIsLoaded pattern exactly and is covered by both with-dom and without-dom tsc checks. Leaving final sign-off to a human since packages/bun-types/ has a CODEOWNERS entry.
What was reviewed
- Confirmed
BunEvent/BunEventTargetmembers are a straight move of the previous global bodies; only difference is theBun.-qualified listener/option types, which resolve to the same interfaces inbun.d.ts. - Checked the new conditional types match the ~30 sibling
LibEmptyOr*declarations already inglobals.d.ts. - Verified the two new
tsctest cases exercise both branches (lib.dom present →EventTarget[]assignable; absent → tuple type retained) and follow the neighboringBun.mmaptest's spawn/assert shape.
Extended reasoning...
Overview
This is a types-only fix in packages/bun-types/globals.d.ts for #40574: Bun's global Event/EventTarget interface bodies unconditionally merged with lib.dom.d.ts, and the composedPath(): [EventTarget?] tuple signature conflicted with lib.dom's composedPath(): EventTarget[]. The fix relocates the member declarations into Bun.__internal.BunEvent / BunEventTarget and gates them behind LibDomIsLoaded-conditional LibEmptyOrBunEvent / LibEmptyOrBunEventTarget types, so the global interfaces contribute {} when lib.dom is present. Two new tsc-driven tests in test/integration/bun-types/bun-types.test.ts and a satisfies check in fixture/dom.ts cover both configurations.
Security risks
None. This touches only ambient TypeScript declarations and test fixtures that invoke tsc; no runtime code, no I/O, no untrusted input handling.
Level of scrutiny
Low-to-moderate. The change is mechanical and mirrors the exact pattern already used ~30 times in the same file (ReadableStream, WebSocket, TextDecoder, PerformanceEntry, etc. all use interface X extends Bun.__internal.LibEmptyOr...). The moved member list is byte-identical to the removed block aside from Bun.-prefixing the listener/option parameter types, which is required now that the interface lives inside declare module "bun" — I confirmed Bun.EventListener, Bun.EventListenerObject, Bun.AddEventListenerOptions, and Bun.EventListenerOptions all exist in bun.d.ts. The declare var Event / declare var EventTarget constructor declarations are left untouched, which is correct since they were never the source of the conflict.
Other factors
.github/CODEOWNERS assigns /packages/bun-types/ to a specific owner, so per the approval guidelines I'm deferring rather than approving outright. The tests are well-structured (stderr/stdout asserted before exit code, await using on the spawned process, both branches of the conditional covered), and the fixture addition in dom.ts gives ongoing coverage in the main fixture run. No prior review comments exist on the PR.
|
Thanks for the review. The remaining red buildkite lanes are failures that also reproduce without this diff (bun-build-api snapshot hashes, bun-patch on Windows 2019, an ICU-dependent URL test on darwin x64). They are reported separately. The TypeScript types check and the bun-types integration tests are green. |
2 participants
Problem
lib: ["dom"], the globalEventmerges lib.dom'scomposedPath(): EventTarget[]with bun-types'composedPath(): [EventTarget?](packages/bun-types/globals.d.ts:283).ReturnType<Event["composedPath"]>resolves to the tuple, so a real DOM path is rejected:error TS2322: Type 'EventTarget[]' is not assignable to type '[(EventTarget | undefined)?]'.EventTarget(Node,Element,Document) inherits the problem. Fixes bun-types global Event merges with lib.dom, making composedPath() unsatisfiable #40574.Fix
EventandEventTargetmembers intoBun.__internal.BunEventandBunEventTarget, gated onLibDomIsLoaded. The global interfaces extend the gated types, so with lib.dom loaded bun-types contributes{}and lib.dom's declarations win.ReadableStream,CompressionStream, andTextDecoder(LibEmptyOr*onLibDomIsLoaded).test/integration/bun-types/bun-types.test.tsgets two new tsc checks (with and without lib.dom) plus a fixture check infixture/dom.ts. The with-dom check fails on main with the exact TS2322 above. All 19 tests in the file pass with the fix.Background
typeof globalThis extends { onabort: any }(Bun.__internal.LibDomIsLoaded) and contributes an empty type when lib.dom is present.EventandEventTargetwere bare global interfaces, so they merged with lib.dom instead of deferring. Methods with different signatures merge as overloads, which is why tsc reports no merge error, only an unsatisfiable overload pair.declare var Eventanddeclare var EventTargetconstructors stay as they are. Their types are identical to lib.dom's, so the merge of the value declarations was never the problem.[review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
root cause · written by the author bot
The bug was that bun-types declared its Node-style Event and EventTarget members directly on the global interfaces, so when lib.dom was also loaded the declarations merged and the Node-flavoured composedPath(): [EventTarget?] signature conflicted with lib.dom's composedPath(): EventTarget[], making the merged type unsatisfiable by any DOM implementation. The fix moves those member bodies into Bun.__internal.BunEvent and BunEventTarget behind the existing LibDomIsLoaded conditional, following the same pattern already used for types like ReadableStream and TextDecoder. As a result, the global…