Repository navigation
Pin how stream constructors convert their arguments - #7727
Merged
Merged
Conversation
Contributor
|
Since last review: 1 resolved, 0 still open, 0 new. Not re-run: api-compat, docs, compat-flags, design-simplicity (no author changes in their files since the last review) Reviewed commit: 85902abb · github run |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
guybedford
approved these changes
Oct 13, 2026
guybedford
left a comment
guybedford
left a comment
Contributor
There was a problem hiding this comment.
LGTM. Ran the readable, readable-byte, writable, transform, strategies, piping and identity suites across the -ts, -cpp and -cpp-pedantic cells locally; all pass.
One non-blocking follow-up at the edge of this change: the source/sink dictionaries themselves are still read out of WebIDL order. writable.ts reads underlyingSink.type and throws the RangeError before abort/close/start/write are read and type-checked, where WebIDL converts the whole UnderlyingSink dictionary (lexicographic, each callback checked) and only then checks type. So new WritableStream({ type: 'x', write: 1 }) is a TypeError per spec but a RangeError here. readable.ts has the same shape with type read before autoAllocateChunkSize/cancel/pull/start. Pre-existing, so fine to leave for a later layer.
jasnell
added this pull request to stack #7710
October 13, 2026 23:39
jasnell
force-pushed
the
jasnell/ts-streams-duplicate-consolidaton-5
branch
2 times, most recently
from
October 14, 2026 16:24
334f47b to
c331c99
Compare
Base automatically changed from
jasnell/ts-streams-duplicate-consolidaton-4
to
main
October 14, 2026 20:04
jasnell
force-pushed
the
jasnell/ts-streams-duplicate-consolidaton-5
branch
from
October 14, 2026 20:27
c331c99 to
31951b3
Compare
The ReadableStream, WritableStream and TransformStream constructors
convert their arguments differently in each implementation, and
little of it was pinned. Pin it on both, ahead of moving the
TypeScript constructors to WebIDL argument conversion:
- a first argument that is null, another primitive or a function;
- a strategy argument that is null, another primitive or a
function;
- the order in which the arguments and the strategy members are
read, including when highWaterMark is converted to a number;
- which error wins when two are possible, including the cases where
TypeScript already follows the spec and must keep doing so.
This finds a C++ divergence: a TransformStream readable strategy
without a highWaterMark (null or {}) gets the ReadableStream default
of 1, because C++ applies the TransformStream default of 0 only when
the argument is omitted. TypeScript applies 0, as the spec does.
The TypeScript ReadableStream, WritableStream and TransformStream constructors each converted their arguments in their own way. ReadableStream accepted a primitive source; WritableStream and TransformStream rejected a function as a sink or transformer; ReadableStream and WritableStream threw on a null strategy, while all three accepted a primitive one; and a strategy's size was read before its highWaterMark. They now convert as WebIDL does, which matches C++ wherever C++ and the spec agree: - the source, sink or transformer must be an object, a function included; null and every other primitive throw TypeError; - each strategy converts as a dictionary: null means no strategy, every other primitive throws TypeError, and highWaterMark (with its ToNumber) is read before size; - the strategies are converted before the first argument's members are read, TransformStream's included; - the highWaterMark RangeError comes after the type checks, as the spec's ExtractHighWaterMark does. Two changes move TypeScript away from C++. new TransformStream(null) throws, as new ReadableStream(null) and new WritableStream(null) already did, where C++ accepts all three. TransformStream reads its strategies before its transformer, where C++ reads the transformer first. The conversion lives in common.ts: requireObject, convertQueuingStrategy, and the spec's ExtractHighWaterMark and ExtractSizeAlgorithm, shared by the three constructors and by IdentityTransformStream and FixedLengthStream, whose behavior is unchanged. The highWaterMark range check, repeated four times, is now done in one place.
The options of getReader(), a BYOB reader's read(), values() and the pipe methods are WebIDL dictionaries: undefined and null mean the defaults, a function's members are read, and any other primitive throws TypeError. The TypeScript implementation rejected a function at all four, and null at read() and values(). It also rejected a function as the init of CountQueuingStrategy and ByteLengthQueuingStrategy. C++ accepts all of these. A new toDictionary() in common.ts converts the four options arguments, and toStrategyDictionary() is now built on it. The strategy constructors use requireObject() instead: their init has a required member, so undefined and null still throw. Error messages are unchanged. New parity tests pin each site with null, a function and the other primitives, in both implementations. Only the TypeScript implementation changes, and it is still behind the experimental typescript_implemented_streams flag.
The header said an invalid chunk errors an IdentityTransformStream under TypeScript. In both implementations it rejects only its own write and the stream stays usable, as the file's tests assert and the suite's divergence ledger already records.
jasnell
force-pushed
the
jasnell/ts-streams-duplicate-consolidaton-5
branch
from
October 14, 2026 20:48
31951b3 to
85902ab
Compare
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
2 participants
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.
Use WebIDL compatible argument coercion