net: throw error if port does not exist in options #22085

yanivfriedensohn · 2018-08-02T11:13:21Z

Throw error ERR_PROPERTY_NOT_IN_OBJECT if the port property does not
exist in the options object argument when calling server.listen().

Refs: #16712

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

#SimilarWeb_RnD

jasnell · 2018-08-03T22:26:47Z

Semver-major because of the addition of a new throw.

@nodejs/tsc ... PTAL. I'm not yet convinced this is definitely something we should do.

Trott · 2018-08-04T03:29:35Z

Hi, @yanivfriedensohn and welcome! Thanks for the PR.

I think the added error is not quite right. If I understand correctly, port is not required. Rather, one of either port or path is required. Might it not be better to improve the message displayed for the existing ERR_INVALID_OPT_VALUE error to indicate that rather than add a new error?

yanivfriedensohn · 2018-08-04T17:10:28Z

Hi @Trott, based on your suggestions, I added an additional error message argument to ERR_INVALID_OPT_VALUE and removed ERR_PROPERTY_NOT_IN_OBJECT.

maclover7 · 2018-08-11T20:21:40Z

@yanivfriedensohn Would you be able to add some tests for the new behavior? We can run CI on this after that

joyeecheung · 2018-08-12T19:28:14Z

How does this fix #16712 ? It requested

APIs that accept a host should respect their port number to avoid confusion and inconsistency. I would also suggest that server.listen() should only support hostname instead of host, leaving parsing hosts to userland... but that would be a breaking change (and should probably be a separate issue).

joyeecheung · 2018-08-12T19:32:50Z

Note that server.listen() works like server.listen(0) so to say that port or path must be specified is not entirely accurate..

yanivfriedensohn · 2018-08-13T21:58:30Z

@maclover7 I added tests for the additional error message.

@joyeecheung this fixes:

The { host : 'localhost' } error message would be good to fix but the other "expected behaviors" could potentially create (possibly severe) ecosystem fallout for not enough value to take the risk.

The message mentions that port or path are required in options. Should I rephrase the message to make it more clear?

joyeecheung · 2018-08-13T22:27:59Z

@yanivfriedensohn In that case please do not put Fixes in the PR/commit message as landing it would close that issue automatically even though it's not fully fixed.

joyeecheung · 2018-08-13T22:28:30Z

lib/net.js

@@ -1496,7 +1496,15 @@ Server.prototype.listen = function(...args) {
    return this;
  }

