stream: expose stream symbols by ronag · Pull Request #45671 · nodejs/node

ronag · 2022-11-29T10:33:01Z

This is required for streams interop with e.g. readable-stream. Currently readable-stream helpers will not work with normal node streams which is confusing and bad for the ecosystem.

nodejs-github-bot · 2022-11-29T10:33:06Z

Review requested:

@nodejs/streams

aduh95

If we expose them, we would need to document them. I don't think we want to do that, making symbols part of the public API goes against the idea of using symbols. Can't readable-stream use Reflect.ownKeys or Object.getOwnPropertySymbols to achieve that?

ronag · 2022-11-29T11:30:13Z

How would it do that? I'm not sure how that would work. Especially in a performant way.

ErickWendel · 2022-11-29T13:41:25Z

How would it do that?

const kKey = Symbol('key')
class My { [kKey]() { console.log('hello') }}

const [, symKey] = Reflect.ownKeys(My.prototype) 
new My()[symKey]()
// hello

ronag · 2022-11-29T15:05:58Z

How would it do that?

const kKey = Symbol('key')
class My { [kKey]() { console.log('hello') }}

const [, symKey] = Reflect.ownKeys(My.prototype) 
new My()[symKey]()
// hello

The symbols are not an a prototype...

ErickWendel · 2022-11-29T19:15:04Z

How would it do that?

const kKey = Symbol('key')
class My { [kKey]() { console.log('hello') }}

const [, symKey] = Reflect.ownKeys(My.prototype) 
new My()[symKey]()
// hello

The symbols are not an a prototype...

They also work in objects, don't they?

const obj = { [Symbol("test")]: 'hey' }
const [symbol] = Reflect.ownKeys(obj)
console.log(obj[symbol]) // hey

For Streams:

const str = stream.Readable.from('abc')
const data = Reflect.ownKeys(str)
const kCapture = data[6]
str[kCapture] // false

ronag · 2022-11-29T19:41:02Z

I still don't see how that will work. Would need to know the static index of the symbol. Which is not possible atm.

vweevers · 2022-11-29T20:01:43Z

This case is similar to Symbol.for('nodejs.util.inspect.custom') in its goal to improve portability. How about using global symbols, e.g. Symbol.for('nodejs.stream.destroyed')? The intent would be clearer, compared to exporting symbols.

RaisinTen · 2022-11-30T05:21:32Z

Why don't we use a regular string key to expose these values like what was done in the case of readable.closed and document that instead? That way, we won't be exposing raw non-global symbols, which aren't meant to be a part of the public API and there would also be no need to use Reflect.ownKeys or Object.getOwnPropertySymbols.

nodejs-github-bot · 2022-11-30T11:02:36Z

CI: https://ci.nodejs.org/job/node-test-pull-request/48247/

ronag · 2022-12-19T09:48:21Z

@nodejs/streams some reviews?

ronag · 2022-12-19T09:49:04Z

@mcollina We will need this in readable-stream for proper interop.

aduh95 · 2022-12-19T09:51:51Z

Adding the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. so we can discuss how to expose internals and what should be the adequate documentation / semver expectations around it.

nodejs-github-bot · 2023-03-16T19:19:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/50404/

nodejs-github-bot · 2023-03-18T10:59:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/50439/

nodejs-github-bot · 2023-03-18T12:52:18Z

CI: https://ci.nodejs.org/job/node-test-pull-request/50444/

nodejs-github-bot · 2023-03-24T07:20:20Z

Landed in 8c60add

anonrig · 2023-03-24T14:02:22Z

Note: I believe this change requires an update to the documentation as well.

ronag · 2023-03-24T15:08:18Z

Note: I believe this change requires an update to the documentation as well.

This is not intended as a public api.

This is required for streams interop with e.g. readable-stream. Currently readable-stream helpers will not work with normal node streams which is confusing and bad for the ecosystem. PR-URL: #45671 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>

