Skip to content

fix(ext/node): align node stream destroy named export#33573

Merged
bartlomieju merged 1 commit into
denoland:mainfrom
ktKongTong:fix-node-steam-named-export-destroy
May 14, 2026
Merged

fix(ext/node): align node stream destroy named export#33573
bartlomieju merged 1 commit into
denoland:mainfrom
ktKongTong:fix-node-steam-named-export-destroy

Conversation

@ktKongTong

Copy link
Copy Markdown
Contributor

align node:stream destroy named export

import { destroy } from "node:stream";

console.log(typeof destroy);

Expect Output: function

Actual Output: Uncaught SyntaxError: The requested module 'node:stream' does not provide an export named 'destroy'

@CLAassistant

CLAassistant commented Apr 27, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@fibibot fibibot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Verified against Node's lib/stream.js — Node's public API is Stream.destroy = destroyer (the implementation is named destroyer internally, but the export surface is destroy). So import { destroy } from 'node:stream' is the correct Node form, and destroyer was a Deno-specific divergence. Renaming the named export to destroy matches Node and keeps the existing Stream.destroy = destroyer assignment on line 126 consistent with the new export.

One observation inline: dropping the destroyer export name entirely is technically a breaking change for any Deno-specific code that imported destroyer from node:stream — though that name never existed in Node, so realistically very few users would have written that. Searched tests/, ext/ and found zero internal usages, so internally clean.

No CI signal yet (the PR is from an external contributor — only lint title and CLA have run; full CI needs a maintainer to authorize), but the change is so small that visual review covers it.

Comment thread ext/node/polyfills/stream.ts

@fibibot fibibot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming the named export to destroyer as destroy matches Node's lib/stream.js, which sets module.exports.destroy = internalDestroy.destroyer — same legacy-normalize helper that Stream.destroy is already wired to at line 126. CI green.

  • nit: a one-line smoke test in tests/unit_node/stream_test.ts (import { destroy } from "node:stream"; assertEquals(typeof destroy, "function")) would lock the regression.

@fibibot

fibibot commented May 13, 2026

Copy link
Copy Markdown
Contributor

@bartlomieju this is ready to merge

@lunadogbot lunadogbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

destroyer is loaded directly via core.loadExtScript in every polyfill that uses it (stream.ts:49, adapters.js:4, duplexify.js:21, compose.js:7), so renaming the node:stream export to destroy only affects userland — and matches Node's module.exports.destroy = internalDestroy.destroyer. CI is green.

@lunadogbot

Copy link
Copy Markdown
Contributor

@bartlomieju this is ready to merge

@bartlomieju
bartlomieju merged commit e5959e4 into denoland:main May 14, 2026
112 checks passed
littledivy pushed a commit to crowlKats/deno that referenced this pull request Jun 10, 2026
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.

5 participants