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

doc: stream.compose documentation improvement #40861

Closed
wants to merge 3 commits into from
Closed

doc: stream.compose documentation improvement #40861

wants to merge 3 commits into from

Conversation

JoakimCh
Copy link

@JoakimCh JoakimCh commented Nov 18, 2021

  • made it more clear that it must be used with Duplex streams (e.g. transforms)
  • and that its functionality should not be confused with stream.Duplex.from({writable, readable})

My first pull request ever, resolves #40812

Made it more clear that it should be used with Duplex streams (e.g. transforms) and that its functionality should not be confused with `stream.Duplex.from({writable, readable})`.
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Nov 18, 2021
are destroyed, including the outer `Duplex` stream.
Combines two or more `Duplex` streams (e.g. transforms) into a `Duplex`
stream that writes to the first stream and reads from the last. Each provided
stream is piped into the next, using `stream.pipeline`. If any of the streams
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
stream is piped into the next, using `stream.pipeline`. If any of the streams
stream is piped into the next, using `stream.pipeline()`. If any of the streams

stream is piped into the next, using `stream.pipeline`. If any of the streams
error then all are destroyed, including the outer `Duplex` stream.

It should not be confused with `stream.Duplex.from({writable, readable})`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
It should not be confused with `stream.Duplex.from({writable, readable})`
It should not be confused with `stream.Duplex.from()`


It should not be confused with `stream.Duplex.from({writable, readable})`
which can combine a write-only stream with a read-only stream to create a
`Duplex` (where the streams doesn't get piped).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`Duplex` (where the streams doesn't get piped).
`Duplex` (where the streams don't get piped).

error then all are destroyed, including the outer `Duplex` stream.

It should not be confused with `stream.Duplex.from({writable, readable})`
which can combine a write-only stream with a read-only stream to create a
Copy link
Contributor

@vweevers vweevers Nov 19, 2021

Choose a reason for hiding this comment

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

This comparison to stream.Duplex.from() can be generalized because it's not necessarily about "write-only" or "read-only" (which are terms that are not used in existing docs) as in having only a writable or readable side. Rather, it's about whether to use those sides.

So for this paragraph I suggest:

Use other utilities when the streams are not Duplex or when connecting them is not desirable. For example, to instead create a Duplex stream from a writable stream and a readable stream without piping one into the other, use stream.Duplex.from().

In addition, can we move it down by one paragraph? Because describing the difference between stream.compose() and stream.pipeline() is IMO more important than describing the difference between stream.compose() and stream.Duplex.from(). And the first paragraph already mentions stream.pipeline() so that question is brought up first.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM with the suggested edits

@JoakimCh JoakimCh closed this Apr 25, 2022
@JoakimCh JoakimCh deleted the patch-1 branch April 25, 2022 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stream.compose documentation could be better
6 participants