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: Fix the ServerResponse methods explainations to make it clear and not puzzled #22305

Closed
wants to merge 2 commits into from
Closed

Conversation

ghost
Copy link

@ghost ghost commented Aug 14, 2018

Ref: #14146.

In short: ServerResponse acutally inherits from OutgoingMessage,
with a series of methods like those in Stream.Writable. So we cannot
use implements(this has made poeple feel puzzled because there are
still many methods we don't need or have), so inherits from Stream is enough,
due to some core reasons and performance told by mcollina from the ref
(See some latest discussions at Ref).

  • 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]

@nodejs-github-bot nodejs-github-bot added the doc label Aug 14, 2018
@ghost
Copy link
Author

@ghost ghost commented Aug 14, 2018

/cc:@mcollina :)

@Trott
Copy link
Member

@Trott Trott commented Aug 14, 2018

doc/api/http.md Outdated
The request implements the [Writable Stream][] interface. This is an
[`EventEmitter`][] with the following events:
The request inherits from [Stream][], with some methods like what we have in
[Writable Stream][]. There's an [`EventEmitter`][] with the following events:
Copy link
Member

@mcollina mcollina Aug 14, 2018

Choose a reason for hiding this comment

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

I would change There's an EventEmitter with the following events to It will emit the following events:

The There's an EventEmitter part looks like it is something different from the response.

Copy link
Author

@ghost ghost Aug 14, 2018

Choose a reason for hiding this comment

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

OK, fixed. Thanks!

Copy link
Member

@mcollina mcollina left a comment

LGTM

@trivikr
Copy link
Member

@trivikr trivikr commented Aug 14, 2018

@trivikr trivikr added the author ready label Aug 14, 2018
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Doc format LGTM.

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Aug 14, 2018

Node.js Collaborators, please, add 👍 here if you approve fast-tracking.

lpinca
lpinca approved these changes Aug 14, 2018
@richardlau
Copy link
Member

@richardlau richardlau commented Aug 14, 2018

Please could whoever lands this correct the spelling of explanations in the commit message?

@vsemozhetbyt vsemozhetbyt added the fast-track label Aug 14, 2018
Trott
Trott previously requested changes Aug 14, 2018
Copy link
Member

@Trott Trott left a comment

I'm confused by this. Does it implement all the methods of Writeable Stream? If so, then "with all methods of Writable Stream". But if that's the case, then "implements the interface" seems correct to me in the first place.

If it's not all the methods, can we say which methods it implements and which it does not?

@ghost
Copy link
Author

@ghost ghost commented Aug 15, 2018

@Trott

Does it implement all the methods of Writeable Stream?

The answer is:No.

So you are also feeling confused by such sayings now :)

But don't worry. Let's compare the methods together in both Stream.Writable and Stream to make it more clear!

【Stream.Writable】
writable.cork()
writable.end([chunk][, encoding][, callback])
writable.setDefaultEncoding(encoding)
writable.uncork()
writable.writableHighWaterMark
writable.write(chunk[, encoding][, callback])
writable.destroy([error])

【ServerResponse】
response.addTrailers(headers)
response.connection
response.end([data][, encoding][, callback])
response.finished
response.getHeader(name)
response.getHeaderNames()
response.getHeaders()
response.hasHeader(name)
response.headersSent
response.removeHeader(name)
response.sendDate
response.setHeader(name, value)
response.setTimeout(msecs[, callback])
response.socket
response.statusCode
response.statusMessage
response.write(chunk[, encoding][, callback])
response.writeContinue()
response.writeHead(statusCode[, statusMessage][, headers])

ONLY the two in bold below are the same (similar but NOT FROM Stream.Writable). Other methods from Stream.Writable aren't implemented yet (Maybe we don't need them according to the real case).

@ghost
Copy link
Author

@ghost ghost commented Aug 15, 2018

@Trott:Add two diff methods as more clear. Thanks

doc/api/http2.md Outdated
The response inherits from [Stream][], with some methods like what we have in
[Writable Stream][]. It will emit the following events:
The response inherits from [Stream][], with two methods like what we have in
[Writable Stream][]: [`response.write()`][] and [`response.end()`][].
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Aug 15, 2018

Choose a reason for hiding this comment

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

Is It will emit the following events: omission intentional?

Copy link
Member

@Trott Trott Aug 15, 2018

Choose a reason for hiding this comment

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

I don't think we usually preface documented events with a sentence like that so I'm for removing it.

doc/api/http.md Outdated
@@ -313,8 +313,10 @@ the data is read it will consume memory that can eventually lead to a
Node.js does not check whether Content-Length and the length of the
body which has been transmitted are equal or not.

The request implements the [Writable Stream][] interface. This is an
[`EventEmitter`][] with the following events:
The request inherits from [Stream][], with two methods like what we have in
Copy link
Member

