Repository navigation
Align the TS ReadableStream async iterator with WebIDL - #7534
Conversation
|
Since last review: 0 resolved, 0 still open, 1 new. Aligns TypeScript ReadableStream async-iterator behavior and expands parity coverage. Other findings
Reviewed commit: 2beeeea4 · github run |
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
Checked out and verified locally: readable-ts@, readable-cpp@ and //src/wpt:streams-ts@ pass, and I probed the three shapes directly against both implementations and Node.
The synchronous first next() is safe with respect to the ongoing promise because readableStreamDefaultReaderRead always delivers through PromisePrototypeThen, so the read request's chunk()/close()/error() clear state.current in a microtask after next() has assigned it, which is exactly WebIDL's fulfillSteps timing. Removing started is correct; it only existed to force that first hop. return() before any next() now cancels synchronously, as WebIDL's returnSteps do.
The red CI jobs are unrelated flakes (container-client@all-compat-flags snapshot 404 on linux, unsafe-module-test@all-compat-flags timeout on windows).
9f10107 to
f03c1a5
Compare
Three shape differences from the WebIDL asynchronous iterator prototype: - next() and return() threw a TypeError synchronously when called on anything but a stream's iterator. They now return a promise rejected with it. C++ still throws (readable ledger #25). - The prototype had no class string. It now has WebIDL's @@toStringTag "ReadableStream AsyncIterator" (non-writable). C++ uses "ReadableStreamAsyncIterator" (writable) (ledger #13). - The first next() was always chained behind an already-resolved promise, so it read, and pulled, one microtask late, and its result looked `then` up twice. With no ongoing promise it now runs its steps at once, as WebIDL does: a started stream pulls synchronously (parity with C++ and with reader.read()), and the result looks `then` up once. The unbound-reader check in the next steps now rejects rather than throws, since those steps can now run inside next() itself. The prototype's own @@asynciterator is kept (WebIDL inherits it from %AsyncIteratorPrototype%), so for-await over values() does not depend on that prototype. Pins: readable async-iteration.js iteratorPrototypeShape (class string), new iteratorMethodsRejectForeignThis and firstNextPullsSynchronously; then-interceptors.js thenGetterPerIteratorNext's first next() is now 1 lookup. Without the source change exactly these four fail, in the TS cells only. Compatibility: the API surface change (the new @@toStringTag) is behind the experimental typescript_implemented_streams flag.
WebIDL's return() sets the iterator's ongoing promise to the return
steps' promise, then hands the caller a separate promise chained on it
that resolves with { value, done: true }. The TS iterator returned the
ongoing promise itself, and its return steps (an async function)
resolved with the result object. The return steps now follow the
streams return algorithm: they return the cancel promise directly, or
a promise resolved with undefined, and only an already-finished
iterator's steps resolve with a result object of their own.
Observable through then interceptors only: on a finished iterator,
return() now looks `then` up twice (WebIDL); C++ and Node look it up
once (readable ledger #26, thenGetterPerIteratorReturn). On a live
iterator it is once everywhere.
Also pins, as parity, a pull() run by the first next() that itself
calls next() (nextFromPullReadsBehindOuter): the reentrant call finds
no ongoing promise, so its read queues behind the outer one, and a
later next() waits for the outer alone (outer=1, inner=2, third=3 in
both implementations). Node defers the first next() by a microtask and
yields third=2, inner=3, as the TS iterator did before the first next()
ran its steps at once.
Without the source change thenGetterPerIteratorReturn fails in the TS
cells; with the pre-#7534 readable.ts nextFromPullReadsBehindOuter
fails too.
Compatibility: behind the experimental typescript_implemented_streams
flag.
f03c1a5 to
2beeeea
Compare
Three shape differences from the WebIDL asynchronous iterator prototype:
anything but a stream's iterator. They now return a promise rejected
with it. C++ still throws (readable ledger TODO(launch) -> TODO(beta) #25).
@@toStringTag "ReadableStream AsyncIterator" (non-writable). C++ uses
"ReadableStreamAsyncIterator" (writable) (ledger Tweak the readme a bit #13).
promise, so it read, and pulled, one microtask late, and its result
looked
thenup twice. With no ongoing promise it now runs its stepsat once, as WebIDL does: a started stream pulls synchronously (parity
with C++ and with reader.read()), and the result looks
thenup once.The unbound-reader check in the next steps now rejects rather than
throws, since those steps can now run inside next() itself. The
prototype's own @@asynciterator is kept (WebIDL inherits it from
%AsyncIteratorPrototype%), so for-await over values() does not depend
on that prototype.
Pins: readable async-iteration.js iteratorPrototypeShape (class string),
new iteratorMethodsRejectForeignThis and firstNextPullsSynchronously;
then-interceptors.js thenGetterPerIteratorNext's first next() is now 1
lookup. Without the source change exactly these four fail, in the TS
cells only.
Compatibility: the API surface change (the new @@toStringTag) is behind
the experimental typescript_implemented_streams flag.