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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Feb 27, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5747d9f
http: add default argument for Agent.prototype.getName
xtx1130 Feb 9, 2022
f513e4a
doc: change `agent.getName`'s argumnet to be optional
xtx1130 Feb 10, 2022
32e574b
doc: Update doc/api/http.md
xtx1130 Feb 11, 2022
69a6bfb
doc: Update doc/api/http.md
xtx1130 Feb 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion doc/api/http.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,10 +296,14 @@ the agent when `keepAlive` is enabled. Do not modify.
Sockets in the `freeSockets` list will be automatically destroyed and
removed from the array on `'timeout'`.

### `agent.getName(options)`
### `agent.getName([options])`

<!-- YAML
added: v0.11.4
xtx1130 marked this conversation as resolved.
Show resolved Hide resolved
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/41906
description: The `options` parameter is now optional.
-->

* `options` {Object} A set of options providing information for name generation
Expand Down
2 changes: 1 addition & 1 deletion lib/_http_agent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ Agent.defaultMaxSockets = Infinity;
Agent.prototype.createConnection = net.createConnection;

// Get the key for a given set of request options
Agent.prototype.getName = function getName(options) {
Agent.prototype.getName = function getName(options = {}) {
let name = options.host || 'localhost';
Comment on lines +220 to 221
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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. :)


name += ':';
Expand Down
2 changes: 1 addition & 1 deletion lib/https.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ Agent.prototype.createConnection = createConnection;
* }} [options]
* @returns {string}
*/
Agent.prototype.getName = function getName(options) {
Agent.prototype.getName = function getName(options = {}) {
let name = FunctionPrototypeCall(HttpAgent.prototype.getName, this, options);

name += ':';
Expand Down
8 changes: 7 additions & 1 deletion test/parallel/test-http-agent-getname.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,13 @@ assert.strictEqual(
'localhost:80:192.168.1.1'
);

// empty
// empty argument
assert.strictEqual(
agent.getName(),
'localhost::'
);

// empty options
assert.strictEqual(
agent.getName({}),
'localhost::'
Expand Down
6 changes: 6 additions & 0 deletions test/parallel/test-https-agent-getname.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@ const https = require('https');

const agent = new https.Agent();

// empty argument
assert.strictEqual(
agent.getName(),
'localhost::::::::::::::::::::::'
);

// empty options
assert.strictEqual(
agent.getName({}),
Expand Down