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: argument types for https & argument types cleanup for http #11681

Closed
wants to merge 2 commits into from

Conversation

ameliavoncat
Copy link
Contributor

@ameliavoncat ameliavoncat commented Mar 4, 2017

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

documentation

Description of changes

Added argument data types to the docs for the https module.
Added missing argument data types to the docs for the http module.
Lowercased primitive data types in the http module for consistency.
Changed Integer data types to Number for consistency.

Issue

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Mar 4, 2017
@hiroppy hiroppy added the http Issues or PRs related to the http subsystem. label Mar 4, 2017
doc/api/https.md Outdated
- `options` {Object | String}
- `method` {String} Default = `GET`
- `protocol` {String} Default = `http`
- `host` | `hostname` {String} Default = `localhost`
Copy link
Member

Choose a reason for hiding this comment

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

Maybe put them in separate entries and mention they are aliases?

doc/api/http.md Outdated
@@ -110,16 +110,16 @@ added: v0.3.4

* `options` {Object} Set of configurable options to set on the agent.
Can have the following fields:
* `keepAlive` {boolean} Keep sockets around even when there are no
Copy link
Member

Choose a reason for hiding this comment

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

Er..I think lowercased primitive types are preferred after #11167?

Copy link
Contributor Author

@ameliavoncat ameliavoncat Mar 4, 2017

Choose a reason for hiding this comment

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

Ahh, I had read the summary (which said the opposite) but not the comment thread. Will change.

doc/api/https.md Outdated
[`tls.createServer()`][]. The `requestListener` is a function which is
- `options` {Object}
- `pfx` {String | Buffer}
- `key` {String | Buffer | Array[strings | buffers | objects]}
Copy link
Member

Choose a reason for hiding this comment

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

Buffer[] | string[] | Object[] are supported by the tools now..

doc/api/https.md Outdated
- `requestListener` {Function}

Returns a new HTTPS web server object. The `options` are similar to
[`tls.createServer()`][] and [`tls.createSecureContext()`][]. The `requestListener` is a function which is
Copy link
Member

Choose a reason for hiding this comment

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

nit: 80-characters wrap?

@ameliavoncat ameliavoncat force-pushed the https-data-types branch 2 times, most recently from d0bee48 to ad4ef9c Compare March 5, 2017 04:24
@ameliavoncat
Copy link
Contributor Author

Changes made.

doc/api/http.md Outdated
- `path` {string} Default = `/`
- `headers` {Object}
- `auth` {string}
- `agent` {Agent | boolean} Default = `globalAgent`
Copy link
Member

Choose a reason for hiding this comment

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

http.globalAgent would be clearer

doc/api/https.md Outdated
- `path` {string} Default = `/`
- `headers` {Object}
- `auth` {string}
- `agent` {Agent | boolean} Default = `globalAgent`
Copy link
Member

Choose a reason for hiding this comment

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

https.globalAgent would be clearer(notice https.globalAgent !== http.globalAgent)

doc/api/https.md Outdated
<!-- YAML
added: v0.3.6
-->
- `options` {Object | string}
Copy link
Member

Choose a reason for hiding this comment

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

Can you merge the type signatures with option explanations below? Thanks!

@ameliavoncat
Copy link
Contributor Author

Updated. 😄

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Almost there!

doc/api/http.md Outdated
@@ -1528,7 +1529,20 @@ added to the [`'request'`][] event.
added: v0.3.6
-->

* `options` {Object}
* `options` {Object | string}
- `protocol` {string} Defaults to `http:`.
Copy link
Member

Choose a reason for hiding this comment

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

Elsewhere in the change you using the pattern Default = ....

Would you please update for consistency? Thank you! 😄

doc/api/https.md Outdated
happens at the connection level, *before* the HTTP request is sent. Default `true`.
- `secureProtocol` {string}
The SSL method to use, e.g. `SSLv3_method` to force SSL version 3. The
possible values depend on your installation of OpenSSL and are defined in the
Copy link
Member

Choose a reason for hiding this comment

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

