bun-types: type Subprocess stdio properties by what the runtime exposes for each stdio option - #39283
bun-types: type Subprocess stdio properties by what the runtime exposes for each stdio option#39283robobun wants to merge 1 commit into
Conversation
… stdio option Spawn.ReadableToIO, ReadableToSyncIO and WritableToIO claimed that a BunFile, ArrayBufferView, Blob, Request or Response passed as stdio shows up on the Subprocess as a number. Only a caller-supplied file descriptor (a number or Bun.file(fd)) does, and even that is undefined when it is the parent's own standard stream; a ReadableStream passed as stdin (directly or as a Request/Response body) comes back as the stream; everything else is undefined. spawnSync also returns the fd number for fd stdout/stderr, which ReadableToSyncIO typed as undefined. Map fd inputs to number | undefined, stream inputs to ReadableStream | undefined and the rest to undefined, document the mapping on the types and the Subprocess/SyncSubprocess properties, fix the fixture assertions that encoded the old claim, and type-check fixture/spawn.ts on debug builds too so the assertions run there.
|
Warning Review limit reached
Next review available in: 13 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 (4)
Comment |
|
Status: ready for review. Reproduced on the released bun 1.4.0 (linux x64) by spawning Overlaps to be aware of when merging: #39279 adds the same debug-build |
There was a problem hiding this comment.
I reviewed this and didn't find any bugs. Since it changes the public bun-types surface for Subprocess stdio (widening several property types and adding ReadableStream to WritableIO), and packages/bun-types/ has a CODEOWNER, a human sign-off is still worth having — the description also flags coordination with #39279 and #34864.
What was reviewed:
- The three conditional mapping types (
ReadableToIO,ReadableToSyncIO,WritableToIO) against the runtime behavior described forWritable::to_js/Readable::to_js/Stdio::extract_blob. - Conditional-type ordering:
BunFile extends Blob, so thenumber | BunFilearm correctly matches before the fall-throughundefinedarm;ArrayBufferView/Blobnow fall through toundefinedas intended. - The holder-type unions pinned in the fixture and mirrored in
docs/runtime/child-process.mdx. - The new
bun-types.test.tscase follows the existingBun.mmaptsc-spawn pattern and drains stdout/stderr concurrently.
Extended reasoning...
Overview
This PR corrects three conditional mapping types in packages/bun-types/bun.d.ts (SpawnOptions.ReadableToIO, ReadableToSyncIO, WritableToIO) so that Subprocess.stdin/stdout/stderr and SyncSubprocess.stdout/stderr are typed by what the runtime actually returns for each stdio option. It adds ReadableStream to WritableIO, widens fd/BunFile inputs to number | undefined, and moves Blob/ArrayBufferView inputs to the undefined arm. It adds JSDoc to the mapping types and to the Subprocess/SyncSubprocess stdio properties, updates the Reference block in docs/runtime/child-process.mdx, replaces three incorrect fixture assertions with ~90 lines of exact-type assertions in test/integration/bun-types/fixture/spawn.ts, and adds a spawned-tsc test case in bun-types.test.ts so the fixture is exercised on debug builds.
Security risks
None. This is a .d.ts change plus docs and type-fixture assertions; no executable runtime code changes.
Level of scrutiny
Medium. The change is a correctness fix that aligns declared types with observed runtime behavior (the PR includes a runtime probe table and points at the exact Rust functions that produce each value). However, it changes the public @types/bun surface: Bun.Subprocess["stdin"] gains ReadableStream, SyncSubprocess["stdout"] gains number, and callers who previously got number for stdin: Bun.file(...) / stdin: fd will now get number | undefined. The author already checked blast radius (cd test && bun run typecheck before/after), but user-facing type widenings on a heavily-used API are the kind of thing the packages/bun-types/ CODEOWNER should confirm.
Other factors
packages/bun-types/is owned by a CODEOWNER in.github/CODEOWNERS, which per the approval guidelines means I should not auto-approve.- The PR description explicitly notes it interacts with two open PRs (#39279 asserts
stdin: 0→number, which this changes tonumber | undefined; #34864 adds anArrayBufferViewarm toReadableToSyncIO). Whichever lands second needs a small follow-up, so a human should be aware of the merge order. - I checked conditional-type resolution order:
BunFileextendsBlob, but the newWritableToIOtestsnumber | BunFilebefore falling through, soBunFilecorrectly maps tonumber | undefinedand plainBlob/ArrayBufferViewfall through toundefined. TheReadableStream | Request | Responsearm is placed after the fd arm and before the catch-all, which is correct. - Tests: the fixture uses
tsd.expectType(...).is<...>()for exact equality (not just assignability), pins the distributed holder-type unions, and the PR shows 25 diagnostics against the unfixed.d.ts, so the assertions are demonstrably load-bearing. The newbun-types.test.tscase mirrors the existingBun.mmapspawned-tscpattern byte-for-byte.
Problem
Bun.spawn(cmd, { stdin: Bun.file(path) }),{ stdin: new Blob(..) },{ stdin: new Uint8Array(..) },{ stdin: new Response("x") }and{ stdout: Bun.file(path) }all typeproc.stdin/proc.stdoutasnumber; at runtime (bun 1.4.0) every one of them isundefined.{ stdin: someReadableStream }(or aRequest/Responsewith a streaming body) typesproc.stdinasundefined; at runtime it is the stream.{ stdin: 0 }/{ stdout: 1 }/{ stderr: 2 }type the property asnumber; at runtime Bun turns the parent's own descriptor into"inherit"and the property isundefined.Bun.spawnSync(cmd, { stdout: fd })typesresult.stdoutasundefined; at runtime it is the fd number.packages/bun-types/bun.d.ts(Spawn.ReadableToIO,ReadableToSyncIO,WritableToIO, around line 7439) listBunFile | ArrayBufferView | Blob | Request | Response | numberas inputs that yieldnumber. The runtime only echoes a caller-supplied fd:Writable::to_js(src/runtime/api/bun/subprocess/Writable.rs:393) returns a number forWritable::FdandundefinedforBuffer/Memfd/Ignore/Inherit;Readable::to_jsandto_buffered_value(subprocess/Readable.rs:240,:270) do the same;Stdio::extract_blob(src/runtime/api/bun/spawn/stdio.rs:589) turnsBun.file(fd)intoStdio::FdandBun.file(path)intoStdio::Path; aReadableStreamstdin is cached as thestdinproperty itself (js_bun_spawn_bindings.rs:1728); fds 0/1/2 in their own slot becomeStdio::Inherit(stdio.rs:480).test/integration/bun-types/fixture/spawn.ts:162-180asserted the wrongnumberclaim forRequest,ResponseandUint8Arraystdin.Fix
ReadableToIO/ReadableToSyncIO:number | BunFilemaps tonumber | undefined; everything other than"pipe"maps toundefined.WritableToIO: same, plusReadableStream | Request | Responsemaps toReadableStream | undefined.WritableIO(the union of everythingstdincan hold) gainsReadableStreamto stay equal toWritableToIO<Writable>.number | undefinedrather thannumberfor fd inputs:Bun.file(path)andBun.file(fd)are the same static type but giveundefinedand a number, and a raw0/1/2in its own slot givesundefined;numberalone is wrong for the most commonBun.file()usage. Why| undefinedon the stream arm: aRequest/Responsewith an in-memory body, and streams Bun can read as a blob (blob.stream(),Bun.file(p).stream()), giveundefined. The holder-type unions stay the same shape except for the additions above:Subprocess["stdout"]is stillReadableStream | number | undefined,Subprocess["stdin"]becomesFileSink | ReadableStream | number | undefined,SyncSubprocess["stdout"]becomesBuffer | number | undefined; the fixture pins all three.cd test && bun run typecheck(bun's own test suite, which spawns constantly) before and after the change adds no diagnostic to any line that type-checked before; the only new ones are the three fixture lines that encoded the old claim and a second diagnostic ontest/js/node/process/process-stdout-write-after-end.test.ts:65, which already failed to type-check (.text()onnumber | ReadableStream).scripts/buildtype-checks identically before and after.Subprocess.stdin/stdout/stderrandSyncSubprocess.stdout/stderrstates the mapping (these properties had no JSDoc, and the alias JSDoc is not what the IDE shows when hoveringproc.stdin). The Reference block indocs/runtime/child-process.mdxgets the two changed unions.test/integration/bun-types/fixture/spawn.tsasserts the exact property type for fd,0/1/2,Bun.file(),Blob,ArrayBufferView,ReadableStream,Request,Responseandfetch()stdin, fd /Bun.file()/ArrayBufferViewstdout and stderr (spawnandspawnSync),readable, and the holder-type unions. Against the unfixedbun.d.tsthe fixture produces 25 diagnostics (lines 172-250); with the fixbun test test/integration/bun-types/bun-types.test.tspasses all 16 cases (no-lib.dom, lib.dom, tsgo).bun-types.test.tsare skipped on debug builds, so a small case that runstscoverfixture/spawn.tsis added sobun bd testexercises the fixture too. It is the same case bun-types: declare Subprocess.writable and lint interface Subprocess against the classes.ts table #39279 adds (byte-identical, so the two merge cleanly), and both become redundant if test(bun-types): replace the tsgo and Bun.mmap spawns with one whole-fixture tsc run, enforced by a lint #39270 (which type-checks the whole fixture on debug builds) lands first.Subprocess.writableand assertsstdin: 0/stdout: 1givenumber, which this PR changes tonumber | undefined; whichever lands second needs that one assertion updated. Bun.spawnSync: write stdout/stderr into a caller-provided Uint8Array #34864 adds anArrayBufferViewarm toReadableToSyncIOfor a new spawnSync feature and will need a trivial rebase over this.ReadableToIO<undefined>saysReadableStream, which is right forstdout(default"pipe") but not forBun.spawn'sstderr(default"inherit"); fixing that needs the mapping to know which slot it is for, and is tracked separately.Background
Bun.spawninfers three type parameters (In,Out,Err) from thestdin/stdout/stderr(orstdio) options, andSubprocess<In, Out, Err>computes the type of each stdio property from them with a conditional type:WritableToIO<In>for stdin,ReadableToIO<Out>/<Err>for stdout and stderr;SyncSubprocessusesReadableToSyncIO. With a literal option (stdin: "pipe") the property gets the exact type; with the unconstrained default (Bun.Subprocessused as a holder type) the conditional distributes over the whole option union, so the property becomes the union of every arm. That is why the PR pins the holder-type unions: they are what code that stores aSubprocesswithout knowing its configuration sees.Stdiovariant (spawn/stdio.rs) and then into aReadable/Writablevariant owned by theSubprocess; the JS getters (subprocess.rsget_stdinetc., cached on first read) convert that variant to a value.Fdis the only variant that produces a number. Inputs Bun copies into the child itself (Blob,ArrayBufferView, memfd, a path it opens) produceundefinedbecause there is nothing for the caller to hold; a"pipe"produces aFileSink(stdin) orReadableStream(stdout/stderr); aReadableStreamstdin is piped by Bun and the stream object itself is stored in thestdinproperty slot.Runtime probe (bun 1.4.0, linux x64): what each option produces
The fd numbers are unchanged after the child exits and on a first read after exit.
Diagnostics the new fixture produces against the unfixed bun.d.ts