Implement ReadableStream.from#32533
Open
robobun wants to merge 6 commits into
Open
Conversation
Add the WHATWG `ReadableStream.from(asyncIterable)` static to the constructor shared by the global `ReadableStream` and the one exported from node:stream/web. It follows the ReadableStreamFromIterable algorithm: reads Symbol.asyncIterator first and falls back to Symbol.iterator (adapting a sync iterator per CreateAsyncFromSyncIterator, awaiting its values), pulls lazily with highWaterMark 0, forwards cancel to the iterator's return method, and throws ERR_ARG_NOT_ITERABLE for non-iterable arguments, matching Node.
Collaborator
Author
Contributor
|
Found 1 issue this PR may fix:
``` 🤖 Generated with Claude Code |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughImplements ChangesReadableStream.from static method
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Collaborator
Author
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/builtins/ReadableStream.ts`:
- Line 112: The static method `from` lacks explicit `this` type annotation
required for C++-bound builtin functions. Add `this: typeof ReadableStream` as a
typed parameter to the `from` function signature to match the pattern used in
the `initialize` instance method which correctly types `this` as the
ReadableStream type. This typing is necessary to enable proper direct method
binding in C++ as per the coding guidelines for builtin functions.
- Around line 145-149: In the ReadableStream iterator adaptation logic, you must
always await iterResult.value for sync iterators on all code paths to prevent
swallowing promise rejections from the value getter. Restructure the code to
extract the await statement outside of the conditional that checks
iterResult.done, ensuring that when sync is true, iterResult.value is awaited
regardless of whether done is true or false. Additionally, apply this same await
pattern to the cancel function mentioned in the comment to ensure promise
rejections are properly handled there as well.
In `@test/js/web/streams/streams.test.js`:
- Around line 535-620: Add two new test cases to the ReadableStream.from() test
suite to cover Promise rejection scenarios in the iterator value paths. Create a
test that verifies rejected promises are properly propagated when a sync
iterator's next() method returns { done: true, value: Promise.reject(...) }, and
another test that verifies rejection handling when a sync iterator's return()
method returns { value: Promise.reject(...) }. Both tests should verify that the
rejection is caught and propagated correctly during async iteration, ensuring
the implementation properly handles the await statements in the close path for
both successful and rejected promise outcomes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 616da589-df72-497c-bc34-72930b93a97f
📒 Files selected for processing (3)
src/js/builtins/ReadableStream.tssrc/jsc/bindings/webcore/JSReadableStream.cpptest/js/web/streams/streams.test.js
The CreateAsyncFromSyncIterator adaptation awaits the value of every result, including a done result and the iterator's return() result. Await it on those paths too so a sync iterator that hands back a rejected promise surfaces the rejection (erroring the stream or rejecting cancel) instead of leaving it as an unhandled rejection.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
ReadableStreamFromIterable reads a native async iterator's value solely on the not-done path; the earlier hoist read it unconditionally, invoking the value getter of a done result. Keep the value read under the not-done branch for async iterators while still awaiting it on the done branch for sync iterators (CreateAsyncFromSyncIterator), so a rejected promise still surfaces there.
Route the iterator-protocol validation failures through $ERR_INVALID_STATE_TypeError so they carry code ERR_INVALID_STATE like Node (iterator method, iterator.next(), and iterator.return() not returning/fulfilling with an object). Guard the ERR_ARG_NOT_ITERABLE message's String(iterable) with a try/catch so a null-prototype object (or a throwing toString/Symbol.toPrimitive) still reports the code instead of a raw 'Cannot convert ... to primitive value' TypeError. Declare the from static in the no-DOM bun-types fallback so server-only projects without lib.dom get the type.
Collaborator
Author
|
This PR is ready for review: the implementation is complete, all review threads are resolved, and the tests pass locally ( CI build 63640 is red only because of a transient infrastructure failure during dependency setup: That is a GitHub server error fetching a build dependency; the Windows and aarch64 test shards aborted before running any tests, so it is unrelated to this change. The previous run was likewise red only on unrelated macOS aarch64 infra (a puppeteer browser download, an amd64/arm64 Docker image mismatch for autobahn, and an R2 network timeout). Could a maintainer re-run CI when convenient? The diff itself is green. |
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
1 participant
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.
What
Adds the WHATWG
ReadableStream.from(asyncIterable)static method, which Bun was missing on both the globalReadableStreamand the one exported fromnode:stream/web(they are the same constructor in Bun).Fixes #32529.
Fixes #3700 (original feature request).
Repro (before)
After this change it prints
a/b/c, matching Node.Cause
The
ReadableStreamconstructor (JSReadableStreamDOMConstructor) only installedlength/name/prototype; no staticfromwas ever defined. WebKit gatesfrombehind a setting Bun does not enable, and Bun's builtins never provided it.Fix
frombuiltin insrc/js/builtins/ReadableStream.tsimplementing theReadableStreamFromIterablealgorithm:Symbol.asyncIteratorfirst, falling back toSymbol.iterator(a sync iterator is adapted perCreateAsyncFromSyncIterator, so its yielded values are awaited),highWaterMark: 0) so the iterator does not start until the stream is read,cancel(reason)to the iterator'sreturnmethod,ERR_ARG_NOT_ITERABLE(aTypeError) for non-iterable arguments, and propagates the usualTypeErrorfornull/undefined.src/jsc/bindings/webcore/JSReadableStream.cppwith{ writable, enumerable, configurable },length1,name"from", matching Node and WebIDL.Because
node:stream/webre-exports the global constructor, one install covers both entry points.Verification
bun bd test test/js/web/streams/streams.test.js -t "ReadableStream.from"(17 tests): sync iterables, strings, async generators, null/undefined chunks, sync promise-value awaiting, async-over-sync precedence, laziness, cancel -> return, error propagation, and theERR_ARG_NOT_ITERABLE/TypeErrorpaths. The suite fails on the released binary and passes with this change.