doc: Fix the `ServerResponse` methods explainations to make it clear and not puzzled · Pull Request #22305 · nodejs/node · GitHub

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]

/cc：@mcollina :)

@nodejs/documentation

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.

OK, fixed. Thanks!

LGTM

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/560/

Doc format LGTM.

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

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

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?

@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).

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

Is It will emit the following events: omission intentional?

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

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?)

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:

This line is unnecessary and can be removed.

OK. I'll remove.

@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 :)

How about this?:

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

#Event 'connect'

@Trott：Fixed, thanks!

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).

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/566/

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/567/

Thanks! It seems I've passed all.

Landed in 16accff

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>

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"?

@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.

OK, Thanks for all of your patience!

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>

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Aug 14, 2018

mcollina reviewed Aug 14, 2018

View reviewed changes

mcollina approved these changes Aug 14, 2018

View reviewed changes

trivikr approved these changes Aug 14, 2018

View reviewed changes

trivikr added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Aug 14, 2018

vsemozhetbyt approved these changes Aug 14, 2018

View reviewed changes

maclover7 approved these changes Aug 14, 2018

View reviewed changes

lpinca approved these changes Aug 14, 2018

View reviewed changes

vsemozhetbyt added the fast-track PRs that do not need to wait for 48 hours to land. label Aug 14, 2018

Trott previously requested changes Aug 14, 2018

View reviewed changes

vsemozhetbyt reviewed Aug 15, 2018

View reviewed changes

Trott reviewed Aug 15, 2018

View reviewed changes

Remove useless refs

934c390

Trott closed this Aug 16, 2018

ghost deleted the HttpUpdate branch August 16, 2018 03:01

targos mentioned this pull request Sep 5, 2018

Release proposal: v10.10.0 #22716

Merged

This was referenced Sep 6, 2018

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-android-nodejs#236

Closed

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-nodejs#221

Closed

gireeshpunathil mentioned this pull request Dec 30, 2019

HttpServerResponse.setDefaultEncoding() #14146

Closed

doc: Fix the `ServerResponse` methods explainations to make it clear and not puzzled · Pull Request #22305 · nodejs/node · GitHub

Uh oh!

Uh oh!

doc: Fix the ServerResponse methods explainations to make it clear and not puzzled - #22305

doc: Fix the ServerResponse methods explainations to make it clear and not puzzled#22305ghost wants to merge 2 commits intomasternodejs/node:masterfrom unknown repositoryCopy head branch name to clipboard

Conversation

ghost commented Aug 14, 2018

Uh oh!

ghost commented Aug 14, 2018

Uh oh!

Trott commented Aug 14, 2018

Uh oh!

mcollina Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

ghost Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

trivikr commented Aug 14, 2018

Uh oh!

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Aug 14, 2018

Uh oh!

richardlau commented Aug 14, 2018

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 15, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Aug 15, 2018

Uh oh!

vsemozhetbyt Aug 15, 2018

Choose a reason for hiding this comment

Uh oh!

Trott Aug 15, 2018

Choose a reason for hiding this comment

Uh oh!

Trott Aug 15, 2018

Choose a reason for hiding this comment

Uh oh!

ghost Aug 15, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott Aug 15, 2018

Choose a reason for hiding this comment

Uh oh!

ghost Aug 15, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 15, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Aug 15, 2018

Uh oh!

ghost commented Aug 15, 2018

Uh oh!

Trott commented Aug 15, 2018

Uh oh!

Trott commented Aug 16, 2018

Uh oh!

ghost commented Aug 16, 2018

Uh oh!

Trott commented Aug 16, 2018

Uh oh!

ghost commented Aug 16, 2018

Uh oh!

Trott commented Aug 16, 2018

Uh oh!

ghost commented Aug 16, 2018

Uh oh!

Reviewers

doc: Fix the ServerResponse methods explainations to make it clear and not puzzled#22305
ghost wants to merge 2 commits into
masterfrom
unknown repository

ghost commented Aug 15, 2018 •
edited by ghost
Loading

ghost Aug 15, 2018 •
edited by ghost
Loading

ghost Aug 15, 2018 •
edited by ghost
Loading

ghost commented Aug 15, 2018 •
edited by ghost
Loading