Skip to content
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

webstreams: integrate into common stream utils #39316

Closed
jasnell opened this issue Jul 9, 2021 · 18 comments
Closed

webstreams: integrate into common stream utils #39316

jasnell opened this issue Jul 9, 2021 · 18 comments
Labels
good first issue Issues that are suitable for first-time contributors. stream Issues and PRs related to the stream subsystem. web streams

Comments

@jasnell
Copy link
Member

jasnell commented Jul 9, 2021

With the introduction of web streams, it would be good to integrate support into the various common stream utilities...

Refs: #39134

/cc @mcollina @ronag

  • stream.finished()
const { finished } = require('stream');
finished(new ReadableStream(), (err) => { /* ... */ });
finished(new WritableStream(), (err) => { /* ... */ });
finished(new TransformStream(), (err) => { /* ... */ });
import { finished } from 'stream/promises';
await finished(new ReadableStream());
await finished(new WritableStream());
await finished(new TransformStream());
  • stream.pipeline()
const { pipeline } = require('stream');
pipeline(new ReadableStream(), new TransformStream(), new WritableStream(), (err) => { /* ... */ });
import { pipeline } from 'stream/promises';
await pipeline(new ReadableStream(), new TransformStream(), new WritableStream());
  • stream.addAbortSignal()
const { addAbortSignal } = require('stream');
const ac = new AbortController();
const readable = new ReadableStream();
addAbortSignal(ac.signal, readable);
@mcollina mcollina added the good first issue Issues that are suitable for first-time contributors. label Jul 9, 2021
@mcollina
Copy link
Member

mcollina commented Jul 9, 2021

As far as streams goes, this is probably a good first issue.

@ronag ronag added the stream Issues and PRs related to the stream subsystem. label Jul 10, 2021
@ghost
Copy link

ghost commented Jul 12, 2021

Going to take a look at this as my first issue. Is that okay?

@ronag
Copy link
Member

ronag commented Jul 12, 2021

No need to ask for permission. Just go ahead.

@Thokya
Copy link

Thokya commented Jul 26, 2021

Hello, I would like to assist on this issue. Can I get some more documentation and reading

@jimmywarting
Copy link

jimmywarting commented Jul 27, 2021

import { pipeline } from 'stream/promises';
await pipeline(new ReadableStream(), new TransformStream(), new WritableStream());

Adapting pipeline to whatwg:streams feels almost a bit unnecessary, whatwg:streams are already kind of promise based already

await new ReadableStream()
  .pipeThrough(new TransformStream())
  .pipeTo(new WritableStream())

I think i would never use pipeline and whatwg:streams together, I don't know so much about finished either...

@ronag
Copy link
Member

ronag commented Aug 3, 2021

#39519 did add support for some of the things here but lacked tests so it was disabled. If someone wants to pick up this issue I would recommend looking at #39519, uncomment the web stream stuff, add tests and fix any remaining issue there.

@ghost
Copy link

ghost commented Aug 3, 2021

Thanks for the heads up. I'll take a look this week :)

@mcollina
Copy link
Member

mcollina commented Aug 3, 2021

Adapting pipeline to whatwg:streams feels almost a bit unnecessary, whatwg:streams are already kind of promise based already

The goal should be to add those for cross compatibility, so we can "pipe" from a WHATWG Stream to a Node.js Stream easily and without additional overhead.

@evanrittenhouse
Copy link

@mcollina does this still need to be handled? I can take it if so, although will take me a while.

@iSatVeerSingh
Copy link

is this issue still unresolved ?? Can anyone tell me please??

@mcollina
Copy link
Member

I think this is still unresolved. A PR adding support (or at a minimum tests) for this would be highly appreciated.

The use case these utilities are there to answer is to create interoperability between webstreams and nodestreams.

@Marcellofabrizio
Copy link

@mcollina any documentation I can read to get more information on this? I feel like I can handle this as a first-timer.

@mcollina
Copy link
Member

Not much TBH, however in the simplistic form: start writing a test, and keep working until it passes.

@Marcellofabrizio
Copy link

#39519 did add support for some of the things here but lacked tests so it was disabled. If someone wants to pick up this issue I would recommend looking at #39519, uncomment the web stream stuff, add tests and fix any remaining issue there.

@mcollina maybe pick up after this?

@mcollina
Copy link
Member

yes!

@vijayabhaskar-ev
Copy link

Can i take up this issue or it is resolved?

@mcollina
Copy link
Member

go for it

debadree25 added a commit to debadree25/node that referenced this issue Jan 22, 2023
debadree25 added a commit to debadree25/node that referenced this issue Jan 23, 2023
Refs: nodejs#39316
PR-URL: nodejs#46205
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
juanarbol pushed a commit that referenced this issue Jan 26, 2023
Refs: #39316
PR-URL: #46205
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
juanarbol pushed a commit that referenced this issue Jan 31, 2023
Refs: #39316
PR-URL: #46205
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
nodejs-github-bot pushed a commit that referenced this issue Feb 2, 2023
Refs: #39316
PR-URL: #46307
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
debadree25 added a commit to debadree25/node that referenced this issue Feb 2, 2023
debadree25 added a commit to debadree25/node that referenced this issue Feb 15, 2023
debadree25 added a commit to debadree25/node that referenced this issue Feb 15, 2023
nodejs-github-bot pushed a commit that referenced this issue Feb 17, 2023
Refs: #39316
PR-URL: #46273
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Feb 18, 2023
Refs: #39316
PR-URL: #46307
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
MylesBorins pushed a commit that referenced this issue Feb 18, 2023
Refs: #39316
PR-URL: #46273
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Feb 20, 2023
Refs: #39316
PR-URL: #46273
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
nodejs-github-bot pushed a commit that referenced this issue Feb 27, 2023
Refs: #39316
PR-URL: #46675
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@debadree25
Copy link
Member

debadree25 commented Feb 27, 2023

Implemented in:

debadree25 added a commit to debadree25/node that referenced this issue Feb 27, 2023
Refs: nodejs#39316
PR-URL: nodejs#46273
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
debadree25 added a commit to debadree25/node that referenced this issue Feb 27, 2023
Refs: nodejs#39316
PR-URL: nodejs#46307
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
debadree25 added a commit to debadree25/node that referenced this issue Feb 27, 2023
Refs: nodejs#39316
PR-URL: nodejs#46675
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
debadree25 added a commit to debadree25/node that referenced this issue Feb 27, 2023
Refs: nodejs#39316
PR-URL: nodejs#46675
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
debadree25 added a commit to debadree25/node that referenced this issue Feb 27, 2023
Refs: nodejs#39316
PR-URL: nodejs#46273
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
debadree25 added a commit to debadree25/node that referenced this issue Feb 27, 2023
Refs: nodejs#39316
PR-URL: nodejs#46307
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
debadree25 added a commit to debadree25/node that referenced this issue Feb 27, 2023
Refs: nodejs#39316
PR-URL: nodejs#46675
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Mar 13, 2023
Refs: #39316
PR-URL: #46675
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Mar 14, 2023
Refs: #39316
PR-URL: #46675
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this issue Apr 11, 2023
Refs: #39316
PR-URL: #46307
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
danielleadams pushed a commit that referenced this issue Apr 11, 2023
Refs: #39316
PR-URL: #46273
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this issue Apr 11, 2023
Refs: #39316
PR-URL: #46675
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are suitable for first-time contributors. stream Issues and PRs related to the stream subsystem. web streams
Projects
None yet
Development

No branches or pull requests