Emit dns error thrown by native code #115
Conversation
|
Thanks! Reviewing... |
| self.emit('error', e) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
Please use semicolons after statements.
|
I finished my review. Still not sure about proxyquire, though. What it does is quite neat, no question. But it sounds like it is quite a messy business. I will probably give that a glance too. Do you have long-term experience in using it? How stable is it (across different node versions)? |
|
Sorry for my delay in replying - I've somehow managed to rent a holiday apartment without WiFi. Schoolboy error. Sorry also for the semi-colon thing - I'd mean to push the subsequent commit to a branch instead of master so didn't mean those changes to be part of this pull request. They do appear to be valid though - when using node-mdns on really flaky WiFi networks I noticed service names would occasionally come back as an empty string. I've added the extra punctuation. As for the publishing to npm/using a git url/tarball - I've been deploying in corporate environments with silly content filter policies so sometimes github is blocked but npm isn't. Obviously in this case using github URLs won't work so it's easier to just publish the fork under a different name then delete it once pull requests are merged to the upstream version and new versions are released. Anyway I've rebased that commit out of the repo history. Proxyquire - I've used it quite a bit. You are right it does some horrible things to That said I'd rather see some sort of dependency injection so it's not necessary but that would turn into quite the rewrite. |
No worries. The patch looks good to me... after resolving my confusion about proxyquire. ;) @ronkorving, care to take a second look and release the fix? |
|
Looks good to me, thanks :) |
Emit dns error thrown by native code
|
Released |
The native
dns_sd.DNSServicePRocessResultmethod can throw an error if there's a DNS failure, resulting in an uncaught exception and your application going down, so this pull request wraps it in a try/catch block and gets theBrowserobject (extendsMDNSService) to emit an error event instead.Also includes a unit test. Actually the
test_functional/simple browsingtest is failing at the moment but I figure better fixed in a different commit.