-
-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reimplement a synchronous method - Fixes #13 #14
Conversation
index.js
Outdated
module.exports.v6 = () => internalIp('v6'); | ||
module.exports.v4 = () => internalIp('v4'); | ||
function sync(family) { | ||
const result = defaultGateway[family].sync(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to wrap this line in a try...catch
and return the defaults in case of error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do
index.js
Outdated
|
||
return Boolean(ret); | ||
}); | ||
function match(gateway, family) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe findIp
might be a better name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from line 16 // Look for the matching interface in all local interfaces
- that's why I chose the name. doesn't much matter to me what you think it should be, happy to change it, but I thought it was appropriate given that comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I still think findIp
is more appropriate. The matching is only part of the process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aye, will change
const prefix = ipaddr.parse(addr.netmask).prefixLengthFromSubnetMask(); | ||
const net = ipaddr.parseCIDR(`${addr.address}/${prefix}`); | ||
|
||
if (net[0].kind() === gatewayIp.kind() && gatewayIp.match(net)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just noticed this: could you add a check for net[0]
here, e.g. if (net[0] && net[0].kind()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a direct copy paste from the code as it presently is in master. mentioning only to confirm that you want to piggy back on this PR for the change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, I can fix that later myself, no need to do it here.
This PR makes use of a recent update to
default-gateway
which provides synchronous methods, to reimplement sync support in this module.