Skip to content

Conversation

@camilojd
Copy link

Currently ip.address() will search in all interfaces given by os.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:

{
  lo0:
   [ { address: '127.0.0.1',
       netmask: '255.0.0.0',
       family: 'IPv4',
       mac: '00:00:00:00:00:00',
       internal: true },
     { address: '10.200.10.1',
       netmask: '255.255.255.0',
       family: 'IPv4',
       mac: '00:00:00:00:00:00',
       internal: true } ],
  en0:
   [ { address: 'fe80::d9:8c22:6588:6d41',
       netmask: 'ffff:ffff:ffff:ffff::',
       family: 'IPv6',
       mac: '06:00:00:06:0e:00',
       scopeid: 4,
       internal: false },
     { address: '192.168.1.72',
       netmask: '255.255.255.0',
       family: 'IPv4',
       mac: '06:00:00:06:0e:00',
       internal: false } ]
}

The proper value for ip.address() should be 192.168.1.72, and NOT 10.200.10.1, as currently returned by node v8.1.3 on macOS 10.12.6.

@brentvatne
Copy link

I ran into this same problem, this seems valuable

lib/ip.js Outdated
}

var all = Object.keys(interfaces).map(function (nic) {
function priority (name) {

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;

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)?

Copy link
Author

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) {

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)

@camilojd
Copy link
Author

@brentvatne just pushed a commit addressing your comments. Thanks for the review!

@camilojd
Copy link
Author

@brentvatne travis is green 🚀

@brentvatne
Copy link

@camilojd - I would merge this if I were a maintainer for the project! alas I am not ;)

Copy link
Owner

@indutny indutny left a 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

var all = Object.keys(interfaces).map(function (nic) {
function priority(name) {
if (name.substring(0, 2) === 'en' || name.substring(0, 3) === 'eth') {
Copy link
Owner

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?

Copy link
Author

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!

@camilojd
Copy link
Author

@indutny sorry, I forgot to ping you after the change. Do you think this is good to go?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants