http,https: add default argument for Agent.prototype.getName

[xtx1130]

http,https: add default argument for Agent.prototype.getName

const https = require('https')
let agent = new https.Agent()
console.log(agent.getName())

Before:

node:_http_agent:221
  let name = options.host || 'localhost';
                     ^

TypeError: Cannot read properties of undefined (reading 'host')
    at Agent.getName (node:_http_agent:221:22)
    at Agent.getName (node:https:207:14)
    at Object.<anonymous> (/Users/xtx/Desktop/leetcode/test.js:5:19)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:17:47

after:

localhost::::::::::::::::::::::

[nodejs-github-bot]

Review requested:

• @nodejs/crypto
• @nodejs/http
• @nodejs/net

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/42453/

[aduh95]

I'd prefer if instead of creating a throw away object, we used optional chaining and set/keep the default value to undefined.

Suggested change

	Agent.prototype.getName = function getName(options = {}) {
	let name = options.host || 'localhost';
	Agent.prototype.getName = function getName(options = undefined) {
	let name = options?.host || 'localhost';

[xtx1130]

The option has many attributes, such as port,localAddress, family and so on. It will be more simplify to create a default object than make every attributes with ?.

[aduh95]

It will be more simplify to create a default object than make every attributes with ?.

Depends what you mean by simplify, it's a smaller change in term of source code changes, but it's not simpler for end users. Using a throw away objects also has side effects which could be surprising for them:

// In userland:
Object.prototype.host = 'something unrelated';

// In core:
function getName(options = {}) { return options.host || 'localhost'; }
console.log(getName()); // 'something unrelated'

// In userland:
Object.prototype.host = 'something unrelated';

// In core:
function getName(options = undefined) { return options?.host || 'localhost'; }
console.log(getName()); // 'localhost'

My opinion is that Node.js core modules should use optional chaining over creating a throw away objects, it's not a blocking concern by any mean though. Feel free to disagree, the PR is fine as is.

[xtx1130]

Thanks for your detailed explanation. But I have seen many default arguments in nodejs is using empty object, I think we can draft another PR to fix it, if it is necessary. :)

[lpinca]

No strong opinion but the options object is not documented as optional https://nodejs.org/api/http.html#agentgetnameoptions. We should eventually also update the documentation.

[xtx1130]

@lpinca Thanks for your remind. I have changed the options argument in the documentation to be optional.

[rickyes]

LGTM

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/42476/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/42499/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/42503/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/42516/

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/41906
✔  Done loading data for nodejs/node/pull/41906
----------------------------------- PR info ------------------------------------
Title      http,https: add default argument for Agent.prototype.getName (#41906)
Author     小菜  (@xtx1130, first-time contributor)
Branch     xtx1130:fix-_http_agent-getName -> nodejs:master
Labels     http, https, author ready, commit-queue-squash
Commits    4
 - http: add default argument for Agent.prototype.getName
 - doc: change `agent.getName`'s argumnet to be optional
 - doc: Update doc/api/http.md
 - doc: Update doc/api/http.md
Committers 2
 - SindreXie 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/41906
Reviewed-By: Matteo Collina 
Reviewed-By: Ricky Zhou <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41906
Reviewed-By: Matteo Collina 
Reviewed-By: Ricky Zhou <[email protected]>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - doc: Update doc/api/http.md
   ℹ  This PR was created on Wed, 09 Feb 2022 08:49:26 GMT
   ✔  Approvals: 2
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/41906#pullrequestreview-877174171
   ✔  - Ricky Zhou (@rickyes): https://github.com/nodejs/node/pull/41906#pullrequestreview-879743293
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-02-13T08:29:46Z: https://ci.nodejs.org/job/node-test-pull-request/42516/
- Querying data for job/node-test-pull-request/42516/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/1906354504

[nodejs-github-bot]

Landed in 04c68ba