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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: Options passed needlessly to https request object and agent prop #35360

Open
netanel-haber opened this issue Sep 26, 2020 · 5 comments
Open
Labels
doc Issues and PRs related to the documentations. https Issues or PRs related to the https subsystem.

Comments

@netanel-haber
Copy link

馃摋 API Reference Docs Problem

  • Version: v14.12.0

Location

Affected URL(s):

Description

In the following example provided for the https.request method, the request config object is passed both to the top level https request object, and to the agent property of said object.
This seems to either be a typo, or unclear. I would expect to see the following:

  • "key" and "cert" should be provided to the agent object only, and should not appear at the top level of the options object.
  • The properties that comprise the URL object ("hostname", "port", "path" and "method") should not be provided to the agent object.

I think this would avoid confusion.

const options = {
  hostname: 'encrypted.google.com',
  port: 443,
  path: '/',
  method: 'GET',
  key: fs.readFileSync('test/fixtures/keys/agent2-key.pem'),
  cert: fs.readFileSync('test/fixtures/keys/agent2-cert.pem')
};
options.agent = new https.Agent(options);

const req = https.request(options, (res) => {
  // ...
});
@netanel-haber netanel-haber added the doc Issues and PRs related to the documentations. label Sep 26, 2020
@netanel-haber netanel-haber changed the title doc: Options passed to https request object and agent property doc: Options passed needlessly to https request object and agent prop Sep 26, 2020
@dev-script
Copy link
Contributor

Hi @netanel-haber , I would like to work on this issue.

@netanel-haber
Copy link
Author

The question is, if it's really an issue at all or just my misunderstanding/a general styling decision that was made for the docs.

@lpinca lpinca added the https Issues or PRs related to the https subsystem. label Oct 26, 2020
@lpinca
Copy link
Member

lpinca commented Oct 26, 2020

@netanel-haber I think this was done for simplicity to avoid creating two options objects. Feel free to open a PR if you think that using two options objects clarifies the example. I personally don't mind.

@netanel-haber
Copy link
Author

Thanks for the reply - Where can I find the format for posting prs to this repo?

@lpinca
Copy link
Member

lpinca commented Oct 26, 2020

See https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md

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. https Issues or PRs related to the https subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants