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

Documentation for "path", "bytesRead", "bytesWritten" properties should also be in stream.html, not just fs.html #14448

Closed
lll000111 opened this issue Jul 24, 2017 · 6 comments
Labels
doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. stream Issues and PRs related to the stream subsystem.

Comments

@lll000111
Copy link

lll000111 commented Jul 24, 2017

Relating to #4327 and #4368

I just searched and searched and had to resort to various tricky Google queries to find that the path property of stream(!) objects is not documented on the "stream" doc page - but on the "fs" page.

https://nodejs.org/dist/latest-v8.x/docs/api/fs.html#fs_writestream_path

I think this belongs to https://nodejs.org/dist/latest-v8.x/docs/api/stream.html#stream_class_stream_writable and https://nodejs.org/dist/latest-v8.x/docs/api/stream.html#stream_class_stream_readable (too)

On that note, I find it highly confusing that there is a class fs.WriteStream (same for ReadStream) documented. Sure, it says "is a WriteStream", but combined with the fact that the documentation is DIFFERENT for the class on the two pages leads to the assumption that maybe it indeed is something different? I had to check the code in lib/fs.js to make sure it really is the same class.

So maybe there should be only one location for stream class documentation - and that should be under "streams"?

@lll000111 lll000111 changed the title Documentation for "path" and "bytesWritten" property should also be in stream.html, not just fs.html Documentation for "path", "bytesRead", "bytesWritten" properties should also be in stream.html, not just fs.html Jul 24, 2017
@addaleax addaleax added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. stream Issues and PRs related to the stream subsystem. labels Jul 24, 2017
@addaleax
Copy link
Member

I just searched and searched and had to resort to various tricky Google queries to find that the path property of stream(!) objects is not documented on the "stream" doc page - but on the "fs" page.

Yes, that’s because non-fs streams don’t have that property (for example, sockets).

I find it highly confusing that there is a class fs.WriteStream (same for ReadStream) documented. Sure, it says "is a WriteStream", but combined with the fact that the documentation is DIFFERENT for the class on the two pages leads to the assumption that maybe it indeed is something different? I had to check the code in lib/fs.js to make sure it really is the same class.

Yes, fs.WriteStream and stream.Writable are different things – fs.WriteStream is a subclass of stream.Writable.

@lll000111
Copy link
Author

lll000111 commented Jul 24, 2017

Okay, some confusion (here). We have a WriteStream and a WriteStream. The entries for both (read/write) streams on the "fs" page say

ReadStream is a Readable Stream.

and

WriteStream is a Writable Stream.

pointing to the stream page. So they are not, really. If I understand you correctly, they are based on ("extends") those streams but have the mentioned additional properties? I'm looking at their code now, but I'd say the documentation could be slightly more clear.




I'm also just found that the documentation for fs.WriteStream and fs.ReadStream shows two events close and open . But close as an event is defined on stream$Writable already (has it been overwritten?) , and what is actually meant is a new method close available on fs.WriteStream and fs.ReadStream? Which I found in the Flow library definitions for "node.js/fs" and then found in lib/fs.js as ReadStream.prototype.close = .... (later also copied to WriteStream).

After checking write-stream docs in both "fs" and "stream" and also the code a little bit I'm not sure how I should end a file write stream: Call the end method it got from "stream", or call the close method it got from "fs"?

The example for a file write stream(!) actually is in "streams":
https://nodejs.org/api/stream.html#stream_writable_end_chunk_encoding_callback

So what is the close method for? Ist it purely internal? It doesn't have a "_" in front like the other internal functions (and why did the Flow lib.def. authors pick it up as official method - but that's another question).

@ghost
Copy link

ghost commented Aug 3, 2017

Okay, some confusion (here). We have a WriteStream and a WriteStream.

No, the types are:

  • stream.WritableStream and fs.WriteStream (note the name difference, Writable vs Write)
  • stream.ReadableStream and fs.ReadStream (note the name difference, Readable vs Read)

The documentation is correct.

@lll000111
Copy link
Author

lll000111 commented Aug 3, 2017

The documentation is confusing. Also, what I wrote - a bit more than half a sentence.

I'll unsubscribe, I said what I had to say - the path forward does not look like it's going to be a fruitful and/or useful discussion.

@ghost
Copy link

ghost commented Aug 3, 2017

This issue asks to add members of filesystem streams to their parent types in the documentation. This is not the case in the implementation, therefore would be incorrect to do.

The stream types in the stream module (e.g: Readable, Writable, Duplex and Transform) are abstract types, that can be extended to implement a concrete stream type. There are many examples of this in node itself (e.g: http.IncomingMessage extends stream.Readable).

In OOP, the able suffix usually hints the type is an abstract type or interface.

I quite honestly do not see the confusion here. However, a link could help making this clearer.

e.g:

-- ReadStream is a Readable Stream
++ fs.ReadStream is a stream.Readable

Possibly with links to make it easier to navigate.

@apapirovski
Copy link
Member

I don't think there's anything confusing here and no one has really volunteered to make this change in over 6 months. Closing but feel free to re-open if you're submitting a PR to change it.

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. fs Issues and PRs related to the fs subsystem / file system. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants