net: throw error if port does not exist in options

[yanivfriedensohn]

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]

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]

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]

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]

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

[joyeecheung]

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]

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

[yanivfriedensohn]

@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]

@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]

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

[joyeecheung]

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]

@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]

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]

@joyeecheung is this ready for CI?

[joyeecheung]

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

[Trott]

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]

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

[Trott]

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

[yanivfriedensohn]

@Trott is this author ready?

[mcollina]

LGTM

[addaleax]

Landed in 4a0466f 🎉