check close callback is function #609
Conversation
|
I'd rather it fail for trying to use a non-function as a function than to fail silently. |
|
Another alternative would be to check for a function and throw if it is not one. Failing silently is a bad idea though. |
|
after having a quick cruise through lib I have to agree with @yosuke-furukawa that his proposal is idiomatic of what we have now in the networking code. There's a bunch of checks throwing My vote would be for letting this one through and someone having a second pass applying more rigour, particularly on the ones that are explicitly checking arguments named Then we'd need to discuss whether this would constitute a breaking change.. |
|
Looks good to me. |
|
|
||
| var assert = require('assert'), | ||
| common = require('../common'), | ||
| dgram = require('dgram'); |
There was a problem hiding this comment.
Minor nit: can you use a var (or let or const) declaration per line here?
|
Style nits but essentially LGTM. |
|
I fixed all comments, thank you Ben. |
|
@rvagg once this lands, I'll take a second pass at this and other code to find some places to introduce |
|
|
||
| var server = net.createServer(assert.fail); | ||
|
|
||
| server.on('error', function(error) { |
There was a problem hiding this comment.
Should probably let this error bubble up as well
|
I fixed all comments. please review again. |
| @@ -341,7 +341,7 @@ function afterSend(err) { | |||
|
|
|||
|
|
|||
| Socket.prototype.close = function(callback) { | |||
| if (callback) | |||
| if (util.isFunction(callback)) | |||
There was a problem hiding this comment.
can you change this to typeof check?
|
Fixed!! |
PR-URL: #609 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Evan Lucas <evanlucas@me.com>
PR-URL: #609 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Evan Lucas <evanlucas@me.com>
close methods have a callback. But the functions do not check the callback is a function.