-  throw new ERR_INVALID_OPT_VALUE('options', util.inspect(options));
+  let additionalMessage;
+  if (typeof options === 'object' &&


typeof options === 'object' here is always true since it's normalized before

joyeecheung · 2018-08-13T22:30:19Z

lib/net.js

+    additionalMessage = '"port" or "path" must be specified in options.';
+  }
+
+  throw new ERR_INVALID_OPT_VALUE('options', util.inspect(options),


This error message will not be accurate since options is an argument, not an option value - the user does not pass { options: { ... } } to this method. ERR_INVALID_ARG_VALUE may be more accurate.

yanivfriedensohn · 2018-08-15T19:02:05Z

@joyeecheung following your suggestion, I used ERR_INVALID_ARG_VALUE instead of ERR_INVALID_OPT_VALUE and reverted the change to ERR_INVALID_OPT_VALUE.

Trott · 2018-08-15T20:51:03Z

Tests have been added so I've removed the in progress label. /cc @maclover7 Feel free to add it back if it still applies.

yanivfriedensohn · 2018-08-21T18:38:59Z

@joyeecheung is this ready for CI?

joyeecheung · 2018-08-21T22:18:56Z

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

Throw error ERR_PROPERTY_NOT_IN_OBJECT if the port property does not exist in the options object argument when calling server.listen(). Refs: nodejs#16712

…ALUE

…ALID_OPT_VALUE

Trott · 2018-08-22T04:20:47Z

Rebased and force pushed to resolve the dreaded jinja LICENSE CRLF thing. (I think we'll all be happy when that problem is a few weeks in the past.)

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

yanivfriedensohn · 2018-08-22T19:51:13Z

@Trott issue #18435 seems to be related to the failed test.

Trott · 2018-08-23T22:05:13Z

Resume Build: https://ci.nodejs.org/job/node-test-pull-request/16705/

yanivfriedensohn · 2018-08-31T19:57:12Z

@Trott is this author ready?

mcollina

LGTM

addaleax · 2018-09-02T11:23:28Z

Landed in 4a0466f 🎉

Throw error ERR_INVALID_ARG_VALUE if the port and path properties do not exist in the options object argument when calling server.listen(). Refs: #16712 PR-URL: #22085 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. net Issues and PRs related to the net subsystem. labels Aug 2, 2018

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 3, 2018

yanivfriedensohn force-pushed the throw-error-if-port-does-not-exist-in-options branch from c75c291 to dc961c8 Compare August 4, 2018 19:56

maclover7 added the wip Issues and PRs that are still a work in progress. label Aug 11, 2018

yanivfriedensohn force-pushed the throw-error-if-port-does-not-exist-in-options branch 2 times, most recently from 8bed77c to 7498e3c Compare August 13, 2018 21:36

joyeecheung reviewed Aug 13, 2018

View reviewed changes

yanivfriedensohn force-pushed the throw-error-if-port-does-not-exist-in-options branch from 7498e3c to 4862c1f Compare August 15, 2018 18:35

joyeecheung approved these changes Aug 15, 2018

View reviewed changes

Trott removed the wip Issues and PRs that are still a work in progress. label Aug 15, 2018

BridgeAR approved these changes Aug 21, 2018

View reviewed changes

Trott added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Aug 22, 2018

yanivfriedensohn added 4 commits August 21, 2018 21:19

net: throw error if port does not exist in options

6a90ec5

Throw error ERR_PROPERTY_NOT_IN_OBJECT if the port property does not exist in the options object argument when calling server.listen(). Refs: nodejs#16712

errors: add an additional error message argument to ERR_INVALID_OPT_V…

0c0fbe4

…ALUE

net: use ERR_INVALID_OPT_VALUE instead of ERR_PROPERTY_NOT_IN_OBJECT

54fc80f

errors: remove ERR_PROPERTY_NOT_IN_OBJECT

daae72b

yanivfriedensohn added 3 commits August 21, 2018 21:19

test: add tests for the additional error message

20b525e

errors: revert adding an additional error message argument to ERR_INV…

7be75d8

…ALID_OPT_VALUE

net: use ERR_INVALID_ARG_VALUE instead of ERR_INVALID_OPT_VALUE

630d987

Trott force-pushed the throw-error-if-port-does-not-exist-in-options branch from 4862c1f to 630d987 Compare August 22, 2018 04:19

BridgeAR requested a review from a team August 31, 2018 20:09

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 31, 2018

mcollina approved these changes Aug 31, 2018

View reviewed changes

addaleax closed this Sep 2, 2018

yanivfriedensohn deleted the throw-error-if-port-does-not-exist-in-options branch June 21, 2019 10:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: throw error if port does not exist in options #22085

net: throw error if port does not exist in options #22085

yanivfriedensohn commented Aug 2, 2018 •

edited

Loading

jasnell commented Aug 3, 2018

Trott commented Aug 4, 2018 •

edited

Loading

yanivfriedensohn commented Aug 4, 2018

maclover7 commented Aug 11, 2018

joyeecheung commented Aug 12, 2018

joyeecheung commented Aug 12, 2018

yanivfriedensohn commented Aug 13, 2018

joyeecheung commented Aug 13, 2018

joyeecheung Aug 13, 2018

joyeecheung Aug 13, 2018

yanivfriedensohn commented Aug 15, 2018

Trott commented Aug 15, 2018

yanivfriedensohn commented Aug 21, 2018

joyeecheung commented Aug 21, 2018

Trott commented Aug 22, 2018

yanivfriedensohn commented Aug 22, 2018

Trott commented Aug 23, 2018

yanivfriedensohn commented Aug 31, 2018

mcollina left a comment

addaleax commented Sep 2, 2018

net: throw error if port does not exist in options #22085

net: throw error if port does not exist in options #22085

Conversation

yanivfriedensohn commented Aug 2, 2018 • edited Loading

Checklist

jasnell commented Aug 3, 2018

Trott commented Aug 4, 2018 • edited Loading

yanivfriedensohn commented Aug 4, 2018

maclover7 commented Aug 11, 2018

joyeecheung commented Aug 12, 2018

joyeecheung commented Aug 12, 2018

yanivfriedensohn commented Aug 13, 2018

joyeecheung commented Aug 13, 2018

joyeecheung Aug 13, 2018

Choose a reason for hiding this comment

joyeecheung Aug 13, 2018

Choose a reason for hiding this comment

yanivfriedensohn commented Aug 15, 2018

Trott commented Aug 15, 2018

yanivfriedensohn commented Aug 21, 2018

joyeecheung commented Aug 21, 2018

Trott commented Aug 22, 2018

yanivfriedensohn commented Aug 22, 2018

Trott commented Aug 23, 2018

yanivfriedensohn commented Aug 31, 2018

mcollina left a comment

Choose a reason for hiding this comment

addaleax commented Sep 2, 2018

yanivfriedensohn commented Aug 2, 2018 •

edited

Loading

Trott commented Aug 4, 2018 •

edited

Loading