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

[WIP] doc: better stream docs #19143

Closed
wants to merge 1 commit into from
Closed

Conversation

wuweiweiwu
Copy link
Contributor

@wuweiweiwu wuweiweiwu commented Mar 5, 2018

Adding better and more user friendly stream docs as per #8646

Any suggestions / feedback is extremely welcomed :) I'm definitely missing some npm modules.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Adding new section headers for stream docs.
@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 Mar 5, 2018
@MoonBall
Copy link
Member

MoonBall commented Mar 6, 2018

One nit: concat-stream appeared twice in core npm modules.

@davisjam
Copy link
Contributor

Suggestion: Add a section early on explaining why streams are such an important concept in Node. The current document takes this for granted in the front matter, but given that this is quite a long document I think we might as well be near-pedantic about it.

additional notes. The first section explains the elements of the stream API that
are required to *use* streams within an application. The second section explains
the elements of the API that are required to *implement* new types of streams.
The third section is geared towards explanations and *code examples*. The fourth
section showcases important *npm modules* that make working with streams
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we really want to implement a section like that to be honest. In the past, we've avoided the appearance of promoting certain modules over their competitors. We'd want to be very careful about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! I can get rid of the specific section and maybe instead have examples that use those modules?

additional notes. The first section explains the elements of the stream API that
are required to *use* streams within an application. The second section explains
the elements of the API that are required to *implement* new types of streams.
The third section is geared towards explanations and *code examples*. The fourth
Copy link
Member

Choose a reason for hiding this comment

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

Nit: "geared towards explanations" is less informative than this should be IMO. Maybe this?

The third section explains <whatever it is supposed to explain> including code samples.

Or perhaps even better, this?

The third section contains code samples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! Will update

@Trott
Copy link
Member

Trott commented Mar 17, 2018

@nodejs/documentation @nodejs/streams

@mcollina
Copy link
Member

flush-write-stream is not needed as we have added _final in Writable now.

I think this should better live in a guide in the website.

@wuweiweiwu
Copy link
Contributor Author

wuweiweiwu commented Mar 18, 2018

@mcollina Should I move this to https://github.com/nodejs/nodejs.org?

perhaps one of the guides?

@mcollina
Copy link
Member

@wuweiweiwu I think so, yes.

@wuweiweiwu
Copy link
Contributor Author

@mcollina Sounds good! Ill close this and open a PR in nodejs.org repo

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.

6 participants