Uh oh!
There was an error while loading. Please reload this page.
http: add type checking for hostname - #12494
Conversation
There was a problem hiding this comment.
The first two conditionals can be shortened to just host != null or host != undefined.
There was a problem hiding this comment.
Yeah... that's fine. I'm not a fan of != for that but shorter is better.
There was a problem hiding this comment.
It could be more efficient too, I have not checked how V8 compiles the two different methods of checking.
There was a problem hiding this comment.
Perhaps we might use: "hostname"/"host" must either be a string, undefined, or null to keep with the theme of quoting property names?
jasnell
commented
Apr 18, 2017
@mscdex Updated |
There was a problem hiding this comment.
Why the common.noop here? I'd think we'd either want mustCall() or mustNotCall().
There was a problem hiding this comment.
The main part that this is testing is that the call to http.request() does not throw synchronously. The fact that it will emit the error event is non-sequential to this particular test. This is set simply to avoid the unhandled error exception.
There was a problem hiding this comment.
Should we include "host" as well, since this same function is used for both?
There was a problem hiding this comment.
I was stewing over that. Not sure. I'm fine with doing so just not sure if we should
There was a problem hiding this comment.
If I'm not wrong this can be removed if host != null is used instead of host != undefined.
97f9dc6 to
46c75f1Comparejasnell
commented
Apr 19, 2017
jasnell
commented
Apr 20, 2017
@nodejs/ctc ... PTAL |
There was a problem hiding this comment.
Maybe include 0 as well, since it would have been treated as a false-y value?
There was a problem hiding this comment.
This function is not doing any checks actually
There was a problem hiding this comment.
Why not common.mustNotCall() here?
There was a problem hiding this comment.
@jasnell's reply on a comment above for same question
The main part that this is testing is that the call to http.request() does not throw synchronously. The fact that it will emit the error event is non-sequential to this particular test. This is set simply to avoid the unhandled error exception.
There was a problem hiding this comment.
Mmmm, makes sense. Cool then.
46c75f1 to
0b5dc3fCompareAdd type checking for options.hostname / options.host Maintains the use of `undefined` and `null` for default `localhost`, but other falsy values (like `false`, `0` and `NaN`) are rejected.
0b5dc3f to
9ec4133Comparejasnell
commented
Apr 24, 2017
jasnell
commented
Apr 24, 2017
Linting error was from an unrelated commit |
Add type checking for options.hostname / options.host Maintains the use of `undefined` and `null` for default `localhost`, but other falsy values (like `false`, `0` and `NaN`) are rejected. PR-URL: #12494 Ref: #12488 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
jasnell
commented
Apr 24, 2017
Landed in 85a4e25 |
This test would currently create HTTP requests to localhost:80 and would time out on machines that actually had an server listening there. To address that, `end()` the requests that are generated. PR-URL: nodejs#12627 Ref: nodejs#12494
This test would currently create HTTP requests to localhost:80 and would time out on machines that actually had an server listening there. To address that, `end()` the requests that are generated. PR-URL: #12627 Ref: #12494 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Add type checking for options.hostname / options.host Maintains the use of
undefinedandnullfor defaultlocalhost, but other falsy values (likefalse,0andNaN) are rejected.Refs: #12488
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
http