search

danielzzz / node-ping

a poor man's ping library (using udp scanning) for node
MIT License
336 stars 105 forks source link

no warning when ping is not installed #160

Closed sunny-Ne5 closed 1 year ago

sunny-Ne5 commented 1 year ago

I was using node-ping inside a docker container which doesn't had iputils-ping installed. ping.sys.probe(req.params.ip, (isUp) => { res.end(JSON.stringify({status: isUp ? "true" : "false"})); }); isUp was always coming as null with no warning whatsoever that ping is not installed in the system. However, installing iputils-ping inside container fixed the issue.

Should we throw warning for this use case? I'll be happy to add the fix if given some guidance.

mondwan commented 1 year ago

Hi,

Have you tried the promise base response? I suppose it should fit your need

On Sun, 19 Mar 2023, 19:36 SUNNY TIWARI, @.***> wrote:

I was using node-ping inside a docker container which doesn't had iputils-ping installed. ping.sys.probe(req.params.ip, (isUp) => { res.end(JSON.stringify({status: isUp ? "true" : "false"})); }); isUp was always coming as null with no warning whatsoever that ping is not installed in the system. However, installing iputils-ping inside container fixed the issue.

Should we throw warning for this use case? I'll be happy to add the fix if given some guidance.

— Reply to this email directly, view it on GitHub https://github.com/danielzzz/node-ping/issues/160, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA5YBI7PRC6UGIKA44UKIFLW45N4FANCNFSM6AAAAAAWAJL6H4 . You are receiving this because you are subscribed to this thread.Message ID: @.***>

sunny-Ne5 commented 1 year ago

Hi mondwan, as I mentioned installing ping fixed the issue for me but it took me sometime to figure that out. Ideally library should have throwed a warning that ping is not installed.

mondwan commented 1 year ago

Thanks for the report.

But, what I mean is that the error message you need will be presented within the promise based response.

The callback style interface is kind of a legacy. There is a ticket for aligning the response between the callback, and the promise interface. Just so happened that we haven't decided when to do such enhancement yet.

On Sun, 19 Mar 2023, 19:47 SUNNY TIWARI, @.***> wrote:

Hi mondwan, as I mentioned installing ping fixed the issue for me but it took me sometime to figure that out. Ideally library should have throwed a warning that ping is not installed.

— Reply to this email directly, view it on GitHub https://github.com/danielzzz/node-ping/issues/160#issuecomment-1475378659, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA5YBI2WLC2AGUQ3LJGS7IDW45PETANCNFSM6AAAAAAWAJL6H4 . You are receiving this because you commented.Message ID: @.***>

sunny-Ne5 commented 1 year ago

Got it, thanks 😃. Will add the promise based approach. Closing this ticket.