Please replace your with the. (in generally we avoid use of pronouns such as you and your in the docs. Thank you!

@ameliavoncat
Copy link
Contributor Author

Changes made. You and your were used alot in http so I changed those as well.

@ameliavoncat ameliavoncat force-pushed the https-data-types branch 2 times, most recently from 72af375 to a2ac3ea Compare March 7, 2017 07:58
doc/api/http.md Outdated
@@ -1528,7 +1528,20 @@ added to the [`'request'`][] event.
added: v0.3.6
-->

* `options` {Object}
* `options` {Object | string}
Copy link
Member

Choose a reason for hiding this comment

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

Actually, I am not sure if the options need to be explained here since most of them are just passed to http.request, and that one has a more detailed explanation about these options? Maybe something like

* `options` {Object | string}: Takes the same values as `options` in 
  [`http.request(options[, callback])`][] except that `method` is set to `GET`

is enough? This way future updates to http.request don't need to take care of the explanations here.

doc/api/https.md Outdated
Returns a new HTTPS web server object. The `options` is similar to
[`tls.createServer()`][]. The `requestListener` is a function which is
automatically added to the `'request'` event.
- `options` {Object}
Copy link
Member

Choose a reason for hiding this comment

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

Same here, tls.createServer() and tls.createSecureContext() have a detailed explanation about these options, so linking to the docs of them should be enough. (Of course the signature of options itself is still necessary)

doc/api/https.md Outdated
<!-- YAML
added: v0.3.6
-->
- `options` {Object | string}
Copy link
Member

Choose a reason for hiding this comment

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

Ditto for http.get

doc/api/https.md Outdated
<!-- YAML
added: v0.3.6
-->
- `options` {Object | string}
Copy link
Member

Choose a reason for hiding this comment

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

Ditto for http.request, moduloagent, port, protocol differences

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM once @joyeecheung is happy with it also

@ameliavoncat ameliavoncat force-pushed the https-data-types branch 2 times, most recently from 5966723 to 3f63bd3 Compare March 7, 2017 22:14
@ameliavoncat
Copy link
Contributor Author

Updated. 😄

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Looking good, thanks!

@joyeecheung
Copy link
Member

jasnell pushed a commit that referenced this pull request Mar 8, 2017
Ref: #9399
PR-URL: #11681
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
jasnell pushed a commit that referenced this pull request Mar 8, 2017
Ref: #9399
PR-URL: #11681
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@jasnell
Copy link
Member

jasnell commented Mar 8, 2017

Landed in f6b0309...9772fb9
Thank you very much @ameliavoncat !

@jasnell jasnell closed this Mar 8, 2017
@italoacasas
Copy link
Contributor

This is not landing clearly in v7.x.

italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 13, 2017
Ref: nodejs#9399
PR-URL: nodejs#11681
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 14, 2017
Ref: nodejs#9399
PR-URL: nodejs#11681
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
Ref: nodejs#9399
PR-URL: nodejs#11681
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
Ref: nodejs#9399
PR-URL: nodejs#11681
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@MylesBorins
Copy link
Contributor

Are the changes relevant to v6.x? italoacasas@9c3cf13 cherry picks clean.

@MylesBorins
Copy link
Contributor

ping

Returns a new HTTPS web server object. The `options` is similar to
[`tls.createServer()`][]. The `requestListener` is a function which is
automatically added to the `'request'` event.
- `options` {Object} Accepts `options` from [`tls.createServer()`][] and [`tls.createSecureContext()`][].
Copy link
Member

Choose a reason for hiding this comment

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

long lines. can you please make sure to wrap all lines at <= 80 chars

Copy link
Member

Choose a reason for hiding this comment

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

HA! oops lol... totally missed that this had already landed lol :-D

@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks

Makes a request to a secure web server.

The following additional `options` from [`tls.connect()`][] are also accepted when using a
custom [`Agent`][]:
`pfx`, `key`, `passphrase`, `cert`, `ca`, `ciphers`, `rejectUnauthorized`, `secureProtocol`, `servername`
Copy link
Member

Choose a reason for hiding this comment

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

@joyeecheung does this section apply to v6.x as well?

@gibfahn
Copy link
Member

gibfahn commented Jun 17, 2017

Landed on v6.x-staging, LMK if that was a mistake.

gibfahn pushed a commit that referenced this pull request Jun 17, 2017
Ref: #9399
PR-URL: #11681
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
Ref: #9399
PR-URL: #11681
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
Ref: #9399
PR-URL: #11681
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants