-
Notifications
You must be signed in to change notification settings - Fork 207
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
Sort interfaces by priority when searching an IP address #82
base: main
Are you sure you want to change the base?
Conversation
I ran into this same problem, this seems valuable |
lib/ip.js
Outdated
@@ -376,7 +376,25 @@ ip.address = function(name, family) { | |||
return res[0].address; | |||
} | |||
|
|||
var all = Object.keys(interfaces).map(function (nic) { | |||
function priority (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.
should remove space before (name
for consistency with the rest of the codebase
lib/ip.js
Outdated
a = priority(a); | ||
b = priority(b); | ||
|
||
return a - b; |
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.
why not just return priority(a) - priority(b)
?
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.
@brentvatne you're right :-) I did that to debug my code.
lib/ip.js
Outdated
return a - b; | ||
}); | ||
|
||
var all = sortedInterfaces.map(function (nic) { |
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.
should remove space between function
and (nic)
@brentvatne just pushed a commit addressing your comments. Thanks for the review! |
@brentvatne travis is green 🚀 |
@camilojd - I would merge this if I were a maintainer for the project! alas I am not ;) |
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.
LGTM, with a minor suggestion.
lib/ip.js
Outdated
@@ -376,7 +376,22 @@ ip.address = function(name, family) { | |||
return res[0].address; | |||
} | |||
|
|||
var all = Object.keys(interfaces).map(function (nic) { | |||
function priority(name) { | |||
if (name.substring(0, 2) === 'en' || name.substring(0, 3) === 'eth') { |
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.
Sorry for delay. I've a suggestion, what do you think about either using slice
here or RegExp?
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.
@indutny slice
works for me!
@indutny sorry, I forgot to ping you after the change. Do you think this is good to go? |
Currently
ip.address()
will search in all interfaces given byos.networkInterfaces()
. This needs to take into account that multiple valid IPs can be found, and some of them may be preferable to others, depending on the type of network adapter.For example, when
os.networkInterfaces()
returns:The proper value for
ip.address()
should be192.168.1.72
, and NOT10.200.10.1
, as currently returned by node v8.1.3 on macOS 10.12.6.