-
-
Notifications
You must be signed in to change notification settings - Fork 161
fix(runtime): dispatch node:stream super() through any bound-export heritage shape #10649
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
proggeramlug
wants to merge
2
commits into
main
Choose a base branch
from
wip/10448-stream-subclass-heritage
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+295
−0
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| ### Fixed | ||
|
|
||
| - **`node:stream` subclass overrides (`_transform`/`_write`/`_read`) are no | ||
| longer ignored when the heritage reaching `class X extends <base>` is a | ||
| local alias, an indirect subclass, a class expression, or a CJS | ||
| destructured `require('stream')` — the shape nodemailer uses in every | ||
| stream class it defines. `write()`/`push()` used to throw | ||
| `ERR_METHOD_NOT_IMPLEMENTED` because the override was never installed on | ||
| `this`; the dynamic `super()` dispatch now recognizes the resolved | ||
| bound-export value regardless of how the heritage expression reached it. | ||
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| 'use strict'; | ||
| // CommonJS half of test_gap_10448_stream_subclass_heritage.ts: the exact | ||
| // shape nodemailer uses everywhere (`const { Transform } = | ||
| // require('stream'); class X extends Transform`), plus the sibling | ||
| // Writable/Readable/Duplex destructured shapes the issue lists as broken | ||
| // the same way, and a namespace-member export for comparison. | ||
| // | ||
| // State is captured via public class fields, not a constructor-body | ||
| // assignment after `super(...args)` with a rest-param spread — that shape | ||
| // (`constructor(...args) { super(...args); ... }`) hits a separate, | ||
| // pre-existing gap (native stream methods go missing) independent of | ||
| // heritage shape or this issue; not exercised here to keep this test | ||
| // isolated to #10448's own defect. | ||
| const { Transform, Writable, Readable, Duplex } = require('stream'); | ||
| const stream = require('stream'); | ||
|
|
||
| class CjsTransform extends Transform { | ||
| _transform(chunk, _enc, cb) { | ||
| cb(null, String(chunk).toUpperCase()); | ||
| } | ||
| } | ||
|
|
||
| // `require('stream').Transform` reached via a namespace member on a plain | ||
| // `require()` result (not destructured) — control: this shape is already | ||
| // recognized statically (`is_genuine_node_stream_parent`). | ||
| class CjsViaMember extends stream.Transform { | ||
| _transform(chunk, _enc, cb) { | ||
| cb(null, String(chunk).toUpperCase()); | ||
| } | ||
| } | ||
|
|
||
| class CjsWritable extends Writable { | ||
| captured = ''; | ||
| _write(chunk, _enc, cb) { | ||
| this.captured += String(chunk).toUpperCase(); | ||
| cb(); | ||
| } | ||
| } | ||
|
|
||
| class CjsReadable extends Readable { | ||
| _done = false; | ||
| _read() { | ||
| if (this._done) return; | ||
| this._done = true; | ||
| this.push('x'); | ||
| this.push('y'); | ||
| this.push(null); | ||
| } | ||
| } | ||
|
|
||
| // Write-half only (no `_read`/push): proves the destructured `Duplex` | ||
| // heritage installs `_write` the same way `Writable` does, without | ||
| // depending on read/write event-ordering across engines. | ||
| class CjsDuplex extends Duplex { | ||
| captured = ''; | ||
| _write(chunk, _enc, cb) { | ||
| this.captured += String(chunk).toUpperCase(); | ||
| cb(); | ||
| } | ||
| } | ||
|
|
||
| module.exports = { | ||
| CjsTransform, | ||
| CjsViaMember, | ||
| CjsWritable, | ||
| CjsReadable, | ||
| CjsDuplex, | ||
| }; |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,167 @@ | ||
| // #10448: node:stream subclass overrides (`_transform`/`_write`/`_read`) | ||
| // were ignored whenever the heritage reaching `class X extends <base>` was | ||
| // anything OTHER than a shape `is_genuine_node_stream_parent` recognizes | ||
| // statically at HIR-lowering time | ||
| // (`crates/perry-hir/src/lower_decl/class_decl.rs`) — a local alias | ||
| // (`const Alias = Transform`), an indirect subclass, a class expression, or | ||
| // a CJS destructured `require('stream')` (the shape nodemailer uses in | ||
| // every stream class it defines). `write()`/`push()` then threw | ||
| // `ERR_METHOD_NOT_IMPLEMENTED` because the override was never installed on | ||
| // `this`. | ||
| // | ||
| // (`PassThrough` is a separate, deeper gap — HIR never recognizes it | ||
| // statically even via a bare import, unlike Readable/Writable/Duplex/ | ||
| // Transform, so it needs its own follow-up; not covered by this test, see | ||
| // the PR body.) | ||
| // | ||
| // Each check awaits its own stream before starting the next so output order | ||
| // is deterministic regardless of engine event-loop/nextTick scheduling | ||
| // differences — only the per-check content is the thing under test. State | ||
| // is captured via public class fields (not a `constructor(...args) { | ||
| // super(...args); ... }` rest-spread pattern, which hits an unrelated | ||
| // pre-existing gap independent of heritage shape). | ||
| import { Transform, Writable, Readable, Duplex } from "stream"; | ||
| import * as streamNs from "stream"; | ||
| import { | ||
| CjsTransform, | ||
| CjsViaMember, | ||
| CjsWritable, | ||
| CjsReadable, | ||
| CjsDuplex, | ||
| } from "./gap_10448_stream_subclass_heritage_helper.cjs"; | ||
|
|
||
| const AliasTransform = Transform; | ||
|
|
||
| class ViaImport extends Transform { | ||
| _transform(chunk: any, _enc: string, cb: any) { | ||
| cb(null, String(chunk).toUpperCase()); | ||
| } | ||
| } | ||
| class ViaAlias extends AliasTransform { | ||
| _transform(chunk: any, _enc: string, cb: any) { | ||
| cb(null, String(chunk).toUpperCase()); | ||
| } | ||
| } | ||
| class ViaNamespaceMember extends streamNs.Transform { | ||
| _transform(chunk: any, _enc: string, cb: any) { | ||
| cb(null, String(chunk).toUpperCase()); | ||
| } | ||
| } | ||
| class Mid extends AliasTransform {} | ||
| class ViaIndirect extends Mid { | ||
| _transform(chunk: any, _enc: string, cb: any) { | ||
| cb(null, String(chunk).toUpperCase()); | ||
| } | ||
| } | ||
| const ViaClassExpr = class extends AliasTransform { | ||
| _transform(chunk: any, _enc: string, cb: any) { | ||
| cb(null, String(chunk).toUpperCase()); | ||
| } | ||
| }; | ||
|
|
||
| function runTransform(name: string, T: any): Promise<void> { | ||
| return new Promise((resolve) => { | ||
| const t = new T(); | ||
| let out = ""; | ||
| t.on("data", (c: any) => (out += c)); | ||
| t.on("end", () => { | ||
| console.log(name, JSON.stringify(out)); | ||
| resolve(); | ||
| }); | ||
| try { | ||
| t.write("ab"); | ||
| t.end("c"); | ||
| } catch (e: any) { | ||
| console.log(name, "threw", e.code); | ||
| resolve(); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| function runWritable(name: string, W: any): Promise<void> { | ||
| return new Promise((resolve) => { | ||
| let w: any; | ||
| try { | ||
| w = new W(); | ||
| } catch (e: any) { | ||
| console.log(name, "threw (construct)", e.message); | ||
| resolve(); | ||
| return; | ||
| } | ||
| w.on("finish", () => { | ||
| console.log(name, JSON.stringify(w.captured)); | ||
| resolve(); | ||
| }); | ||
| try { | ||
| w.write("ab"); | ||
| w.end("c"); | ||
| } catch (e: any) { | ||
| console.log(name, "threw", e.code); | ||
| resolve(); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| function runReadable(name: string, R: any): Promise<void> { | ||
| return new Promise((resolve) => { | ||
| let r: any; | ||
| try { | ||
| r = new R(); | ||
| } catch (e: any) { | ||
| console.log(name, "threw (construct)", e.message); | ||
| resolve(); | ||
| return; | ||
| } | ||
| let out = ""; | ||
| r.on("data", (c: any) => (out += c)); | ||
| r.on("end", () => { | ||
| console.log(name, JSON.stringify(out)); | ||
| resolve(); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| class WViaWritable extends Writable { | ||
| captured = ""; | ||
| _write(chunk: any, _enc: string, cb: any) { | ||
| this.captured += String(chunk).toUpperCase(); | ||
| cb(); | ||
| } | ||
| } | ||
|
|
||
| class RViaReadable extends Readable { | ||
| private _done = false; | ||
| _read() { | ||
| if (this._done) return; | ||
| this._done = true; | ||
| this.push("m"); | ||
| this.push("n"); | ||
| this.push(null); | ||
| } | ||
| } | ||
|
|
||
| class DViaDuplex extends Duplex { | ||
| captured = ""; | ||
| _write(chunk: any, _enc: string, cb: any) { | ||
| this.captured += String(chunk).toUpperCase(); | ||
| cb(); | ||
| } | ||
| } | ||
|
|
||
| async function main() { | ||
| await runTransform("Transform via import ", ViaImport); | ||
| await runTransform("Transform via alias ", ViaAlias); | ||
| await runTransform("Transform via namespace member", ViaNamespaceMember); | ||
| await runTransform("Transform via indirect subclas", ViaIndirect); | ||
| await runTransform("Transform via class expression", ViaClassExpr); | ||
| await runTransform("Transform CJS destructured ", CjsTransform); | ||
| await runTransform("Transform CJS namespace member", CjsViaMember); | ||
| await runWritable("Writable CJS destructured ", CjsWritable); | ||
| await runWritable("Duplex CJS destructured ", CjsDuplex); | ||
| await runReadable("Readable CJS destructured ", CjsReadable); | ||
| await runWritable("Writable via import ", WViaWritable); | ||
| await runReadable("Readable via import ", RViaReadable); | ||
| await runWritable("Duplex via import ", DViaDuplex); | ||
| } | ||
|
|
||
| main(); |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 17554
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 4639
Limit the changelog claim to supported constructors.
The dispatch handles
Readable,Writable,Duplex, andTransformonly.PassThroughremains unsupported because HIR does not recognize it as anode:streamparent. The broadnode:streamwording can imply support that this change does not provide. Name the supported constructors or state thePassThroughlimitation.🤖 Prompt for AI Agents