stream: compose

[ronag]

pipe is similar to pipeline however it supports stream composition, i.e.

const transform3 = stream.pipe(transform1, transform2)
// transform3 is a writable

stream.pipeline(source, transform3, sink, (err) => console.log('err'))

Similar to how rx js provides a top level pipe(...observables) method.

[ronag]

@nodejs/streams Looking for some initial feedback before starting work on tests.

[targos]

@nodejs/streams

[vweevers]

@ronag Knowing that stream.pipeline() is the equivalent of pump, can we say that stream.pipe() is the equivalent of pumpify?

[mscdex]

I'm not familiar with the use cases for this, but I think it can be a bit confusing because of the function name (and where it's being exported) and because we already have the pre-existing pipe() and pipeline(). Does this really need to exist in node core?

[vweevers]

I'm not familiar with the use cases for this

#32020

[ronag]

Does this really need to exist in node core?

There is #32020 and also pumpify which is not entirely correct.

[vweevers]

I think it can be a bit confusing because of the function name [and] the pre-existing pipe()

Some options:

1. No bikeshedding, risking some confusion. Call it pipe() because it's a fitting term. The way I see it, multiple open-ended "pipes" can be combined to form a closed-ended "pipeline". When context requires it, use additional words to differentiate between stream.pipe and stream.prototype.pipe. Same as events.once versus events.prototype.once.
2. Make up a new distinct term, which could take more effort to explain. Ex: segment(), compose().
3. Make it descriptive at the cost of verbosity. Ex: duplexPipeline(), duplexFrom(), writablePipeline().

[ronag]

Rxjs does 1. So there is some precedence.

[MattiasBuelens]

@ronag Sure! 🙂 If it matters for scheduling purposes: I'm based in Europe (currently CEST).

[ronag]

I'm CEST as well. I can be quite flexible next week. What about @benjamingr?

[mcollina]

lgtm

[nodejs-github-bot]

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

[ronag]

This has sufficient approvals but I think @benjamingr would like to further discuss this before landing? @benjamingr would you mind doing a "request for changes" so we don't accidentally land this before that?

[nodejs-github-bot]

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

[ronag]

@nodejs/streams @mcollina I've disabled async function support. I believe in this way this can land without any pending items. I'll open a separate PR we can discuss in regards to async fn/gen API.

This way we can continue with e.g. web stream support

Please 👍 so I know whether I can land this.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[ronag]

Landed in e579acb

[targos]

Like #39134 (comment), this needs a backport to land on v16.x because it depends on the semver-major #39294

[ronag]

#39563