net: move isLegalPort to internal/net #4882
Conversation
|
LGTM |
| assert.strictEqual(net.isLegalPort(65536), false); | ||
| assert.strictEqual(net.isLegalPort('65535'), true); | ||
| assert.strictEqual(net.isLegalPort(undefined), false); | ||
| assert.strictEqual(net.isLegalPort(null), true); |
There was a problem hiding this comment.
The difference between undefined and null seems a bit incongruous in retrospect.
There was a problem hiding this comment.
I agree. Would it be worth it to move the null/undefined checks to this function? They are currently done in net in both places where it is being used. Seems like we could simplify some logic by doing so
There was a problem hiding this comment.
@bnoordhuis is that something you would prefer I do here or in another PR?
There was a problem hiding this comment.
A separate commit in this PR is fine.
There was a problem hiding this comment.
It appears to be a bit more than I expected. Mind if I wait until another PR?
|
LGTM |
| @@ -0,0 +1,11 @@ | |||
| 'use strict'; | |||
There was a problem hiding this comment.
Is a new module file necessary? This seems small enough that it could possibly just go into internal/util
There was a problem hiding this comment.
@jasnell While it is small enough now for that, I have been working on cleaning up some of the logic in net and moving some more into internal. Either way works for me though, so if this is something you feel strongly about, I'll be happy to change.
There was a problem hiding this comment.
@evanlucas ... :-) then by all means, continue as you are!
|
Minor nit but overall LGTM |
| function isLegalPort(port) { | ||
| if (typeof port === 'string' && port.trim() === '') | ||
| return false; | ||
| return +port === (port >>> 0) && port >= 0 && port <= 0xFFFF; |
There was a problem hiding this comment.
Would it make sense to use Number.isInteger here?
There was a problem hiding this comment.
The problem with parseInt() is that something like '10c' becomes 10, which is not right. The problem with Number.isInteger() is that '0xff' (or any other string) is false. There are all kinds of weird edge cases.
There was a problem hiding this comment.
True. Should we really allow values like that? Coercion allows people to use a lot of undocumented allowed values like this and I am not sure if we can cover all the possible valid values without a lot of checks. For example this condition would allow zero to one element number arrays.
There was a problem hiding this comment.
My personal opinion - no we shouldn't coerce. But, it would be an unnecessary breaking change. Also, this function is used for range checking, not type checking.
|
LGTM |
isLegalPort can be used in more places than just net.js. This change moves it to a new internal net module in preparation for using it in the dns module. PR-URL: nodejs#4882 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
evanlucas
force-pushed
the
dnsnet-internal
branch
from
8f0ed03 to
6cbbfef
Compare
|
Landed in 6cbbfef. Thanks |
isLegalPort can be used in more places than just net.js. This change moves it to a new internal net module in preparation for using it in the dns module. PR-URL: #4882 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
isLegalPort can be used in more places than just net.js. This change moves it to a new internal net module in preparation for using it in the dns module. PR-URL: #4882 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
isLegalPort can be used in more places than just net.js. This change moves it to a new internal net module in preparation for using it in the dns module. PR-URL: #4882 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
isLegalPort can be used in more places than just net.js. This change moves it to a new internal net module in preparation for using it in the dns module. PR-URL: #4882 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
isLegalPort can be used in more places than just net.js. This change moves it to a new internal net module in preparation for using it in the dns module. PR-URL: nodejs#4882 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
isLegalPort can be used in more places than just net.js. This change
moves it to a new internal net module in preparation for using it in
the dns module.