Repository navigation
bun-types: keep generic process.removeListener and off signatures visible with @types/node 24 - #40004
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughChangesProcess listener type compatibility
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This restores valid TypeScript calls to process.off and process.removeListener without changing runtime behavior. The regression coverage and full bun-types checks pass, so no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
@coderabbitai review |
|
|
@coderabbitai review |
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — minimal types-only fix that re-declares the generic off/removeListener signatures so the memoryPressure overloads no longer hide the inherited ones under @types/node@24.
What was reviewed:
- Confirmed the two added signatures match
EventEmitter's exactly, so on@types/node@26(which declares them onProcessdirectly) the merge is a no-op — the existing base-fixture tests cover that path. - Checked the other augmented methods (
on/once/addListener/prependListener/prependOnceListener/emit) are declared onProcessby@types/node@24, so they don't need the same treatment. - The new test follows the existing
Bun.mmapspawned-tsc pattern in the same file, pins the major, and asserts the installed@types/nodeversion to guard against resolution drift making it vacuous.
Extended reasoning...
Overview
This PR fixes a TypeScript declaration-merging regression in packages/bun-types/overrides.d.ts. When bun-types augments NodeJS.Process with memoryPressure-specific overloads for off and removeListener, and the user has @types/node@24 installed (which only inherits those two methods from EventEmitter rather than declaring them on Process), the augmentation hides the inherited generic signatures — so process.removeListener("SIGINT", fn) fails to type-check. The fix re-declares the generic (event: string | symbol, listener: (...args: any[]) => void): this signatures alongside the specific ones. A new integration test in test/integration/bun-types/bun-types.test.ts pins @types/node@24 and runs tsc over calls that exercise both the generic and memoryPressure signatures.
Security risks
None. This is a pure .d.ts change with no runtime effect, plus a test addition.
Level of scrutiny
Low. The production change is two overload signatures plus an explanatory comment. The signatures are copied verbatim from EventEmitter, so on @types/node versions that already declare them on Process (26+), declaration merging adds an identical overload — harmless. The PR description correctly explains why only off and removeListener need this (the other event methods are declared directly on Process in @types/node@24, so bun-types' overloads merge with them rather than shadowing an inherited signature).
Other factors
The test is well-constructed: it follows the exact pattern of the neighboring Bun.mmap spawned-tsc check (writes a tsconfig + fixture into TEMP_DIR, copies the packed bun-types/@types/bun from BASE_FIXTURE_DIR, spawns tsc, drains stdout/stderr/exited concurrently via Promise.all, asserts output before exit code). It explicitly asserts @types/node's installed major is 24. so a future registry retag can't silently make the test vacuous, and it also asserts the memoryPressure overload's typed level parameter still works — i.e. the fix didn't lose the specific typing. The bun add @types/node@24 network call matches this file's existing convention (the same file already installs @typescript/native-preview, @types/react, etc. from the registry). No prior reviewer comments to address.
f0b736f to
beea668
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The two-line .d.ts fix is minimal and the root-cause analysis (derived-interface method declarations hiding inherited EventEmitter overloads under @types/node@24) checks out. Deferring for a human look since packages/bun-types/ has a CODEOWNER.
What was reviewed:
- Confirmed the generic
(event: string | symbol, ...)signatures match whatEventEmitterprovides, so the added overloads are a no-op merge on@types/node@26and only restore the hidden signatures on 24. - Checked the other augmented event methods (
on,once,addListener,prependListener,prependOnceListener,emit) —@types/node@24declares those onProcessdirectly, so they merge without hiding and need no matching change. - The new test pins
@types/node@24, asserts the resolved major to guard against drift, and still exercises the typedmemoryPressurelistener; it follows the same spawned-tscpattern as the neighbouringBun.mmapcheck.
Extended reasoning...
Overview
This PR fixes a TypeScript declaration-merging regression in packages/bun-types/overrides.d.ts. The existing memoryPressure-specific overloads for off and removeListener on NodeJS.Process hid the generic signatures inherited from EventEmitter when used with @types/node@24, causing tsc to reject calls like process.removeListener("SIGINT", fn) (#40003). The fix re-declares the generic (event: string | symbol, listener: (...args: any[]) => void): this signatures alongside the specific overloads. A new integration test pins @types/node@24 and verifies both the generic event names and the typed memoryPressure listener type-check.
Security risks
None. This is a pure type-declaration change with no runtime code path affected. The test only spawns tsc over a fixture.
Level of scrutiny
Low-to-medium. The .d.ts change is two lines that mirror signatures already present on EventEmitter, so the effective API surface is unchanged — it just restores visibility of overloads that were being shadowed. The test is 53 lines and closely follows the existing Bun.mmap / TextDecoder spawned-tsc pattern in the same file, including the bun add from the registry (which this integration test file already does throughout). The PR description's mechanistic explanation of why only off/removeListener are affected (they alone are not re-declared on Process by @types/node@24) is correct and I verified the other five event methods do not need the same treatment.
Other factors
The gating factor for approval is that packages/bun-types/ is owned by a CODEOWNER in .github/CODEOWNERS, so per policy this should get a human sign-off even though the change itself is small and mechanically sound. The evidence block shows the new test fails on main with the exact reported TS2345 error and passes with the fix, and the full 16-test bun-types suite still passes on release. No prior human review comments to address.
|
Updated 2:15 AM PT - Aug 30th, 2026
✅ @robobun, your commit 25976ed8a0c9d403ae9f8becb2c717ae975230c3 passed in 🧪 To try this PR locally: bunx bun-pr 40004That installs a local version of the PR into your bun-40004 --bun |
|
@robobun fix conflicts |
…ible The memoryPressure overloads declared on NodeJS.Process hide the removeListener and off signatures inherited from EventEmitter when @types/node does not declare them on Process directly (@types/node <= 24). Re-declare the generic signatures next to the memoryPressure overloads so any event name still typechecks. Fixes #40003
|
Rebased onto main and resolved the conflicts. Both were in test/integration/bun-types/bun-types.test.ts: main added the TextDecoder (#40119) and Event/EventTarget (#40574) blocks at the same insertion point as the new test, so I kept those blocks and moved the new test after them. The diff against main is unchanged (same 61 added lines). The full bun-types suite passes locally, 20/20. |
beea668 to
7e07e7a
Compare
bun-types 1.4.1 restores the generic process.off signature that 1.4.0 shadowed with its memoryPressure overload (oven-sh/bun#40004), and the root floor now sits at ^1.4.1, so the EventEmitter cast in registerProcessTeardown has nothing left to work around. No runtime change; typecheck and the Application lifecycle tests stay green.
bun-types 1.4.1 restores the generic process.off signature that 1.4.0 shadowed with its memoryPressure overload (oven-sh/bun#40004), and the root floor now sits at ^1.4.1, so the EventEmitter cast in registerProcessTeardown has nothing left to work around. No runtime change; typecheck and the Application lifecycle tests stay green.
* ci: move the Bun trial lane and bun-types to 1.4.1 Bun 1.4.1 shipped today. The trial lane in ci.yml follows it, and the root bun-types floor moves with it. Every other workflow keeps pinning the primary version (1.3.14); scripts/workflow-bun-version.test.ts enforces that and stays green. The lockfile was regenerated on 1.3.14 so the primary lane's --frozen-lockfile install is unchanged. Verified on 1.4.1: bun install --frozen-lockfile, build:clean, typecheck, test:bun (6495 pass / 0 fail), test:examples. * refactor(server): drop the bun-types 1.4.0 process.off cast bun-types 1.4.1 restores the generic process.off signature that 1.4.0 shadowed with its memoryPressure overload (oven-sh/bun#40004), and the root floor now sits at ^1.4.1, so the EventEmitter cast in registerProcessTeardown has nothing left to work around. No runtime change; typecheck and the Application lifecycle tests stay green.
2 participants
Problem
@types/node@24installed, tsc rejectsprocess.removeListener("SIGINT", fn)andprocess.off(...)witherror TS2345: Argument of type '"SIGINT"' is not assignable to parameter of type '"memoryPressure"'(bun-types 1.4.0 memoryPressure overrides break process.removeListener/off with @types/node@24 #40003).packages/bun-types/overrides.d.ts:112-114. It declaresoffandremoveListeneroverloads for thememoryPressureevent onNodeJS.Process.@types/node@24does not declare these two methods onProcess, it only inherits them fromEventEmitter. A method declared on a derived interface hides the inherited overloads, somemoryPressurebecame the only accepted event name.Fix
(event: string | symbol, listener: (...args: any[]) => void): thissignatures foroffandremoveListenernext to thememoryPressureoverloads.EventEmitterprovides on@types/node@24. On@types/node@26, which declares the same generic signatures onProcessdirectly, the merge adds an identical overload and nothing changes.on,once,addListener,prependListener,prependOnceListener,emit) are declared onProcessby@types/node@24, so declaration merging keeps them visible and they need no change.test/integration/bun-types/bun-types.test.tspins@types/node@24and runs tsc overremoveListener/off/memoryPressurecalls. It fails on main with the exact error above. The full bun-types integration suite passes (16/16).Background
bun-typesaugmentsNodeJS.Processinoverrides.d.tsto type the Bun-onlymemoryPressureevent (Add process.on('memoryPressure') event #32594).Processcoexist with the ones@types/nodedeclares there. Hiding only happens for methods the base interface (EventEmitter) declares and the derived interface (Process) does not.@types/node@latest(currently 26), which masks the bug. That is why New memoryPressure event type signature deletes all type signatures for other process events #39807 was closed as unreproducible. The new test pins major 24 and asserts the installed major so resolution drift cannot make it vacuous.ProcessEventMapapproach, bundled with unrelated scripts/build typecheck fixes. This PR is the minimal fix for the reported break on the current@types/nodeLTS.[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 1
evidence per changed file
root cause · written by the author bot
The bug was that the memoryPressure overloads for removeListener and off in overrides.d.ts were the only declarations of those methods on the NodeJS.Process interface under @types/node@24, where Process inherits them solely from EventEmitter; since re-declared methods in a derived interface hide inherited overloads rather than merging with them, the memoryPressure signature became the only one visible, rejecting any other event name. The fix re-declares the generic (eventName: string | symbol, listener: (...args: any[]) => void) overload alongside the memoryPressure overloads, so both the t…