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: revise http documentation #8486

Closed
wants to merge 2 commits into
base: master
from

Conversation

@TimothyGu
Member

TimothyGu commented Sep 10, 2016

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
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.

@mscdex mscdex added http doc labels Sep 11, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Sep 12, 2016

* `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}

This comment has been minimized.

@thefourtheye

thefourtheye Sep 12, 2016

Contributor

Strictly speaking, async functions return nothing. Isn't that the case here?

This comment has been minimized.

@TimothyGu

TimothyGu Sep 12, 2016

Member

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

This comment has been minimized.

@thefourtheye

thefourtheye Sep 25, 2016

Contributor

Okay, in that case, can we atleast specify that the callback is optional?

This comment has been minimized.

@TimothyGu

TimothyGu Sep 25, 2016

Member

It is, as shown in the function signature:

### agent.createConnection(options[, callback])

@Trott Trott force-pushed the nodejs:master branch to c5ce7f4 Sep 21, 2016

@imyller

LGTM

@addaleax

LGTM, thanks for putting in the effort to do this!

* `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

This comment has been minimized.

@lpinca

lpinca Sep 25, 2016

Member

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

This comment has been minimized.

@lpinca

lpinca Sep 25, 2016

Member

This should be 'request'.

@lpinca

lpinca approved these changes Sep 25, 2016

LGTM

@lpinca

This comment has been minimized.

Member

lpinca commented Sep 25, 2016

I really appreciate the effort that has been put here, thanks.

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Sep 25, 2016

http.md
  1585:1-1585:38  warning  Found unused definition  no-unused-definitions  remark-lint

⚠ 1 warning

This definition became unused:

[`Buffer`]: buffer.html#buffer_buffer
@jasnell

This comment has been minimized.

Member

jasnell commented Oct 7, 2016

Ping @TimothyGu ... can you take a look at @ChALkeR's most recent comment.

TimothyGu added some commits Sep 10, 2016

doc: fix location of `'checkExpectation'` event
It is emitted by the Server, analogous to the `'checkContinue'` event,
not by the ClientRequest.
@TimothyGu

This comment has been minimized.

Member

TimothyGu commented Oct 8, 2016

@jasnell, fixed.

@TimothyGu

This comment has been minimized.

Member

TimothyGu commented Oct 11, 2016

Ping?

@jasnell

This comment has been minimized.

Member

jasnell commented Oct 11, 2016

@ChALkeR ... LGTY?

@ChALkeR

LGTM

@jasnell jasnell self-assigned this Oct 11, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Oct 11, 2016

LGTM

jasnell added a commit that referenced this pull request Oct 11, 2016

doc: revise http documentation
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>
@jasnell

This comment has been minimized.

Member

jasnell commented Oct 11, 2016

Landed in 52c7f9d

@jasnell jasnell closed this Oct 11, 2016

@TimothyGu TimothyGu deleted the TimothyGu:http-params branch Oct 12, 2016

jasnell added a commit that referenced this pull request Oct 12, 2016

doc: revise http documentation
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>

MylesBorins added a commit that referenced this pull request Nov 11, 2016

doc: revise http documentation
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>
@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Nov 11, 2016

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

@TimothyGu TimothyGu referenced this pull request Nov 12, 2016

Closed

doc: fix type of http.request's `agent` option #9584

2 of 2 tasks complete

@MylesBorins MylesBorins referenced this pull request Nov 22, 2016

Merged

v6.9.2 proposal #9735

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment