lib: allow server.listen({ port: "1234" }) #1116
Conversation
| // is an integer and that it falls within the legal range of port numbers. | ||
| function isLegalPort(port) { | ||
| if (typeof port === 'string' && port.trim() === '') | ||
| return false; |
There was a problem hiding this comment.
@cjihrig What was the motivation for disallowing an empty string?
There was a problem hiding this comment.
Empty string and strings of 1 or more spaces become 0 when the + operator is applied. Since we want to allow zero, I had to specifically check for that case.
There was a problem hiding this comment.
Okay, that much I get. It's not really clear to me though why undefined is allowed (and interpreted as 0) but not a blank string.
There was a problem hiding this comment.
@misterdjules and I had a fair amount of discussion about this. undefined is allowed because the argument is optional and needs a default value. The empty string means you are explicitly passing in an "incorrect" value. I actually wasn't the one to add the original checks. However, I moved the validation, so I sort of took ownership of it when the bug (port zero was no longer allowed) was reported against 0.12.
There was a problem hiding this comment.
Okay, thanks for the explanation. It feels kind of quirky but I don't have a strong enough opinion to file a PR for it. :-)
|
LGTM. The CI appears to be happyish |
net.connect() accepts `{ port: "1234" }` (i.e. a string) as of commit
9d2b89d ("net: allow port 0 in connect()") but net.Server#listen()
did not, creating a minor inconsistency. This commit rectifies that.
Fixes: nodejs#1111
PR-URL: nodejs#1116
Reviewed-By: Colin Ihrig <[email protected]>
|
This is semver-minor right? Perhaps we should let these PRs simmer a little longer before landing. |
|
I guess it is but I already landed it. Want me to revert it for now? |
|
No, just making a point that when changes are non-trivial we should probably be allowing time for dissent and discussion. I'm guessing this one is not too controversial. |
Fishrock123
added
the
semver-minor
|
I'd like to do a release but this would justify a 1.6.0 which is kind of unfortunate because that's a pathetic minor version bump so I'd like to wait a few days to see if we can get something out of one of the other semver-minor PRs in flux atm. We could also have a debate about whether this really is semver-minor.. |
|
One could argue that |
net.connect() accepts
{ port: "1234" }(i.e. a string) as of commit9d2b89d ("net: allow port 0 in connect()") but net.Server#listen()
did not, creating a minor inconsistency. This commit rectifies that.
Fixes: #1111
R=@cjihrig
https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/270/