net: support passing a host with port to Server.prototype.listen

[Zirak]

Distinguish between a host and hostname in the Server.prototype.listen argument, to better align with how browsers and other places in node distinguish between host and hostname.

For more context, see #16712.

This commit:

• Broadens the meaning of the host key to also potentially include a port (i.e. hostname:port)
  • Given such a host option, one can choose to not supply a port parameter
• Allows the user to pass in a hostname key, which behaves just like host in present-day node.

The following three are now equivalent:

server.listen({
  host: 'localhost:5000',
})

server.listen({
  host: 'localhost',
  port: 5000,
})

server.listen({
  hostname: 'localhost',
  port: 5000
})

server.listen(new URL('http://localhost:5000'))

Backwards-compatibility

An important aspect was keeping this change backwards-compatible. As such, the host option is parsed into its hostname and port components.

In case of a conflict between a host component and one of the other options, an exception is thrown:

server.listen({
  host: 'localhost:5000',
  hostname: '127.0.0.1'
}) // => TypeError

server.listen({
  host: 'localhost:5000',
  port: 8080
}) // => TypeError

Caveats

My goal here was to mock-up a PoC which implements the spirit of the proposal in a backwards-compatible matter. Below are some caveats with how I wrote the code and some tradeoffs I took to keep this a PoC. If this proves to be interesting to the project, they'll be fixed, no worries :)

• Right now, host is parsed using a hack around new URL. It's probably not the smartest way of going about this. Regard it as temporary until this gets more traction.

• Similarly, the new errors thrown here should be internal and worded better overall. So far they serve as an example.

• Since ipv6 addresses in urls must be enclosed in square brackets (i.e. [80:fe::]), and the rest of the code expects them to be free outside their cages, we need to trim those. To maintain current behaviour, we need to only trim those if the value inside is an ipv6 address. Current node:

net.createServer().listen({ host: '[foo].com', port: 0 });

After a tick throws Error: getaddrinfo ENOTFOUND [foo].com. Having said that, new URL('http://[foo].com') throws an exception, and Firefox doesn't believes it's a real URL.

• This pollutes the options variable with an extra hostname key the user might not have specified. The options variable gets printed in some cases, and this may not be ideal.

  • The bespoken error message need also be updated.

• Right now, a known bug is when you pass a host containing a port and you also pass the port key as a string (i.e. listen({ host: 'whatever:123', port: '123' })), an error will be thrown about mismatching port values. One potential fix is moving the casting up, from the argument of lookupAndListen or listenInCluster to somewhere handling the arguments.

• The tests are fairly verbose and can be compacted. I opted to making them very explicit and long-form at this point for clarity.

• There are currently no tests for the lonely hostname option.

node questions

• Is there a reason to try and identify invalid IPv6 addresses (like 1::2::3 or :::)? Currently, they fall back on trying to be resolved via dns and failing.

• Did I put the tests in the correct file? Is there a better place for it? e.g. test-net-server-address?

---

Sorry for the extra long PR comment, and thank you for your time.

Checklist

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

[benjamingr]

This looks reasonable to me - @nodejs/http @nodejs/collaborators can this get some opinions/reviews?

[BridgeAR]

@Zirak is it intentional that this is kept as draft?

[Zirak]

@Zirak is it intentional that this is kept as draft?

So far I haven't seen any team-member approval, or that this PR is of any interest. If there is any, and the concerns in the main post are no problem, then I'll gladly pick it back up.

[bnb]

@nodejs/http any chance we can get reviews on this? ❤️