@Trott Trott Aug 15, 2018

Choose a reason for hiding this comment

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

Can we just omit the two methods and the mention of Writable Stream? The methods are documented below and mentioning Writable Stream here seems to confuse more than enlighten.

The request inherits from [Stream][].

That's all that needs to be said. (Or is there a good reason to mention the other stuff?)

Copy link
Author

@ghost ghost Aug 15, 2018

Choose a reason for hiding this comment

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

Consider we still have events' introductions below it, so what about saying this as a transitional one?

The` request inherits from [Stream][], with some events, properties and methods below:

doc/api/http.md Outdated
The request inherits from [Stream][], with two methods like what we have in
[Writable Stream][]: [`request.write()`][] and [`request.end()`][].

And it will emit the following events:
Copy link
Member

@Trott Trott Aug 15, 2018

Choose a reason for hiding this comment

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

This line is unnecessary and can be removed.

Copy link
Author

@ghost ghost Aug 15, 2018

Choose a reason for hiding this comment

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

OK. I'll remove.

@ghost
Copy link
Author

@ghost ghost commented Aug 15, 2018

@Trott
I agree with you on your 1st point:Remove useless and disturbing statements but just keep one, which makes it clearer.

However it would be nicer if we add some transitional statement to link the explainations with the events, I don't want to see that events' introduction suddenly occur without a brief transitional introduction. Something like this:

The request inherits from [Stream][], with some properties, events and methods below:

#Event 'connect'
……

If you insist your ideas and please still tell me, I'll do what you want :)

@Trott
Copy link
Member

@Trott Trott commented Aug 15, 2018

How about this?:

The request inherits from [Stream][], and additionally implements the following:

#Event 'connect'

@ghost
Copy link
Author

@ghost ghost commented Aug 15, 2018

@Trott:Fixed, thanks!

@Trott Trott dismissed their stale review Aug 15, 2018

text is no longer confusing, 👍

Ref: #14146.

In short: `ServerResponse` acutally inherits from `OutgoingMessage`,
with a series of methods like those in `Stream.Writable`. So we cannot
use `implements`(this has made poeple feel puzzled because there are
still many methods we don't need or have), so `inherits from Stream` is enough,
due to some core reasons and performance told by mcollina from the ref
(See some latest discussions at Ref).
@Trott
Copy link
Member

@Trott Trott commented Aug 15, 2018

@Trott
Copy link
Member

@Trott Trott commented Aug 16, 2018

@ghost
Copy link
Author

@ghost ghost commented Aug 16, 2018

Thanks! It seems I've passed all.

@Trott
Copy link
Member

@Trott Trott commented Aug 16, 2018

Landed in 16accff

@Trott Trott closed this Aug 16, 2018
Trott pushed a commit to Trott/io.js that referenced this issue Aug 16, 2018
In short: `ServerResponse` acutally inherits from `OutgoingMessage`,
with a series of methods like those in `Stream.Writable`. So we cannot
use `implements`(this has made poeple feel puzzled because there are
still many methods we don't need or have), so `inherits from Stream` is
enough, due to some core reasons and performance told by mcollina from
the ref (See some latest discussions at Ref).

Ref: nodejs#14146.
PR-URL: nodejs#22305
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@ghost ghost deleted the HttpUpdate branch Aug 16, 2018
@ghost
Copy link
Author

@ghost ghost commented Aug 16, 2018

An interesting thing I've noticed quite long:we landed the submitted changes and closed it, instead of merging it. How to do that? Is there anything benifits/diff for doing that, compared with "merging"?

@Trott
Copy link
Member

@Trott Trott commented Aug 16, 2018

@Maledong See https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#technical-howto for our process. There's an optional step to get things merged rather than closed. Most of us skip that step.

@ghost
Copy link
Author

@ghost ghost commented Aug 16, 2018

OK, Thanks for all of your patience!

targos pushed a commit that referenced this issue Aug 19, 2018
In short: `ServerResponse` acutally inherits from `OutgoingMessage`,
with a series of methods like those in `Stream.Writable`. So we cannot
use `implements`(this has made poeple feel puzzled because there are
still many methods we don't need or have), so `inherits from Stream` is
enough, due to some core reasons and performance told by mcollina from
the ref (See some latest discussions at Ref).

Ref: #14146.
PR-URL: #22305
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this issue Sep 3, 2018
In short: `ServerResponse` acutally inherits from `OutgoingMessage`,
with a series of methods like those in `Stream.Writable`. So we cannot
use `implements`(this has made poeple feel puzzled because there are
still many methods we don't need or have), so `inherits from Stream` is
enough, due to some core reasons and performance told by mcollina from
the ref (See some latest discussions at Ref).

Ref: #14146.
PR-URL: #22305
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready doc fast-track
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants