Fix: Accept capture streams in RenderOptions without a type assertion - #981
Open
tomaspiaggio wants to merge 2 commits into
Open
Fix: Accept capture streams in RenderOptions without a type assertion#981tomaspiaggio wants to merge 2 commits into
RenderOptions without a type assertion#981tomaspiaggio wants to merge 2 commits into
Conversation
tomaspiaggio
force-pushed
the
fix/render-options-stream-types
branch
6 times, most recently
from
July 30, 2026 01:14
e767c28 to
329f9c3
Compare
tomaspiaggio
force-pushed
the
fix/render-options-stream-types
branch
from
July 30, 2026 02:00
329f9c3 to
ef77e09
Compare
Author
|
@sindresorhus sorry for not opening an issue before doing this. I had this patched on my project and thought would be a nice inclusion. I cleaned it up a bit but i think it's good to go. Please let me know if you need any changes. It's NOT urgent. Just types ergonomics mostly. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ink can already render somewhere other than a terminal.
debug: trueexists for it,ink-testing-libraryis built on it, and the readme documentsstdoutas astream.Writable. The types are the only thing standing in the way.The problem
RenderOptionsasks forNodeJS.WriteStreamandNodeJS.ReadStream. Those aretty.WriteStreamandtty.ReadStream, which extendnet.Socket— around 90 members each, fromcursorToandmoveCursortoremoteAddressandsetNoDelay. So the moment you want to capture output instead of printing it, you're stuck:The way around it today is to assert the type away:
That's not a small thing to ask.
as unknown asdoesn't just permit this object, it switches off type checking for that value entirely — including future mistakes that genuinely matter, like gettingwrite's shape wrong. It also can't be used at all in codebases that ban the pattern by lint rule, which puts a documented Ink feature out of reach for them.And Ink's own code has to do it.
ink-testing-librarycasts internally. So does this repository:When the companion package and the library's own tests both have to lie to the compiler to use a supported feature, the type is the thing that's wrong.
What Ink actually needs
Before designing anything I checked what Ink really touches, from the compiled output:
Ten members out of ninety, and most of them conditional:
write()is the only one always used.getWritableStreamState()does!stdout.destroyed && !stdout.writableEnded && (stdout.writable ?? true), which is good evidence the narrow surface is deliberate.on()/off()are only used when rendering interactively (ink.tsx:457) and byuseWindowSize()(use-window-size.ts:32).stdinonly whenstdin.isTTYis set (App.tsx:209). Raw mode input then needsaddListener(),read(),setRawMode(),setEncoding(),ref()andunref(), and kitty keyboard detection also useson(),removeListener()andunshift()(ink.tsx:1202-1255).writableLength, Ink passes a completion callback towrite()(ink.tsx:896) and waits for it, so such a stream must invoke it orwaitUntilExit()andwaitUntilRenderFlush()never settle.That's the contract, and it's what the new types describe — including the conditional parts, in their doc comments, so nobody has to rediscover this from a crash.
The proposal
Which makes the original snippet compile as-is:
Decisions I made, and where I'd like your opinion
Why structural types rather than just loosening to
stream.Writable. That would fixPassThrough, but not the case that motivates this: an in-memory sink is usually a plain object, not aWritablesubclass, and forcing people to extendWritablefor awrite()method is a worse deal than the assertion. Structural types accept both.Why
RenderOptionsis generic instead of just narrowing the three properties. Narrowing them directly was my first attempt, and it broke code that reads the options back out:TypeScript uses one type for reads and writes of a property, so widening what callers may pass in necessarily narrows what everyone else gets out. Generic parameters with Node's types as the defaults avoid that: bare
RenderOptionsmeans exactly what it means today,render()accepts the wide instantiation, and wrapper authors can ask forRenderOptions<InkOutputStream, InkInputStream>explicitly. The cost is two type parameters on a public type. I think that's the right trade, but it's your API — happy to switch to the plain narrowing if you'd rather have the simpler type and accept the read-side break.Why stream detection is left exactly as it is. An earlier revision of this PR also let you pass a capture stream as the positional second argument,
render(tree, sink). That required changing stream detection fromoptions instanceof Streamto a duck-typedtypeof options.write === 'function', and I convinced myself it was strictly wider. It wasn't:render(tree, null)andrender(tree, 42)started throwingTypeError: Cannot use 'in' operator…, where master falls back to the defaults and renders. Any JS caller forwarding a possibly-null options value would have hit that.writemethod but intended as options got reclassified as the output stream, silently dropping the options it carried.I don't like changing behavior people already depend on just because it's convenient, and a second way to pass a stream doesn't come close to earning it — the upside is a shorter call for a handful of users, the downside lands on people who never asked for it. So detection is byte-for-byte master's
instanceof Stream, and the positional parameter is typedWritable: the streams that check accepts and Ink can actually write to. It's deliberately a little narrower than the check — aReadableor a bareStreamsubclass is rejected at compile time even thoughinstanceof Streamwould take it — but nothing the type allows is mishandled at runtime, which is the direction that matters. Capture streams go through the options object, which is the documented form and whatink-testing-libraryuses. I verified behavior matches master fornull, numbers, strings, functions and options-shaped objects carryingwrite.Why every
InkInputStreammember is optional. My first version requiredonandread. That looked reasonable until the new test had to stub both with no-ops for methods Ink never calls on a non-TTY stdin — the split matched nothing at runtime. Ink touches stdin only whenisTTYis set, so the honest encoding is all-optional, with the input requirements documented on the type.This has a cost I want to flag rather than bury: because Node types
tty.ReadStreamandtty.WriteStreamas sockets, an all-optional input type also accepts an output stream, sorender(tree, {stdin: process.stdout})compiles now where master rejected it. It would fail at runtime if a component calleduseInput. The only member that discriminates the two issetRawMode, and requiring it would rule out the minimal stdin that motivated the change. I chose the weaker type and documented the contract, but this one is genuinely a judgement call and I'll follow your preference.What I deliberately left alone. Ink's internals still use Node's stream types.
render()casts at that single boundary, soink.tsx, the contexts, the hooks andinstances.tsare untouched, anduseStdout()/useStdin()/useStderr()keep their current return types. Migrating those is a bigger, breaking change and doesn't belong here — but say the word and I'll do it in this PR if you'd prefer they match.One intentional behavior difference. Passing a stream as an explicit
undefinedornullnow falls back to the process stream instead of overriding the default with it. On master the nullish value reached the renderer, and none of these worked:All three render here. Turning two crashes and a silent no-op into working renders seemed worth keeping, but it is still a change, so it's your call — I'll restore exact parity if you'd rather have it. Everything else is untouched, down to reading only own enumerable properties off the options object, so an inherited
stdoutgetter is ignored exactly as it is today.Compatibility
Every pattern below typechecked against both
masterand this branch:masterrender(tree)render(tree, {stdout: process.stdout, stdin: process.stdin, stderr: process.stderr})render(tree, process.stdout)render(tree, {stdout: sink as unknown as NodeJS.WriteStream})RenderOptionsvalue intorender()options.stdout!.cursorTo(0)const stream: NodeJS.WriteStream = options.stdout!render(tree, {stdout: new PassThrough()})render(tree, new PassThrough())render(tree, {stdout: captureStream, stderr: captureStream})render(tree, {stdout: captureStream, stdin: {}})RenderOptions<InkOutputStream, InkInputStream>for wrapper authorsNothing that compiles today stops compiling, and the runtime is unchanged.
One implementation detail worth flagging: the methods use method shorthand rather than arrow properties, so their parameters stay bivariant and Node's stricter signatures (
setEncoding(encoding: BufferEncoding), for one) still satisfy them. As arrow properties this would rejectprocess.stdoutunderstrictFunctionTypes.Six
options as anycasts intest/components.tsxbecome unnecessary with these types and are removed, since@typescript-eslint/no-unnecessary-type-assertionfails the lint step otherwise. That's the only reason this PR touches that file.Docs and tests
The
optionssection ofrender(tree, options?)now names the new types, says the streams don't have to be terminal streams, points at the conditionalon()/off()and raw-mode requirements, and tells wrapper authors to annotate withRenderOptions<InkOutputStream, InkInputStream>.New tests in
test/render.tsx:process.stdout/stdin/stderrstill assign toRenderOptions(compile-time).{columns, rows, write}object plus a bare{}stdin render and receive output through the options form, with no assertions anywhere in the test.debug: true— the motivating case — asserting the output arrives before unmount, which debug mode writes synchronously duringrender().writemethod still being treated as options, pinning the classification that the duck-typed revision above got wrong.PassThroughpassed positionally, covering theinstanceof Streampath. Nothing in the suite calledrender()with a positional stream before this PR.npm test -- --serial, the command CI runs, exits 0 on both Node 24 and Node 22: typecheck, lint, then 1041 passing, 4 known failures, 1 todo.