`stream.isDestroyed()` has been exported since v19.9.0 but was never documented, while its siblings `isErrored()`, `isReadable()` and `isWritable()` all have entries in `doc/api/stream.md`. Unlike those, `isDestroyed()` rejects Web streams: it returns `null` for anything that is not a Node.js stream. The accepted types are therefore documented as `Readable|Writable|Duplex` rather than also listing `ReadableStream`/`WritableStream`. Refs: nodejs#45671 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: YspritanHyzygy <yspritan@gmail.com>

`stream.isDestroyed()` has been exported since v19.9.0 but was never documented, while its siblings `isErrored()`, `isReadable()` and `isWritable()` all have entries in `doc/api/stream.md`. Unlike those, `isDestroyed()` rejects Web streams: it returns `null` for anything that is not a Node.js stream. The accepted types are therefore documented as `Readable|Writable|Duplex` rather than also listing `ReadableStream`/`WritableStream`. Refs: #45671 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: YspritanHyzygy <yspritan@gmail.com> PR-URL: #64789 Reviewed-By: Aviv Keller <me@aviv.sh> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

ronag requested a review from mcollina November 29, 2022 10:33

ronag added the stream Issues and PRs related to the stream subsystem. label Nov 29, 2022

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Nov 29, 2022

aduh95 reviewed Nov 29, 2022

View reviewed changes

ronag force-pushed the expose-stream-symbols branch from 3d26201 to 9b479d6 Compare November 29, 2022 12:18

ronag requested a review from aduh95 November 30, 2022 10:50

ronag added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Nov 30, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 30, 2022

aduh95 added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Dec 19, 2022

This comment was marked as outdated.

Sign in to view

This was referenced Mar 17, 2023

CI Reliability 2023-03-17 nodejs/reliability#526

Open

CI Reliability 2023-03-18 nodejs/reliability#527

Open

This was referenced Mar 19, 2023

CI Reliability 2023-03-19 nodejs/reliability#528

Open

CI Reliability 2023-03-20 nodejs/reliability#530

Open

CI Reliability 2023-03-21 nodejs/reliability#531

Open

CI Reliability 2023-03-22 nodejs/reliability#532

Open

debadree25 added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 24, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 24, 2023

nodejs-github-bot merged commit 8c60add into nodejs:main Mar 24, 2023

RafaelGSS mentioned this pull request Apr 3, 2023

2023-04-18, Version 20.0.0 (Current) #47381

Merged

RafaelGSS mentioned this pull request Apr 6, 2023

v19.9.0 proposal #47441

Merged

danielleadams mentioned this pull request Jul 10, 2023

v18.17.0 release proposal #48694

Merged

YspritanHyzygy mentioned this pull request Jul 27, 2026

doc: document stream.isDestroyed() #64789

Merged

Uh oh!

Uh oh!

Conversation

ronag commented Nov 29, 2022

Uh oh!

nodejs-github-bot commented Nov 29, 2022

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ErickWendel commented Nov 29, 2022

Uh oh!

ronag commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ErickWendel commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Nov 29, 2022

Uh oh!

vweevers commented Nov 29, 2022

Uh oh!

RaisinTen commented Nov 30, 2022

Uh oh!

nodejs-github-bot commented Nov 30, 2022

Uh oh!

ronag commented Dec 19, 2022

Uh oh!

ronag commented Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Dec 19, 2022

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Mar 16, 2023

Uh oh!

nodejs-github-bot commented Mar 18, 2023

Uh oh!

nodejs-github-bot commented Mar 18, 2023

Uh oh!

nodejs-github-bot commented Mar 24, 2023

Uh oh!

anonrig commented Mar 24, 2023

Uh oh!

ronag commented Mar 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

ronag commented Nov 29, 2022 •

edited

Loading

ronag commented Nov 29, 2022 •

edited

Loading

ErickWendel commented Nov 29, 2022 •

edited

Loading

ronag commented Dec 19, 2022 •

edited

Loading