stream: expose stream symbols #45671

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-10T09:05:59Z

Commit Queue failed

- Loading data for nodejs/node/pull/45671
✔  Done loading data for nodejs/node/pull/45671
----------------------------------- PR info ------------------------------------
Title      stream: expose stream symbols (#45671)
Author     Robert Nagy  (@ronag)
Branch     ronag:expose-stream-symbols -> nodejs:main
Labels     stream, author ready
Commits    1
 - stream: expose stream symbols
Committers 1
 - Robert Nagy 
PR-URL: https://github.com/nodejs/node/pull/45671
Reviewed-By: Matteo Collina 
Reviewed-By: James M Snell 
Reviewed-By: Antoine du Hamel 
Reviewed-By: Benjamin Gruenbaum 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/45671
Reviewed-By: Matteo Collina 
Reviewed-By: James M Snell 
Reviewed-By: Antoine du Hamel 
Reviewed-By: Benjamin Gruenbaum 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - stream: expose stream symbols
   ℹ  This PR was created on Tue, 29 Nov 2022 10:33:01 GMT
   ✔  Approvals: 4
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/45671#pullrequestreview-1307877767
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/45671#pullrequestreview-1223224391
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/45671#pullrequestreview-1240689090
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/45671#pullrequestreview-1318093984
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2023-01-05T18:00:02Z: https://ci.nodejs.org/job/node-test-pull-request/48871/
   ⚠  Commits were pushed after the last Full PR CI run:
   ⚠  - stream: expose stream symbols
- Querying data for job/node-test-pull-request/48871/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/4382938745

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>

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

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

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 10, 2023

debadree25 approved these changes Mar 16, 2023

View reviewed changes

debadree25 added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Mar 16, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 16, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: expose stream symbols #45671

stream: expose stream symbols #45671

ronag commented Nov 29, 2022

nodejs-github-bot commented Nov 29, 2022

aduh95 left a comment

ronag commented Nov 29, 2022 •

edited

Loading

ErickWendel commented Nov 29, 2022

ronag commented Nov 29, 2022 •

edited

Loading

ErickWendel commented Nov 29, 2022 •

edited

Loading

ronag commented Nov 29, 2022

vweevers commented Nov 29, 2022

RaisinTen commented Nov 30, 2022

nodejs-github-bot commented Nov 30, 2022

ronag commented Dec 19, 2022

ronag commented Dec 19, 2022 •

edited

Loading

aduh95 commented Dec 19, 2022

nodejs-github-bot commented Mar 10, 2023

This comment was marked as outdated.

nodejs-github-bot commented Mar 16, 2023

nodejs-github-bot commented Mar 18, 2023

nodejs-github-bot commented Mar 18, 2023

nodejs-github-bot commented Mar 24, 2023

anonrig commented Mar 24, 2023

ronag commented Mar 24, 2023

stream: expose stream symbols #45671

stream: expose stream symbols #45671

Conversation

ronag commented Nov 29, 2022

nodejs-github-bot commented Nov 29, 2022

aduh95 left a comment

Choose a reason for hiding this comment

ronag commented Nov 29, 2022 • edited Loading

ErickWendel commented Nov 29, 2022

ronag commented Nov 29, 2022 • edited Loading

ErickWendel commented Nov 29, 2022 • edited Loading

ronag commented Nov 29, 2022

vweevers commented Nov 29, 2022

RaisinTen commented Nov 30, 2022

nodejs-github-bot commented Nov 30, 2022

ronag commented Dec 19, 2022

ronag commented Dec 19, 2022 • edited Loading

aduh95 commented Dec 19, 2022

nodejs-github-bot commented Mar 10, 2023

This comment was marked as outdated.

nodejs-github-bot commented Mar 16, 2023

nodejs-github-bot commented Mar 18, 2023

nodejs-github-bot commented Mar 18, 2023

nodejs-github-bot commented Mar 24, 2023

anonrig commented Mar 24, 2023

ronag commented Mar 24, 2023

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