refactor: move ReadableStream-related functions from ZigGlobalObject.cpp to ReadableStream.cpp#23547
Conversation
…cpp to ReadableStream.cpp Moved all ReadableStream-related extern C functions and helper functions from ZigGlobalObject.cpp to ReadableStream.cpp for better code organization. This includes: - invokeReadableStreamFunction helper (now local to ReadableStream__tee) - ReadableStream__tee - ReadableStream__cancel - ReadableStream__detach - ReadableStream__isDisturbed - ReadableStream__isLocked - ReadableStreamTag__tagged - ZigGlobalObject__createNativeReadableStream - ZigGlobalObject__readableStreamToArrayBufferBody - ZigGlobalObject__readableStreamToArrayBuffer - ZigGlobalObject__readableStreamToBytes - ZigGlobalObject__readableStreamToText - ZigGlobalObject__readableStreamToFormData - ZigGlobalObject__readableStreamToJSON - ZigGlobalObject__readableStreamToBlob - functionReadableStreamToArrayBuffer - functionReadableStreamToBytes Added necessary includes and namespace declarations to ensure proper compilation.
|
Updated 3:52 PM PT - Oct 12th, 2025
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 23547That installs a local version of the PR into your bun-23547 --bun |
The code generator function declarations are not needed in the header file since they're only used within ReadableStream.cpp.
WalkthroughMoved ReadableStream implementations: removed ReadableStream host/extern declarations from Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/bun.js/bindings/ZigGlobalObject.cpp(0 hunks)src/bun.js/bindings/webcore/ReadableStream.cpp(2 hunks)src/bun.js/bindings/webcore/ReadableStream.h(1 hunks)
💤 Files with no reviewable changes (1)
- src/bun.js/bindings/ZigGlobalObject.cpp
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/webcore/ReadableStream.hsrc/bun.js/bindings/webcore/ReadableStream.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/webcore/ReadableStream.cpp
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: When a JS class has a public constructor, implement Foo, FooPrototype, and FooConstructor (JSDestructibleObject, JSNonFinalObject, InternalFunction)
Define class properties using HashTableValue arrays in C++ bindings
Add iso subspaces for classes with C++ fields
Cache structures in ZigGlobalObject for C++-backed classes
Files:
src/bun.js/bindings/webcore/ReadableStream.cpp
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Cache structures in ZigGlobalObject for C++-backed classes
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.{h,cpp} : If only a class (no public constructor/prototype), use JSC::LazyProperty<JSGlobalObject, Structure>, initialize it in finishCreation, and visit it in visitChildren
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Cache structures in ZigGlobalObject for C++-backed classes
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.h : When there is a class, prototype, and constructor, add a JSC::LazyClassStructure field for the class to ZigGlobalObject.h
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
🧬 Code graph analysis (1)
src/bun.js/bindings/webcore/ReadableStream.cpp (3)
src/bun.js/bindings/webcore/ReadableStream.h (5)
WebCore(34-115)isLocked(55-66)JSValue(90-93)JSValue(95-98)JSValue(100-103)src/bun.js/bindings/BunClientData.h (1)
clientData(200-203)src/js/builtins/ReadableStream.ts (4)
readableStreamToText(122-138)readableStreamToFormData(300-309)readableStreamToJSON(312-331)readableStreamToBlob(334-342)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (11)
src/bun.js/bindings/webcore/ReadableStream.cpp (11)
267-319: ReadableStream__tee: approach looks good; error propagation preserved.Lambda isolates throw scope, reports errors under ASSERT, and converts to two JSValues. LGTM.
470-487: createNativeReadableStream: overall fine.Pattern matches other helpers (lookup builtin, call via CallData, assert exception invariant). LGTM.
489-529: readableStreamToArrayBufferBody: solid caching + type checks.Function caches generator, validates Promise, and returns encoded promise with throw scope. LGTM.
530-536: ZigGlobalObject__readableStreamToArrayBuffer wrapper: LGTM.Thin wrapper over Body function; consistent with others.
537-576: readableStreamToBytes: mirrors ArrayBuffer path correctly.Caching and Promise validation are consistent. LGTM.
600-622: readableStreamToFormData: caching + args pass-through look good.LGTM.
623-644: readableStreamToJSON: caching and dispatch look good.LGTM.
645-666: readableStreamToBlob: consistent with others.LGTM.
667-683: functionReadableStreamToArrayBuffer: argument validation present.LGTM.
684-699: functionReadableStreamToBytes: argument validation present.LGTM.
321-336: Verify cancel parity for unlocked streams
The early-return in ReadableStream__cancel prevents cancel on unlocked streams; spec expects cancel to always produce a promise. Confirm behavior and update if needed.
| extern "C" void ReadableStream__detach(JSC::EncodedJSValue possibleReadableStream, Zig::GlobalObject* globalObject) | ||
| { | ||
| using namespace JSC; | ||
| using namespace WebCore; | ||
|
|
||
| auto value = JSC::JSValue::decode(possibleReadableStream); | ||
| if (value.isEmpty() || !value.isCell()) | ||
| return; | ||
|
|
||
| auto* readableStream = static_cast<WebCore::JSReadableStream*>(value.asCell()); | ||
| if (!readableStream) [[unlikely]] | ||
| return; | ||
| readableStream->setNativePtr(globalObject->vm(), jsNumber(-1)); | ||
| readableStream->setNativeType(0); | ||
| readableStream->setDisturbed(true); | ||
| } |
There was a problem hiding this comment.
ReadableStream__detach updates JS objects without JSLock.
Acquire JSLock before mutating JSReadableStream’s state.
Apply:
extern "C" void ReadableStream__detach(JSC::EncodedJSValue possibleReadableStream, Zig::GlobalObject* globalObject)
{
using namespace JSC;
using namespace WebCore;
- auto value = JSC::JSValue::decode(possibleReadableStream);
+ auto& vm = JSC::getVM(globalObject);
+ JSC::JSLockHolder lock(vm);
+ auto value = JSC::JSValue::decode(possibleReadableStream);
if (value.isEmpty() || !value.isCell())
return;
- auto* readableStream = static_cast<WebCore::JSReadableStream*>(value.asCell());
+ auto* readableStream = jsDynamicCast<WebCore::JSReadableStream*>(value.asCell());
if (!readableStream) [[unlikely]]
return;
- readableStream->setNativePtr(globalObject->vm(), jsNumber(-1));
+ readableStream->setNativePtr(vm, jsNumber(-1));
readableStream->setNativeType(0);
readableStream->setDisturbed(true);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| extern "C" void ReadableStream__detach(JSC::EncodedJSValue possibleReadableStream, Zig::GlobalObject* globalObject) | |
| { | |
| using namespace JSC; | |
| using namespace WebCore; | |
| auto value = JSC::JSValue::decode(possibleReadableStream); | |
| if (value.isEmpty() || !value.isCell()) | |
| return; | |
| auto* readableStream = static_cast<WebCore::JSReadableStream*>(value.asCell()); | |
| if (!readableStream) [[unlikely]] | |
| return; | |
| readableStream->setNativePtr(globalObject->vm(), jsNumber(-1)); | |
| readableStream->setNativeType(0); | |
| readableStream->setDisturbed(true); | |
| } | |
| extern "C" void ReadableStream__detach(JSC::EncodedJSValue possibleReadableStream, Zig::GlobalObject* globalObject) | |
| { | |
| using namespace JSC; | |
| using namespace WebCore; | |
| auto& vm = JSC::getVM(globalObject); | |
| JSC::JSLockHolder lock(vm); | |
| auto value = JSC::JSValue::decode(possibleReadableStream); | |
| if (value.isEmpty() || !value.isCell()) | |
| return; | |
| auto* readableStream = jsDynamicCast<WebCore::JSReadableStream*>(value.asCell()); | |
| if (!readableStream) [[unlikely]] | |
| return; | |
| readableStream->setNativePtr(vm, jsNumber(-1)); | |
| readableStream->setNativeType(0); | |
| readableStream->setDisturbed(true); | |
| } |
🤖 Prompt for AI Agents
In src/bun.js/bindings/webcore/ReadableStream.cpp around lines 338 to 353, the
function ReadableStream__detach mutates JSReadableStream state without holding
the JSLock; acquire a JSC::JSLock (scoped) using the VM from globalObject
(globalObject->vm()) before calling setNativePtr, setNativeType, and
setDisturbed, then release the lock after those mutations so the JS object is
modified under the required lock.
| auto iterable = object->getIfPropertyExists(globalObject, vm.propertyNames->asyncIteratorSymbol); | ||
| RETURN_IF_EXCEPTION(throwScope, {}); | ||
| if (iterable && iterable.isCallable()) { | ||
| fn = iterable; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Incorrect RETURN_IF_EXCEPTION return value for int32_t function.
Using {} returns 0, but this function uses -1 as the error sentinel elsewhere. Return -1 on exception for consistency.
Apply:
- RETURN_IF_EXCEPTION(throwScope, {});
+ RETURN_IF_EXCEPTION(throwScope, -1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto iterable = object->getIfPropertyExists(globalObject, vm.propertyNames->asyncIteratorSymbol); | |
| RETURN_IF_EXCEPTION(throwScope, {}); | |
| if (iterable && iterable.isCallable()) { | |
| fn = iterable; | |
| } | |
| } | |
| auto iterable = object->getIfPropertyExists(globalObject, vm.propertyNames->asyncIteratorSymbol); | |
| RETURN_IF_EXCEPTION(throwScope, -1); | |
| if (iterable && iterable.isCallable()) { | |
| fn = iterable; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/bun.js/bindings/webcore/ReadableStream.cpp around lines 399 to 405, the
RETURN_IF_EXCEPTION call currently returns {} which evaluates to 0, but this
function uses -1 as the error sentinel elsewhere; change the
RETURN_IF_EXCEPTION(throwScope, {}) invocation to
RETURN_IF_EXCEPTION(throwScope, -1) so exceptions propagate the correct -1 error
value and keep behavior consistent with other return sites.
| auto* function = globalObject->m_readableStreamToArrayBuffer.get(); | ||
| if (!function) { | ||
| function = JSFunction::create(vm, globalObject, static_cast<JSC::FunctionExecutable*>(readableStreamReadableStreamToArrayBufferCodeGenerator(vm)), globalObject); | ||
| globalObject->m_readableStreamToArrayBuffer.set(vm, globalObject, function); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Confirm Zig::GlobalObject has WriteBarriers for cached functions and visits them.
Ensure m_readableStreamTo{ArrayBuffer,Bytes,Text,FormData,JSON,Blob} are fields on Zig::GlobalObject, initialized to null, and visited in visitChildren to avoid GC issues.
Run:
Based on learnings
Also applies to: 546-549, 586-591, 610-613, 633-636, 655-658
🏁 Script executed:
#!/bin/bash
# Confirm fields exist
rg -nP 'm_readableStreamTo(ArrayBuffer|Bytes|Text|FormData|JSON|Blob)\b'
# Confirm visitChildren visits them
rg -n -C3 --type=cpp 'visitChildren\(' | rg -n -C2 'm_readableStreamTo(ArrayBuffer|Bytes|Text|FormData|JSON|Blob)'Length of output: 2483
🏁 Script executed:
#!/bin/bash
rg -n -C10 'visitChildren' src/bun.js/bindings/ZigGlobalObject.cppLength of output: 2062
🏁 Script executed:
#!/bin/bash
rg -n -C5 'm_readableStreamToArrayBuffer' src/bun.js/bindings/ZigGlobalObject.cpp
rg -n 'visitGlobalObjectMember' src/bun.js/bindings/ZigGlobalObject.cppLength of output: 544
Ensure m_readableStreamTo WriteBarriers are visited in visitChildrenImpl*
In src/bun.js/bindings/ZigGlobalObject.cpp’s GlobalObject::visitChildrenImpl, add
visitGlobalObjectMember(visitor, thisObject->m_readableStreamToArrayBuffer);
visitGlobalObjectMember(visitor, thisObject->m_readableStreamToBytes);
visitGlobalObjectMember(visitor, thisObject->m_readableStreamToText);
visitGlobalObjectMember(visitor, thisObject->m_readableStreamToFormData);
visitGlobalObjectMember(visitor, thisObject->m_readableStreamToJSON);
visitGlobalObjectMember(visitor, thisObject->m_readableStreamToBlob);after the existing visitGlobalObjectMember calls to prevent these cached JSFunction pointers from being reclaimed by the GC.
🤖 Prompt for AI Agents
In src/bun.js/bindings/webcore/ReadableStream.cpp around lines 497-501, the code
caches JSFunction pointers on the global object (m_readableStreamToArrayBuffer
and related functions) but the garbage collector visitChildrenImpl in
src/bun.js/bindings/ZigGlobalObject.cpp does not currently visit those
write-barrier members; update GlobalObject::visitChildrenImpl to call
visitGlobalObjectMember for m_readableStreamToArrayBuffer,
m_readableStreamToBytes, m_readableStreamToText, m_readableStreamToFormData,
m_readableStreamToJSON, and m_readableStreamToBlob after the existing
visitGlobalObjectMember calls so the GC marks these cached JSFunction pointers
and prevents them from being reclaimed.
| extern "C" JSC::EncodedJSValue ZigGlobalObject__readableStreamToText(Zig::GlobalObject* globalObject, JSC::EncodedJSValue readableStreamValue) | ||
| { | ||
| using namespace JSC; | ||
| using namespace WebCore; | ||
| auto& vm = JSC::getVM(globalObject); | ||
|
|
||
| JSC::JSFunction* function = nullptr; | ||
| if (auto readableStreamToText = globalObject->m_readableStreamToText.get()) { | ||
| function = readableStreamToText; | ||
| } else { | ||
| function = JSFunction::create(vm, globalObject, static_cast<JSC::FunctionExecutable*>(readableStreamReadableStreamToTextCodeGenerator(vm)), globalObject); | ||
|
|
||
| globalObject->m_readableStreamToText.set(vm, globalObject, function); | ||
| } | ||
|
|
||
| JSC::MarkedArgumentBuffer arguments = JSC::MarkedArgumentBuffer(); | ||
| arguments.append(JSValue::decode(readableStreamValue)); | ||
|
|
||
| auto callData = JSC::getCallData(function); | ||
| return JSC::JSValue::encode(call(globalObject, function, callData, JSC::jsUndefined(), arguments)); | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Align readableStreamToText with others (validate Promise).
Unlike Bytes/ArrayBuffer, this path doesn’t assert the result is a Promise. Add the same checks for type safety.
Apply:
extern "C" JSC::EncodedJSValue ZigGlobalObject__readableStreamToText(Zig::GlobalObject* globalObject, JSC::EncodedJSValue readableStreamValue)
{
using namespace JSC;
using namespace WebCore;
auto& vm = JSC::getVM(globalObject);
- JSC::JSFunction* function = nullptr;
+ auto throwScope = DECLARE_THROW_SCOPE(vm);
+ JSC::JSFunction* function = nullptr;
@@
- auto callData = JSC::getCallData(function);
- return JSC::JSValue::encode(call(globalObject, function, callData, JSC::jsUndefined(), arguments));
+ auto callData = JSC::getCallData(function);
+ JSValue result = call(globalObject, function, callData, JSC::jsUndefined(), arguments);
+ if (!result || result.isUndefinedOrNull()) [[unlikely]]
+ RELEASE_AND_RETURN(throwScope, JSValue::encode(result));
+ JSObject* object = result.getObject();
+ if (!object) {
+ throwTypeError(globalObject, throwScope, "Expected object"_s);
+ return {};
+ }
+ auto* promise = jsDynamicCast<JSPromise*>(object);
+ if (!promise) {
+ throwTypeError(globalObject, throwScope, "Expected promise"_s);
+ return {};
+ }
+ RELEASE_AND_RETURN(throwScope, JSValue::encode(promise));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| extern "C" JSC::EncodedJSValue ZigGlobalObject__readableStreamToText(Zig::GlobalObject* globalObject, JSC::EncodedJSValue readableStreamValue) | |
| { | |
| using namespace JSC; | |
| using namespace WebCore; | |
| auto& vm = JSC::getVM(globalObject); | |
| JSC::JSFunction* function = nullptr; | |
| if (auto readableStreamToText = globalObject->m_readableStreamToText.get()) { | |
| function = readableStreamToText; | |
| } else { | |
| function = JSFunction::create(vm, globalObject, static_cast<JSC::FunctionExecutable*>(readableStreamReadableStreamToTextCodeGenerator(vm)), globalObject); | |
| globalObject->m_readableStreamToText.set(vm, globalObject, function); | |
| } | |
| JSC::MarkedArgumentBuffer arguments = JSC::MarkedArgumentBuffer(); | |
| arguments.append(JSValue::decode(readableStreamValue)); | |
| auto callData = JSC::getCallData(function); | |
| return JSC::JSValue::encode(call(globalObject, function, callData, JSC::jsUndefined(), arguments)); | |
| } | |
| extern "C" JSC::EncodedJSValue ZigGlobalObject__readableStreamToText(Zig::GlobalObject* globalObject, JSC::EncodedJSValue readableStreamValue) | |
| { | |
| using namespace JSC; | |
| using namespace WebCore; | |
| auto& vm = JSC::getVM(globalObject); | |
| auto throwScope = DECLARE_THROW_SCOPE(vm); | |
| JSC::JSFunction* function = nullptr; | |
| if (auto readableStreamToText = globalObject->m_readableStreamToText.get()) { | |
| function = readableStreamToText; | |
| } else { | |
| function = JSFunction::create(vm, globalObject, static_cast<JSC::FunctionExecutable*>(readableStreamReadableStreamToTextCodeGenerator(vm)), globalObject); | |
| globalObject->m_readableStreamToText.set(vm, globalObject, function); | |
| } | |
| JSC::MarkedArgumentBuffer arguments = JSC::MarkedArgumentBuffer(); | |
| arguments.append(JSValue::decode(readableStreamValue)); | |
| auto callData = JSC::getCallData(function); | |
| JSValue result = call(globalObject, function, callData, JSC::jsUndefined(), arguments); | |
| if (!result || result.isUndefinedOrNull()) [[unlikely]] | |
| RELEASE_AND_RETURN(throwScope, JSValue::encode(result)); | |
| JSObject* object = result.getObject(); | |
| if (!object) { | |
| throwTypeError(globalObject, throwScope, "Expected object"_s); | |
| return {}; | |
| } | |
| auto* promise = jsDynamicCast<JSPromise*>(object); | |
| if (!promise) { | |
| throwTypeError(globalObject, throwScope, "Expected promise"_s); | |
| return {}; | |
| } | |
| RELEASE_AND_RETURN(throwScope, JSValue::encode(promise)); | |
| } |
🤖 Prompt for AI Agents
In src/bun.js/bindings/webcore/ReadableStream.cpp around lines 578-599, the
readableStreamToText trampoline invokes the JS function but does not validate
that the result is a Promise like the Bytes/ArrayBuffer paths do; add the same
runtime/type checks after the call: verify the returned JSValue is an object and
specifically a Promise (use the project's existing helpers used by other
bindings to test Promise-ness), and if it's not, raise/return a clear error (or
assert) consistent with the other bindings; update control flow to handle the
non-Promise case identically to the other implementations.
| // Forward declarations for code generators | ||
| JSC::FunctionExecutable* readableStreamReadableStreamToArrayBufferCodeGenerator(JSC::VM&); | ||
| JSC::FunctionExecutable* readableStreamReadableStreamToBytesCodeGenerator(JSC::VM&); | ||
| JSC::FunctionExecutable* readableStreamReadableStreamToTextCodeGenerator(JSC::VM&); | ||
| JSC::FunctionExecutable* readableStreamReadableStreamToFormDataCodeGenerator(JSC::VM&); | ||
| JSC::FunctionExecutable* readableStreamReadableStreamToJSONCodeGenerator(JSC::VM&); | ||
| JSC::FunctionExecutable* readableStreamReadableStreamToBlobCodeGenerator(JSC::VM&); | ||
|
|
There was a problem hiding this comment.
Missing declaration for JSC::FunctionExecutable in header (compile-time error).
The header uses JSC::FunctionExecutable* without declaring the type or including its header. Add a forward declaration (or include the proper JSC header).
Apply this diff:
#include "JSReadableStream.h"
+namespace JSC {
+class FunctionExecutable;
+}
+
// Forward declarations for code generators
JSC::FunctionExecutable* readableStreamReadableStreamToArrayBufferCodeGenerator(JSC::VM&);
JSC::FunctionExecutable* readableStreamReadableStreamToBytesCodeGenerator(JSC::VM&);
JSC::FunctionExecutable* readableStreamReadableStreamToTextCodeGenerator(JSC::VM&);
JSC::FunctionExecutable* readableStreamReadableStreamToFormDataCodeGenerator(JSC::VM&);
JSC::FunctionExecutable* readableStreamReadableStreamToJSONCodeGenerator(JSC::VM&);
JSC::FunctionExecutable* readableStreamReadableStreamToBlobCodeGenerator(JSC::VM&);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Forward declarations for code generators | |
| JSC::FunctionExecutable* readableStreamReadableStreamToArrayBufferCodeGenerator(JSC::VM&); | |
| JSC::FunctionExecutable* readableStreamReadableStreamToBytesCodeGenerator(JSC::VM&); | |
| JSC::FunctionExecutable* readableStreamReadableStreamToTextCodeGenerator(JSC::VM&); | |
| JSC::FunctionExecutable* readableStreamReadableStreamToFormDataCodeGenerator(JSC::VM&); | |
| JSC::FunctionExecutable* readableStreamReadableStreamToJSONCodeGenerator(JSC::VM&); | |
| JSC::FunctionExecutable* readableStreamReadableStreamToBlobCodeGenerator(JSC::VM&); | |
| #include "JSReadableStream.h" | |
| namespace JSC { | |
| class FunctionExecutable; | |
| } | |
| // Forward declarations for code generators | |
| JSC::FunctionExecutable* readableStreamReadableStreamToArrayBufferCodeGenerator(JSC::VM&); | |
| JSC::FunctionExecutable* readableStreamReadableStreamToBytesCodeGenerator(JSC::VM&); | |
| JSC::FunctionExecutable* readableStreamReadableStreamToTextCodeGenerator(JSC::VM&); | |
| JSC::FunctionExecutable* readableStreamReadableStreamToFormDataCodeGenerator(JSC::VM&); | |
| JSC::FunctionExecutable* readableStreamReadableStreamToJSONCodeGenerator(JSC::VM&); | |
| JSC::FunctionExecutable* readableStreamReadableStreamToBlobCodeGenerator(JSC::VM&); |
🤖 Prompt for AI Agents
In src/bun.js/bindings/webcore/ReadableStream.h around lines 107 to 114, the
header declares pointers to JSC::FunctionExecutable but doesn't declare that
type; add a forward declaration for JSC::FunctionExecutable (for example, add
"namespace JSC { class FunctionExecutable; }" above these function declarations)
or alternatively include the appropriate JSC header that defines
FunctionExecutable so the compiler knows the type before these prototypes.
WalkthroughReadableStream interop functions and helpers were removed from src/bun.js/bindings/ZigGlobalObject.cpp and reintroduced under src/bun.js/bindings/webcore/ReadableStream.cpp as extern "C" bindings and host functions. The change relocates tee/cancel/detach/state queries, tagging, native creation, and multiple conversion utilities (ArrayBuffer/Bytes/Text/FormData/JSON/Blob). Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/bindings/webcore/ReadableStream.cpp (1)
26-28: Include root.h first to satisfy lintsAdd root.h at the top of binding .cpp files.
As per coding guidelines
+#include "root.h" #include "config.h" #include "ReadableStream.h"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/bun.js/bindings/ZigGlobalObject.cpp(0 hunks)src/bun.js/bindings/webcore/ReadableStream.cpp(2 hunks)
💤 Files with no reviewable changes (1)
- src/bun.js/bindings/ZigGlobalObject.cpp
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/webcore/ReadableStream.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/webcore/ReadableStream.cpp
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: When a JS class has a public constructor, implement Foo, FooPrototype, and FooConstructor (JSDestructibleObject, JSNonFinalObject, InternalFunction)
Define class properties using HashTableValue arrays in C++ bindings
Add iso subspaces for classes with C++ fields
Cache structures in ZigGlobalObject for C++-backed classes
Files:
src/bun.js/bindings/webcore/ReadableStream.cpp
🧠 Learnings (8)
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.{h,cpp} : If only a class (no public constructor/prototype), use JSC::LazyProperty<JSGlobalObject, Structure>, initialize it in finishCreation, and visit it in visitChildren
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Cache structures in ZigGlobalObject for C++-backed classes
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.h : When there is a class, prototype, and constructor, add a JSC::LazyClassStructure field for the class to ZigGlobalObject.h
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
🧬 Code graph analysis (1)
src/bun.js/bindings/webcore/ReadableStream.cpp (3)
src/bun.js/bindings/webcore/ReadableStream.h (5)
WebCore(34-107)isLocked(55-66)JSValue(90-93)JSValue(95-98)JSValue(100-103)src/bun.js/bindings/BunClientData.h (1)
clientData(200-203)src/js/builtins/ReadableStream.ts (4)
readableStreamToText(122-138)readableStreamToFormData(300-309)readableStreamToJSON(312-331)readableStreamToBlob(334-342)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (5)
src/bun.js/bindings/webcore/ReadableStream.cpp (5)
267-319: LGTM: extern "C" ReadableStream__tee moved and scoped safelyArgument checks, builtin lookup, call, and conversion look correct; out-params filled only on success.
321-337: LGTM: ReadableStream__cancel parity retainedType check, locked check, and cancel via WebCore helper look consistent with prior behavior.
470-488: LGTM: createNativeReadableStream wrapperBuiltin lookup + call + exception assertions look correct.
489-576: LGTM: readableStreamTo{ArrayBuffer,Bytes,Text,FormData,JSON,Blob} wrappersLazy init and caching of builtins, argument marshalling, and promise/type checks are consistent.
Also applies to: 578-666
667-699: LGTM: host functions forward to cached helpersArgument guard then delegate; matches surrounding patterns.
| auto iterable = object->getIfPropertyExists(globalObject, vm.propertyNames->asyncIteratorSymbol); | ||
| RETURN_IF_EXCEPTION(throwScope, {}); | ||
| if (iterable && iterable.isCallable()) { |
There was a problem hiding this comment.
Return -1 on exception in ReadableStreamTag__tagged
Returning {} here yields 0 (success code). Use -1 for error like other paths.
- RETURN_IF_EXCEPTION(throwScope, {});
+ RETURN_IF_EXCEPTION(throwScope, -1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto iterable = object->getIfPropertyExists(globalObject, vm.propertyNames->asyncIteratorSymbol); | |
| RETURN_IF_EXCEPTION(throwScope, {}); | |
| if (iterable && iterable.isCallable()) { | |
| auto iterable = object->getIfPropertyExists(globalObject, vm.propertyNames->asyncIteratorSymbol); | |
| RETURN_IF_EXCEPTION(throwScope, -1); | |
| if (iterable && iterable.isCallable()) { |
🤖 Prompt for AI Agents
In src/bun.js/bindings/webcore/ReadableStream.cpp around lines 399-401, the
RETURN_IF_EXCEPTION call currently uses {} which ends up returning 0 (success);
change the exceptional return to -1 to indicate an error like the other error
paths — i.e., replace the {} in RETURN_IF_EXCEPTION(throwScope, {}) with -1 so
the ReadableStreamTag__tagged function returns -1 on exception.
Following Bun's convention of including root.h as the first include.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
src/bun.js/bindings/webcore/ReadableStream.cpp (6)
26-27: Past review comment addressed: root.h included.The file now correctly includes "root.h" as the first header, satisfying the coding guidelines for C++ binding files.
340-355: Fix unsafe cast and acquire JSLock before mutating JS objects.Two critical issues remain from previous reviews:
- Line 349 uses unsafe
static_castinstead ofjsDynamicCast, risking memory corruption- No
JSLockis acquired before mutating JSReadableStream state (lines 352-354), risking race conditionsApply this diff to fix both issues:
extern "C" void ReadableStream__detach(JSC::EncodedJSValue possibleReadableStream, Zig::GlobalObject* globalObject) { using namespace JSC; using namespace WebCore; + auto& vm = JSC::getVM(globalObject); + JSC::JSLockHolder lock(vm); + auto value = JSC::JSValue::decode(possibleReadableStream); if (value.isEmpty() || !value.isCell()) return; - auto* readableStream = static_cast<WebCore::JSReadableStream*>(value.asCell()); + auto* readableStream = jsDynamicCast<WebCore::JSReadableStream*>(value); if (!readableStream) [[unlikely]] return; - readableStream->setNativePtr(globalObject->vm(), jsNumber(-1)); + readableStream->setNativePtr(vm, jsNumber(-1)); readableStream->setNativeType(0); readableStream->setDisturbed(true); }
357-358: Remove redundant forward declaration.The forward declaration on line 357 is immediately followed by the function definition. Remove the duplicate to reduce noise.
Apply this diff:
-extern "C" bool ReadableStream__isDisturbed(JSC::EncodedJSValue possibleReadableStream, Zig::GlobalObject* globalObject); extern "C" bool ReadableStream__isDisturbed(JSC::EncodedJSValue possibleReadableStream, Zig::GlobalObject* globalObject)
367-368: Remove redundant forward declaration.The forward declaration on line 367 is immediately followed by the function definition. Remove the duplicate to reduce noise.
Apply this diff:
-extern "C" bool ReadableStream__isLocked(JSC::EncodedJSValue possibleReadableStream, Zig::GlobalObject* globalObject); extern "C" bool ReadableStream__isLocked(JSC::EncodedJSValue possibleReadableStream, Zig::GlobalObject* globalObject)
401-402: Return -1 on exception for consistency with error sentinel.Line 402 returns
{}which evaluates to 0 (success code), but this function uses -1 as the error sentinel throughout (lines 387, 410, 415, 427, 432). This inconsistency could cause incorrect error handling.Apply this diff:
- RETURN_IF_EXCEPTION(throwScope, {}); + RETURN_IF_EXCEPTION(throwScope, -1);
580-600: Validate Promise result for consistency with other converters.Unlike
readableStreamToArrayBufferandreadableStreamToBytes, this function doesn't validate that the result is a Promise. While the TypeScript code ensures it returns a Promise, adding the same validation would improve type safety and consistency across all converter functions.Apply this diff to add Promise validation:
extern "C" JSC::EncodedJSValue ZigGlobalObject__readableStreamToText(Zig::GlobalObject* globalObject, JSC::EncodedJSValue readableStreamValue) { using namespace JSC; using namespace WebCore; auto& vm = JSC::getVM(globalObject); + auto throwScope = DECLARE_THROW_SCOPE(vm); + JSC::JSFunction* function = nullptr; if (auto readableStreamToText = globalObject->m_readableStreamToText.get()) { function = readableStreamToText; } else { function = JSFunction::create(vm, globalObject, static_cast<JSC::FunctionExecutable*>(readableStreamReadableStreamToTextCodeGenerator(vm)), globalObject); globalObject->m_readableStreamToText.set(vm, globalObject, function); } JSC::MarkedArgumentBuffer arguments = JSC::MarkedArgumentBuffer(); arguments.append(JSValue::decode(readableStreamValue)); auto callData = JSC::getCallData(function); - return JSC::JSValue::encode(call(globalObject, function, callData, JSC::jsUndefined(), arguments)); + JSValue result = call(globalObject, function, callData, JSC::jsUndefined(), arguments); + + if (!result || result.isUndefinedOrNull()) [[unlikely]] + RELEASE_AND_RETURN(throwScope, JSValue::encode(result)); + + JSObject* object = result.getObject(); + if (!object) { + throwTypeError(globalObject, throwScope, "Expected object"_s); + return {}; + } + + auto* promise = jsDynamicCast<JSPromise*>(object); + if (!promise) { + throwTypeError(globalObject, throwScope, "Expected promise"_s); + return {}; + } + + RELEASE_AND_RETURN(throwScope, JSValue::encode(promise)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/bun.js/bindings/webcore/ReadableStream.cpp(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/webcore/ReadableStream.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/webcore/ReadableStream.cpp
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: When a JS class has a public constructor, implement Foo, FooPrototype, and FooConstructor (JSDestructibleObject, JSNonFinalObject, InternalFunction)
Define class properties using HashTableValue arrays in C++ bindings
Add iso subspaces for classes with C++ fields
Cache structures in ZigGlobalObject for C++-backed classes
Files:
src/bun.js/bindings/webcore/ReadableStream.cpp
🧠 Learnings (11)
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.cpp : Include "root.h" at the top of C++ binding files to satisfy lints
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/**/*.{h,cpp} : Use JSC::WriteBarrier for heap-allocated references and implement visitChildren() for custom heap objects
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.{h,cpp} : If only a class (no public constructor/prototype), use JSC::LazyProperty<JSGlobalObject, Structure>, initialize it in finishCreation, and visit it in visitChildren
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.cpp : Initialize the LazyClassStructure in GlobalObject::finishCreation and visit it in GlobalObject::visitChildrenImpl
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Cache structures in ZigGlobalObject for C++-backed classes
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.h : When there is a class, prototype, and constructor, add a JSC::LazyClassStructure field for the class to ZigGlobalObject.h
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
🧬 Code graph analysis (1)
src/bun.js/bindings/webcore/ReadableStream.cpp (3)
src/bun.js/bindings/webcore/ReadableStream.h (5)
WebCore(34-107)isLocked(55-66)JSValue(90-93)JSValue(95-98)JSValue(100-103)src/bun.js/bindings/BunClientData.h (1)
clientData(200-203)src/js/builtins/ReadableStream.ts (4)
readableStreamToText(122-138)readableStreamToFormData(300-309)readableStreamToJSON(312-331)readableStreamToBlob(334-342)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (9)
src/bun.js/bindings/webcore/ReadableStream.cpp (9)
37-48: LGTM!The new includes are appropriate for the moved ReadableStream functions and provide the necessary types and utilities.
269-321: LGTM!The
ReadableStream__teeimplementation is correct:
- Properly uses
jsDynamicCastfor type safety- Lambda acquires
JSLockbefore mutating JS objects- Correctly uses
Bun::IDLRawAnyfor IDL conversion- Exception handling returns appropriate bool values
323-338: LGTM!The
ReadableStream__cancelimplementation correctly:
- Uses
jsDynamicCastfor safe type checking- Uses namespace-qualified
Bun::AbortError- Guards against canceling unlocked streams
472-489: LGTM!The
ZigGlobalObject__createNativeReadableStreamimplementation correctly uses builtin functions and proper exception handling viaEXCEPTION_ASSERT.
532-537: LGTM!Simple wrapper function that correctly delegates to
ZigGlobalObject__readableStreamToArrayBufferBody.
539-578: LGTM!The
ZigGlobalObject__readableStreamToBytesimplementation correctly mirrors the ArrayBuffer pattern with proper Promise validation and caching.
669-684: LGTM!The host function correctly validates argument count and delegates to the implementation function.
686-701: LGTM!The host function correctly validates argument count and delegates to the implementation function, mirroring the pattern used in
functionReadableStreamToArrayBuffer.
491-530: LGTM – WriteBarriers form_readableStreamToArrayBuffer(and related) are included via FOR_EACH_GLOBALOBJECT_GC_MEMBER and visited inGlobalObject::visitChildrenImpl.
| extern "C" JSC::EncodedJSValue ZigGlobalObject__readableStreamToFormData(Zig::GlobalObject* globalObject, JSC::EncodedJSValue readableStreamValue, JSC::EncodedJSValue contentTypeValue) | ||
| { | ||
| using namespace JSC; | ||
| using namespace WebCore; | ||
| auto& vm = JSC::getVM(globalObject); | ||
|
|
||
| JSC::JSFunction* function = nullptr; | ||
| if (auto readableStreamToFormData = globalObject->m_readableStreamToFormData.get()) { | ||
| function = readableStreamToFormData; | ||
| } else { | ||
| function = JSFunction::create(vm, globalObject, static_cast<JSC::FunctionExecutable*>(readableStreamReadableStreamToFormDataCodeGenerator(vm)), globalObject); | ||
|
|
||
| globalObject->m_readableStreamToFormData.set(vm, globalObject, function); | ||
| } | ||
|
|
||
| JSC::MarkedArgumentBuffer arguments = JSC::MarkedArgumentBuffer(); | ||
| arguments.append(JSValue::decode(readableStreamValue)); | ||
| arguments.append(JSValue::decode(contentTypeValue)); | ||
|
|
||
| auto callData = JSC::getCallData(function); | ||
| return JSC::JSValue::encode(call(globalObject, function, callData, JSC::jsUndefined(), arguments)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Validate Promise result for consistency with ArrayBuffer/Bytes.
Like readableStreamToText, this function doesn't validate that the result is a Promise. Consider adding the same validation as readableStreamToArrayBuffer and readableStreamToBytes for consistency and type safety.
| extern "C" JSC::EncodedJSValue ZigGlobalObject__readableStreamToJSON(Zig::GlobalObject* globalObject, JSC::EncodedJSValue readableStreamValue) | ||
| { | ||
| using namespace JSC; | ||
| using namespace WebCore; | ||
| auto& vm = JSC::getVM(globalObject); | ||
|
|
||
| JSC::JSFunction* function = nullptr; | ||
| if (auto readableStreamToJSON = globalObject->m_readableStreamToJSON.get()) { | ||
| function = readableStreamToJSON; | ||
| } else { | ||
| function = JSFunction::create(vm, globalObject, static_cast<JSC::FunctionExecutable*>(readableStreamReadableStreamToJSONCodeGenerator(vm)), globalObject); | ||
|
|
||
| globalObject->m_readableStreamToJSON.set(vm, globalObject, function); | ||
| } | ||
|
|
||
| JSC::MarkedArgumentBuffer arguments = JSC::MarkedArgumentBuffer(); | ||
| arguments.append(JSValue::decode(readableStreamValue)); | ||
|
|
||
| auto callData = JSC::getCallData(function); | ||
| return JSC::JSValue::encode(call(globalObject, function, callData, JSC::jsUndefined(), arguments)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Validate Promise result for consistency with ArrayBuffer/Bytes.
Like readableStreamToText and readableStreamToFormData, this function doesn't validate that the result is a Promise. Consider adding the same validation as readableStreamToArrayBuffer and readableStreamToBytes for consistency and type safety.
| extern "C" JSC::EncodedJSValue ZigGlobalObject__readableStreamToBlob(Zig::GlobalObject* globalObject, JSC::EncodedJSValue readableStreamValue) | ||
| { | ||
| using namespace JSC; | ||
| using namespace WebCore; | ||
| auto& vm = JSC::getVM(globalObject); | ||
|
|
||
| JSC::JSFunction* function = nullptr; | ||
| if (auto readableStreamToBlob = globalObject->m_readableStreamToBlob.get()) { | ||
| function = readableStreamToBlob; | ||
| } else { | ||
| function = JSFunction::create(vm, globalObject, static_cast<JSC::FunctionExecutable*>(readableStreamReadableStreamToBlobCodeGenerator(vm)), globalObject); | ||
|
|
||
| globalObject->m_readableStreamToBlob.set(vm, globalObject, function); | ||
| } | ||
|
|
||
| JSC::MarkedArgumentBuffer arguments = JSC::MarkedArgumentBuffer(); | ||
| arguments.append(JSValue::decode(readableStreamValue)); | ||
|
|
||
| auto callData = JSC::getCallData(function); | ||
| return JSC::JSValue::encode(call(globalObject, function, callData, JSC::jsUndefined(), arguments)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Validate Promise result for consistency with ArrayBuffer/Bytes.
Like the other converter functions, this doesn't validate that the result is a Promise. Consider adding the same validation as readableStreamToArrayBuffer and readableStreamToBytes for consistency and type safety.
…ant forward declarations
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/bun.js/bindings/webcore/ReadableStream.cpp (5)
337-349: CRITICAL: Fix unsafe cast and add JSLock before mutating JS object state.This function still has two critical issues flagged in previous reviews:
- Unsafe cast at line 343: Using
static_castfromJSCell*toJSReadableStream*can cause memory corruption. Must usejsDynamicCast.- Missing JSLock: Mutating JSReadableStream state (lines 346-348) without holding JSLock can cause race conditions.
Apply this diff to fix both issues:
extern "C" void ReadableStream__detach(JSC::EncodedJSValue possibleReadableStream, Zig::GlobalObject* globalObject) { + using namespace JSC; + using namespace WebCore; + + auto& vm = JSC::getVM(globalObject); + JSC::JSLockHolder lock(vm); + auto value = JSC::JSValue::decode(possibleReadableStream); if (value.isEmpty() || !value.isCell()) return; - auto* readableStream = static_cast<WebCore::JSReadableStream*>(value.asCell()); + auto* readableStream = jsDynamicCast<WebCore::JSReadableStream*>(value); if (!readableStream) [[unlikely]] return; - readableStream->setNativePtr(globalObject->vm(), jsNumber(-1)); + readableStream->setNativePtr(vm, jsNumber(-1)); readableStream->setNativeType(0); readableStream->setDisturbed(true); }
384-386: Fix inconsistent error return value.Line 385 returns
{}(which evaluates to 0) on exception, but this function uses -1 as the error sentinel elsewhere (lines 370, 393, 398, 410, 415). This inconsistency can cause incorrect error handling.Apply this diff:
- RETURN_IF_EXCEPTION(throwScope, {}); + RETURN_IF_EXCEPTION(throwScope, -1);
555-573: Missing Promise validation for consistency with ArrayBuffer/Bytes.Unlike
readableStreamToArrayBufferandreadableStreamToBytes, this function doesn't validate that the result is a Promise (line 572). Consider adding the same validation for consistency and type safety.Apply this diff to add validation:
extern "C" JSC::EncodedJSValue ZigGlobalObject__readableStreamToText(Zig::GlobalObject* globalObject, JSC::EncodedJSValue readableStreamValue) { auto& vm = JSC::getVM(globalObject); + auto throwScope = DECLARE_THROW_SCOPE(vm); JSC::JSFunction* function = nullptr; if (auto readableStreamToText = globalObject->m_readableStreamToText.get()) { function = readableStreamToText; } else { function = JSFunction::create(vm, globalObject, static_cast<JSC::FunctionExecutable*>(readableStreamReadableStreamToTextCodeGenerator(vm)), globalObject); globalObject->m_readableStreamToText.set(vm, globalObject, function); } JSC::MarkedArgumentBuffer arguments = JSC::MarkedArgumentBuffer(); arguments.append(JSValue::decode(readableStreamValue)); auto callData = JSC::getCallData(function); - return JSC::JSValue::encode(call(globalObject, function, callData, JSC::jsUndefined(), arguments)); + JSValue result = call(globalObject, function, callData, JSC::jsUndefined(), arguments); + + if (!result || result.isUndefinedOrNull()) [[unlikely]] + RELEASE_AND_RETURN(throwScope, JSValue::encode(result)); + + JSObject* object = result.getObject(); + if (!object) [[unlikely]] { + throwTypeError(globalObject, throwScope, "Expected object"_s); + return {}; + } + + auto* promise = jsDynamicCast<JSPromise*>(object); + if (!promise) [[unlikely]] { + throwTypeError(globalObject, throwScope, "Expected promise"_s); + return {}; + } + + RELEASE_AND_RETURN(throwScope, JSValue::encode(promise)); }
575-614: Missing Promise validation for consistency with ArrayBuffer/Bytes.Both
readableStreamToFormData(lines 575-594) andreadableStreamToJSON(lines 596-614) don't validate that the result is a Promise, unlikereadableStreamToArrayBufferandreadableStreamToBytes. Consider adding the same validation for consistency and type safety.
616-634: Missing Promise validation for consistency with ArrayBuffer/Bytes.Like the other converter functions,
readableStreamToBlobdoesn't validate that the result is a Promise. Consider adding the same validation asreadableStreamToArrayBufferandreadableStreamToBytesfor consistency and type safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/bun.js/bindings/webcore/ReadableStream.cpp(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/webcore/ReadableStream.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/webcore/ReadableStream.cpp
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: When a JS class has a public constructor, implement Foo, FooPrototype, and FooConstructor (JSDestructibleObject, JSNonFinalObject, InternalFunction)
Define class properties using HashTableValue arrays in C++ bindings
Add iso subspaces for classes with C++ fields
Cache structures in ZigGlobalObject for C++-backed classes
Files:
src/bun.js/bindings/webcore/ReadableStream.cpp
🧠 Learnings (11)
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.cpp : Include "root.h" at the top of C++ binding files to satisfy lints
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/**/*.{h,cpp} : Use JSC::WriteBarrier for heap-allocated references and implement visitChildren() for custom heap objects
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.{h,cpp} : If only a class (no public constructor/prototype), use JSC::LazyProperty<JSGlobalObject, Structure>, initialize it in finishCreation, and visit it in visitChildren
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.cpp : Initialize the LazyClassStructure in GlobalObject::finishCreation and visit it in GlobalObject::visitChildrenImpl
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Cache structures in ZigGlobalObject for C++-backed classes
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.h : When there is a class, prototype, and constructor, add a JSC::LazyClassStructure field for the class to ZigGlobalObject.h
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/bun.js/bindings/webcore/ReadableStream.cpp
🧬 Code graph analysis (1)
src/bun.js/bindings/webcore/ReadableStream.cpp (3)
src/bun.js/bindings/webcore/ReadableStream.h (5)
WebCore(34-107)isLocked(55-66)JSValue(90-93)JSValue(95-98)JSValue(100-103)src/bun.js/bindings/BunClientData.h (1)
clientData(200-203)src/js/builtins/ReadableStream.ts (4)
readableStreamToText(122-138)readableStreamToFormData(300-309)readableStreamToJSON(312-331)readableStreamToBlob(334-342)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (4)
src/bun.js/bindings/webcore/ReadableStream.cpp (4)
351-362: LGTM!Both
ReadableStream__isDisturbedandReadableStream__isLockedcorrectly usejsDynamicCastfor type safety and properly delegate to the WebCore ReadableStream methods.
455-470: LGTM!The function correctly creates a native ReadableStream using the builtin function with proper exception handling.
511-553: LGTM!Both
ZigGlobalObject__readableStreamToArrayBufferandZigGlobalObject__readableStreamToBytescorrectly implement the conversion pattern with proper Promise validation.
636-662: LGTM!Both host function wrappers (
functionReadableStreamToArrayBufferandfunctionReadableStreamToBytes) correctly validate argument count and delegate to their respective body functions.
| auto invokeReadableStreamFunction = [](JSC::JSGlobalObject* lexicalGlobalObject, const JSC::Identifier& identifier, JSC::JSValue thisValue, const JSC::MarkedArgumentBuffer& arguments) -> std::optional<JSC::JSValue> { | ||
| JSC::VM& vm = lexicalGlobalObject->vm(); | ||
| auto scope = DECLARE_THROW_SCOPE(vm); | ||
| JSC::JSLockHolder lock(vm); | ||
|
|
||
| auto function = lexicalGlobalObject->get(lexicalGlobalObject, identifier); | ||
| scope.assertNoExceptionExceptTermination(); | ||
| if (scope.exception()) [[unlikely]] | ||
| return {}; | ||
| ASSERT(function.isCallable()); | ||
|
|
||
| auto callData = JSC::getCallData(function); | ||
| auto result = JSC::call(lexicalGlobalObject, function, callData, thisValue, arguments); | ||
| #if ASSERT_ENABLED | ||
| if (scope.exception()) [[unlikely]] { | ||
| Bun__reportError(lexicalGlobalObject, JSC::JSValue::encode(scope.exception())); | ||
| } | ||
| #endif | ||
| EXCEPTION_ASSERT(!scope.exception() || vm.hasPendingTerminationException()); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| return result; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting the lambda to file scope as a helper function.
The invokeReadableStreamFunction lambda is defined inline but only used once. While functional, extracting it to a file-scoped helper would improve readability and allow reuse if needed in the future. However, this is an optional improvement and not a blocker.
| if (!WebCore::ReadableStream::isLocked(globalObject, readableStream)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the cancellation logic is correct.
The code returns early if the stream is NOT locked (line 329), then proceeds to cancel only if the stream IS locked (line 334). This logic seems inverted from typical ReadableStream semantics, where you can only cancel an unlocked stream. Please verify this is intentional and correct.
🏁 Script executed:
#!/bin/bash
# Description: Search for other usages of ReadableStream::cancel to verify the locking semantics.
# Search for calls to ReadableStream::cancel and check if they verify lock state
rg -nP -C5 'ReadableStream::cancel\('Length of output: 3166
🏁 Script executed:
#!/bin/bash
# Inspect isLocked occurrences
rg -nP -C3 'isLocked' src/bun.js/bindings/webcore/ReadableStream.cpp
# Inspect static cancel implementation and instance cancel
rg -nP -C5 'void ReadableStream::cancel\(' src/bun.js/bindings/webcore/ReadableStream.cppLength of output: 2639
🏁 Script executed:
#!/bin/bash
rg -nP 'extern "C".*ReadableStream__cancel' -C3 src/bun.js/bindings/webcore/ReadableStream.cppLength of output: 319
Remove unintended isLocked guard in ReadableStream__cancel. The check at lines 329–331 prevents cancelling unlocked streams; per Streams spec, cancel must be invoked unconditionally. Delete those three lines so ReadableStream::cancel always runs.
🤖 Prompt for AI Agents
In src/bun.js/bindings/webcore/ReadableStream.cpp around lines 329–331, remove
the unintended isLocked guard that prevents calling ReadableStream::cancel on
unlocked streams; delete the three lines checking
WebCore::ReadableStream::isLocked(globalObject, readableStream) and its early
return so ReadableStream::cancel is invoked unconditionally, then rebuild/run
stream tests to verify behavior.
2 participants
Summary
This PR moves all ReadableStream-related functions from
ZigGlobalObject.cpptoReadableStream.cppfor better code organization and maintainability.Changes
Moved 17 functions from
ZigGlobalObject.cpptoReadableStream.cpp:Core ReadableStream Functions
ReadableStream__tee- withinvokeReadableStreamFunctionhelper (converted to lambda)ReadableStream__cancelReadableStream__detachReadableStream__isDisturbedReadableStream__isLockedReadableStreamTag__taggedStream Creation & Conversion
ZigGlobalObject__createNativeReadableStreamZigGlobalObject__readableStreamToArrayBufferBodyZigGlobalObject__readableStreamToArrayBufferZigGlobalObject__readableStreamToBytesZigGlobalObject__readableStreamToTextZigGlobalObject__readableStreamToFormDataZigGlobalObject__readableStreamToJSONZigGlobalObject__readableStreamToBlobHost Functions
functionReadableStreamToArrayBufferfunctionReadableStreamToBytesTechnical Details
File Changes
Implementation Notes
using namespace JSC; using namespace WebCore;) to all extern "C" functionsBun::IDLRawAny,Bun::AbortError)WebCoreJSBuiltins.h,ZigGlobalObject.h,ZigGeneratedClasses.h,helpers.h,BunClientData.h,BunIDLConvert.hTesting
Benefits
ZigGlobalObject.cppis now ~400 lines smaller🤖 Generated with Claude Code