bun-types: drop undefined from the Null* subprocess aliases where the default is "pipe" - #39303
bun-types: drop undefined from the Null* subprocess aliases where the default is "pipe"#39303robobun wants to merge 1 commit into
Conversation
… default is "pipe" An undefined stdio option means the slot's default. Bun.spawn defaults stdout to "pipe" and Bun.spawnSync defaults both stdout and stderr to "pipe", so a process spawned with undefined in one of those slots has a live ReadableStream or a Buffer there, and NullSubprocess["stdout"], NullSyncSubprocess["stdout"] and NullSyncSubprocess["stderr"] came out as ReadableStream | undefined and Buffer | undefined instead of undefined. Remove undefined from those three type arguments. stdin and stderr of Bun.spawn default to "ignore" and "inherit", so undefined stays allowed there. The bun-types test gains a tsc run over fixture/spawn.ts that also runs on debug builds, where the in-process type checks are skipped.
|
Warning Review limit reached
Next review available in: 9 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 |
|
Updated 10:12 PM PT - Aug 15th, 2026
✅ @robobun, your commit cb7ecd2496ab13f879a2a4ac8226e4b2acf0ec52 passed in 🧪 To try this PR locally: bunx bun-pr 39303That installs a local version of the PR into your bun-39303 --bun |
|
Status: ready for review. Bot review had no findings; self-review is in progress and CI is running. Reproduced with tsc 6.0.2 against main's bun.d.ts: the new assertions in test/integration/bun-types/fixture/spawn.ts report With the fix, Related open bun-types spawn PRs: #39279, #39283, #39297 (same added test case, byte-identical) and #39270. This branch merges cleanly with each, and fixture/spawn.ts type-checks in each merged tree. |
There was a problem hiding this comment.
LGTM — tightens the Null* subprocess aliases so their stdout/stderr slots resolve to exactly undefined.
What was reviewed:
- Verified
ReadableToIO/ReadableToSyncIOmap anundefinedoption to a stream /Buffer, and that thespawn/spawnSynctype-parameter defaults are["ignore","pipe","inherit"]and["ignore","pipe","pipe"]— so droppingundefinedfrom exactly the three "pipe"-default slots is correct. - Checked the zero-arg
expectType<T>().is<U>()overload infixture/utilities.tssupports the new assertions, and that the new fixture lines land below the pinnedspawn.ts:62/:107lib.dom diagnostics. - Grepped for
NullSubprocess/NullSyncSubprocess— no in-repo consumers outside the fixture.
Extended reasoning...
Overview
This PR touches three files: packages/bun-types/bun.d.ts (removes undefined from three type arguments in the NullSubprocess and NullSyncSubprocess aliases and expands their JSDoc), test/integration/bun-types/fixture/spawn.ts (17 new lines of type assertions covering the property types and positive/negative assignability), and test/integration/bun-types/bun-types.test.ts (a new Bun.spawn > fixture/spawn.ts type-checks case that spawns tsc over the fixture so it also runs on debug builds).
Security risks
None. This is a pure .d.ts change plus type-check fixture assertions; no runtime code, no native code, no I/O changes.
Level of scrutiny
Low. The change narrows two convenience type aliases to match documented and observed runtime behavior. I cross-checked the claims against the source: ReadableToIO<X> and ReadableToSyncIO<X> at bun.d.ts:7439/:7445 treat undefined like "pipe"; the spawn overloads at :7714/:7749 default Out to "pipe" and Err to "inherit"; the spawnSync overloads at :7786/:7822 default both to "pipe". So the previous undefined in NullSubprocess's stdout slot and both NullSyncSubprocess slots did widen those properties to ReadableStream | undefined / Buffer | undefined, and removing it makes them exactly undefined while keeping undefined in the stdin/stderr slots whose defaults already produce undefined. The reasoning in the PR description is thorough and matches the code.
Other factors
- The fixture additions include both directions: exact property-type checks via
expectType<...>().is<undefined>(), positiveexpectAssignablecases forundefinedin the stdin/stderr slots, and@ts-expect-errorfor the three "pipe"-default slots. I confirmed the zero-argexpectType<T>()overload exists infixture/utilities.ts. - The new fixture lines are appended after line 187, so the two pinned lib.dom diagnostics at
spawn.ts:62/:107in the "checks with lib.dom.d.ts" case are unaffected. NullSubprocess/NullSyncSubprocesshave no in-repo consumers outside the fixture (grepped), so no other code needs updating.- The added
bun-types.test.tscase follows the exact pattern of the neighboringBun.mmapcase (sametscspawn shape, samePromise.allpipe drain, stderr/stdout/exitCode assertion order) and per the description is byte-identical to the one in three sibling PRs to avoid merge conflicts.
Problem
NullSubprocessandNullSyncSubprocess(packages/bun-types/bun.d.ts:8161 and :8169 on main) listundefinedin every stdio type argument, soNullSubprocess["stdout"]isReadableStream<Uint8Array<ArrayBuffer>> | undefinedandNullSyncSubprocess["stdout"]/["stderr"]areBuffer | undefined, notundefined.undefinedin a stdio option means the slot's default, and the default is"pipe"forBun.spawn's stdout and for bothBun.spawnSyncslots, soBun.spawn(cmd, { stdio: ["ignore", undefined, "ignore"] })has a liveReadableStreamon.stdoutandBun.spawnSync(cmd, { stdio: ["ignore", undefined, undefined] })has twoBuffers, yet both are accepted asNullSubprocess/NullSyncSubprocess.Bun.spawndefaults stdin to"ignore"and stderr to"inherit", soundefinedthere leaves nothing on the process.Fix
undefinedfrom the three type arguments whose slot defaults to"pipe":NullSubprocess's stdout, and both ofNullSyncSubprocess's. The stdin and stderr arguments ofNullSubprocessstill acceptundefined. The JSDoc of both aliases now says which options qualify and whyundefinedonly counts in some slots.ReadableToIO/ReadableToSyncIOalready turn anundefinedoption in these slots into a stream /Buffer: with the change, all three properties become exactlyundefined, and aSubprocess/SyncSubprocesswith a"pipe"-by-default slot no longer satisfies the alias.undefinedstdout (orspawnSyncstderr) to aNull*alias, which is the case the alias mistyped. Nothing in this repo uses the aliases outside the fixture, and the docs do not mention them.NullSubprocessones): the three properties areundefined,undefinedis still accepted inBun.spawn's stdin and stderr slots, and the three"pipe"-by-default cases are rejected via@ts-expect-error. Against the unfixed bun.d.ts the new lines produce 6 diagnostics (3 wrong property types, 3 unused@ts-expect-error); with the fix, zero.bun bd test test/integration/bun-types/bun-types.test.ts: the newBun.spawn > fixture/spawn.ts type-checkscase fails with those 6 diagnostics when packages/ is stashed and passes with the fix.USE_SYSTEM_BUN=1 bun test test/integration/bun-types/bun-types.test.ts: 16 pass, including the lib.dom run, whose pinned spawn.ts:62 / :107 diagnostics are unchanged (the new lines are below them).git merge-treeof this branch with each of those (and with test(bun-types): replace the tsgo and Bun.mmap spawns with one whole-fixture tsc run, enforced by a lint #39270) reports no conflicts, and fixture/spawn.ts still type-checks in each merged tree. bun-types: type Subprocess.stderr as undefined when the stderr option is undefined #39297'sNullSubprocess["stderr"]assertion relies on stderr keepingundefined, which this PR preserves.Background
Bun.spawnreturnsSubprocess<In, Out, Err>andBun.spawnSyncreturnsSyncSubprocess<Out, Err>, where the type arguments are the literal types of the stdio options passed.proc.stdoutis typed asReadableToIO<Out>(aReadableStreamfor"pipe", anumberfor an fd, otherwiseundefined), andSyncSubprocess.stdoutasReadableToSyncIO<Out>(Bufferfor"pipe", otherwiseundefined). Both map anundefinedoption like"pipe".NullSubprocess,PipedSubprocess,NullSyncSubprocessand friends are aliases that instantiate those interfaces with a union of options, for holding processes whose exact configuration is not known statically. Because the mapping types distribute over the union, each property of the alias is the union of what every listed option produces, and a process is assignable to the alias when its own options (or what they produce) fit in those unions.undefined(explicitly, or through a union such as astdiotuple with a conditional entry) leaves the runtime default for that slot:["ignore", "pipe", "inherit"]forBun.spawnand["ignore", "pipe", "pipe"]forBun.spawnSync(src/runtime/api/bun/js_bun_spawn_bindings.rs, the@defaultlines onstdio/stdout/stderrin bun.d.ts). Omitting the option altogether does not produce anundefinedtype argument: the spawn signatures default the type parameters to those same values.Runtime check of the defaults (bun 1.4.0)
tsc 6.0.2 over fixture/spawn.ts against the unfixed bun.d.ts