doc: revise http documentation #8486
Conversation
mscdex
added
http
|
@nodejs/documentation |
| * `options` {Object} Options containing connection details. Check | ||
| [`net.createConnection()`][] for the format of the options | ||
| * `callback` {Function} Callback function that receives the created socket | ||
| * Returns: {net.Socket} |
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])
| * `timeout` {Number} Milliseconds before a request is considered to be timed out. | ||
| * `callback` {Function} Optional function to be called when a timeout occurs. Same as binding to the `timeout` event. | ||
|
|
||
| Once a socket is assigned to this request and is connected |
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.
| Emitted when the server closes. | ||
|
|
||
| ### Event: 'connect' | ||
| <!-- YAML | ||
| added: v0.7.0 | ||
| --> | ||
|
|
||
| `function (request, socket, head) { }` | ||
| * `request` {http.IncomingMessage} Arguments for the HTTP request, as it is in | ||
| the [`request`][] event |
|
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 |
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.