{"action":"created","issue":{"url":"https://api.github.com/repos/nodejs/node/issues/4039","labels_url":"https://api.github.com/repos/nodejs/node/issues/4039/labels{/name}","comments_url":"https://api.github.com/repos/nodejs/node/issues/4039/comments","events_url":"https://api.github.com/repos/nodejs/node/issues/4039/events","html_url":"https://github.com/nodejs/node/pull/4039","id":119082689,"number":4039,"title":"net: refactor Server.prototype.listen","user":{"login":"jscissr","id":4520820,"avatar_url":"https://avatars.githubusercontent.com/u/4520820?v=3","gravatar_id":"","url":"https://api.github.com/users/jscissr","html_url":"https://github.com/jscissr","followers_url":"https://api.github.com/users/jscissr/followers","following_url":"https://api.github.com/users/jscissr/following{/other_user}","gists_url":"https://api.github.com/users/jscissr/gists{/gist_id}","starred_url":"https://api.github.com/users/jscissr/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jscissr/subscriptions","organizations_url":"https://api.github.com/users/jscissr/orgs","repos_url":"https://api.github.com/users/jscissr/repos","events_url":"https://api.github.com/users/jscissr/events{/privacy}","received_events_url":"https://api.github.com/users/jscissr/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/nodejs/node/labels/net","name":"net","color":"eb6420"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":7,"created_at":"2015-11-26T17:23:49Z","updated_at":"2015-12-02T18:11:30Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/nodejs/node/pulls/4039","html_url":"https://github.com/nodejs/node/pull/4039","diff_url":"https://github.com/nodejs/node/pull/4039.diff","patch_url":"https://github.com/nodejs/node/pull/4039.patch"},"body":"This commit simplifies `Server.prototype.listen`, removing some redundancy and inconsistency.\r\n\r\nBecause `listen` and `connect` have a similar function signature, `normalizeConnectArgs` can be reused for `listen`.\r\n\r\n`isLegalPort` is used in both `connect` and `listen`, and both had the same additional checks.\r\n`isLegalPort` and the additional checks were moved to `portToNumber`.\r\n\r\n`addressType` in `listenAfterLookup` is already guaranteed to be `4` if address `ip` is the empty string.\r\n\r\n`listenAfterLookup` renamed to `lookupAndListen` for consistency with `lookupAndConnect`, and moved out of `listen`.\r\n\r\n## Functional differences\r\n\r\nTo improve consistency, I actually made some small functional changes:\r\n\r\n- `port` was checked with this complicated code if set in a config object:\r\n  ```\r\n  if (typeof h.port === 'number' || typeof h.port === 'string' ||\r\n      (typeof h.port === 'undefined' && 'port' in h)) {\r\n    // Undefined is interpreted as zero (random port) for consistency\r\n    // with net.connect().\r\n    if (typeof h.port !== 'undefined' && !isLegalPort(h.port))\r\n      throw new RangeError('\"port\" option should be >= 0 and < 65536: ' +\r\n                           h.port);\r\n    …\r\n  ```\r\n  But if set as first argument, it was only coerced to a number with `var port = toNumber(arguments[0]);`.\r\n  Now, it is consistently using the first way, `toNumber` is not used anymore.\r\n \r\n  However in special cases this could be considered a breaking change, as before invalid ports resulted in using a random port instead of throwing.\r\n\r\n- `addressType` was sometimes `null` and sometimes `4`, if no address is given.\r\n  Now it is always `null`.\r\n\r\n- `host` was checked like this:\r\n  ```\r\n  if (arguments[1] !== undefined &&\r\n      typeof arguments[1] !== 'function' &&\r\n      typeof arguments[1] !== 'number')\r\n  ```\r\n  Now it is checked with `if (typeof arguments[1] === 'string')`, this is consistent with `connect`."},"comment":{"url":"https://api.github.com/repos/nodejs/node/issues/comments/161385746","html_url":"https://github.com/nodejs/node/pull/4039#issuecomment-161385746","issue_url":"https://api.github.com/repos/nodejs/node/issues/4039","id":161385746,"user":{"login":"Trott","id":718899,"avatar_url":"https://avatars.githubusercontent.com/u/718899?v=3","gravatar_id":"","url":"https://api.github.com/users/Trott","html_url":"https://github.com/Trott","followers_url":"https://api.github.com/users/Trott/followers","following_url":"https://api.github.com/users/Trott/following{/other_user}","gists_url":"https://api.github.com/users/Trott/gists{/gist_id}","starred_url":"https://api.github.com/users/Trott/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Trott/subscriptions","organizations_url":"https://api.github.com/users/Trott/orgs","repos_url":"https://api.github.com/users/Trott/repos","events_url":"https://api.github.com/users/Trott/events{/privacy}","received_events_url":"https://api.github.com/users/Trott/received_events","type":"User","site_admin":false},"created_at":"2015-12-02T18:11:30Z","updated_at":"2015-12-02T18:11:30Z","body":"The `isLegalPort()` function that you wish to change appears to have been authored by @bnoordhuis in https://github.com/nodejs/node/commit/480b48244 in order to deal with an API inconsistency. Since that also is the motivation for your proposed change, perhaps he can offer an opinion as to the risk and value of the modifications here."}}