doc: revise http documentation#8486
doc: revise http documentation#8486TimothyGu wants to merge 2 commits intonodejs:masterfrom TimothyGu:http-params
Conversation
mscdex
added
http
|
@nodejs/documentation |
doc/api/http.md
Outdated
There was a problem hiding this comment.
Strictly speaking, async functions return nothing. Isn't that the case here?
There was a problem hiding this comment.
The documentation states:
A socket/stream can be supplied in one of two ways: by returning the socket/stream from this function, or by passing the socket/stream to
callback.
So it's not a "true" async or sync function. Indeed, the code explicitly supports both sync and async usage:
const newSocket = self.createConnection(options, oncreate);
if (newSocket)
oncreate(null, newSocket);There was a problem hiding this comment.
Okay, in that case, can we atleast specify that the callback is optional?
There was a problem hiding this comment.
It is, as shown in the function signature:
### agent.createConnection(options[, callback])
addaleax
left a comment
addaleax
left a comment
There was a problem hiding this comment.
LGTM, thanks for putting in the effort to do this!
doc/api/http.md
Outdated
There was a problem hiding this comment.
This is not 100% true, I stumbled upon this the other day. There are cases where socket.setTimeout() is called even if the socket is not connected.
Anyway it's probably better to investigate/fix this in another issue/PR.
doc/api/http.md
Outdated
|
I really appreciate the effort that has been put here, thanks. |
This definition became unused: |
|
Ping @TimothyGu ... can you take a look at @ChALkeR's most recent comment. |
It is emitted by the Server, analogous to the `'checkContinue'` event, not by the ClientRequest.
|
@jasnell, fixed. |
|
Ping? |
|
@ChALkeR ... LGTY? |
|
LGTM |
PR-URL: #8486 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
Landed in 52c7f9d |
PR-URL: #8486 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #8486 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
I've landed this in v6.x-staging. Please let me know if there is anything in here that is specific to v7 and should not be included |
9 participants
Checklist
Affected core subsystem(s)
doc
Description of change
The current documentation for the http module still follows an older format. Type documentation for parameters were also incomplete. This pull request at least fills up the type information for parameters and returns, so that the parsed JSON is usable.