Skip to content

fix(ext/node): start proxy loop at index 0 in Readable.prototype.wrap() - #36566

Open
badgerbees wants to merge 1 commit into
denoland:mainfrom
badgerbees:feature/stream-wrap-fix
Open

fix(ext/node): start proxy loop at index 0 in Readable.prototype.wrap()#36566
badgerbees wants to merge 1 commit into
denoland:mainfrom
badgerbees:feature/stream-wrap-fix

Conversation

@badgerbees

Copy link
Copy Markdown
Contributor

Ports the upstream Node.js PR #64048, which fixes the method-proxying loop in Readable.prototype.wrap() to start at index 0 (nodejs/node#64048).

Previously, the method-proxying loop in Readable.prototype.wrap() started at index 1, silently skipping streamKeys[0] and leaving the method at the first own-enumerable key of the wrapped stream unproxied. The bug was introduced in nodejs/node@ee9e2a2 when ArrayPrototypeForEach(ObjectKeys(stream), ...) — which iterates from index 0 — was rewritten as a manual for loop for performance. The faithful translation starts at 0; a sibling loop converted in the same commit already correctly does so. The bug stayed hidden because keys[0] is usually _events (a non-function), which the loop's typeof guard skips anyway.

This structurally aligns Deno's ext/node/polyfills/internal/streams/readable.js with Node's by changing for (let j = 1; ...) to for (let j = 0; ...) so the proxy loop iterates over every own-enumerable key of the wrapped stream.

The proxying logic is otherwise unchanged: a key is still only proxied when this[i] === undefined && typeof stream[i] === "function", so the previously-skipped first key (typically _events) continues to be safely ignored while any real first method is now correctly bound.

Enables the upstream parallel/test-stream2-readable-wrap-proxy-methods.js, which asserts that a function on the first own-enumerable key of the wrapped stream is proxied onto the Readable.

AI Disclosure:
This PR was prepared with the assistance of an AI coding tool. The underlying code port was authored by a human; the AI was used to audit and analyze the change, compare it against the upstream Node.js PR, and draft this description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant