fix: take the first network found instead of the last one

[vincentfretin]

Take the first network found instead of the last one that may be the docker bridge. This restores the same behavior as 5.0.4, this fixes a regression introduced in 5.1.0 where the default-gateway dependency was dropped.

• This is a bugfix
• This is a feature
• This is a code refactor
• This is a test update
• This is a docs update
• This is a metadata update

For Bugs and Features; did you add new tests?

I tried to include a test and run it with
npm run test:only test/cli/hot-option.test.js
but I can't make it run on my machine, it's just blocking at
RUNS test/cli/hot-option.test.js
no idea why.
I also have lots of failing tests and other files blocking like this when I'm testing on my machine.
Hopefully the CI may execute the test, finger crossed.

Motivation / Use-Case

On version 5.0.4 with --host local-ip on Ubuntu, it returned
https://192.168.1.15:8080
On 5.1.0 and 5.2.0 it returns
https://172.17.0.1:8080 that is the docker bridge, not so useful.

The new code introduced in #5255 was taking the last network of the networks array.
The change in this PR take the first one in networks array.

Breaking Changes

No breaking change, this fixes a regression introduced in 5.1.0 where the default-gateway dependency was dropped.

Additional Info

console.log(networks)
[
  {
    address: '192.168.1.15',
    netmask: '255.255.255.0',
    family: 'IPv4',
    mac: '50:eb:f6:97:9f:6f',
    internal: false,
    cidr: '192.168.1.15/24'
  },
  {
    address: '172.19.0.1',
    netmask: '255.255.0.0',
    family: 'IPv4',
    mac: '02:42:e4:c8:6e:5f',
    internal: false,
    cidr: '172.19.0.1/16'
  },
  {
    address: '172.18.0.1',
    netmask: '255.255.0.0',
    family: 'IPv4',
    mac: '02:42:f6:7e:a2:45',
    internal: false,
    cidr: '172.18.0.1/16'
  },
  {
    address: '172.17.0.1',
    netmask: '255.255.0.0',
    family: 'IPv4',
    mac: '02:42:3e:89:61:cf',
    internal: false,
    cidr: '172.17.0.1/16'
  }
]

[linux-foundation-easycla]

• ✅login: vincentfretin / (daa191c)

The committers listed above are authorized under a signed CLA.

[vincentfretin]

@mahdikhashan and @alexander-akait you worked on #5255 what do you think of the change?

[alexander-akait]

@vincentfretin hm, I think it is a breaking change too, in such case I recommend to provide a value for host... We can't detect a right value in such situation

[vincentfretin]

On Ubuntu it seems I always have loopback, ethernet, wifi and docker bridge in this order.
I have the same order with the ip a command and with node:

const os = require("os");
os.networkInterfaces()

In the previous code it also returned the first matching network address
https://github.com/silverwind/default-gateway/blob/master/index.js#L10-L28
it seems to be also the case for darwin, android, freebsd, openbsd.
For win32 is was really another logic.

What order do you have in other OSes you tested? And why don't you have the issue? Maybe you don't have docker on your machine?
In what OS did you test your changes @mahdikhashan?

[mahdikhashan]

On Ubuntu it seems I always have loopback, ethernet, wifi and docker bridge in this order. I have the same order with the ip a command and with node:

const os = require("os");
os.networkInterfaces()

In the previous code it also returned the first matching network address https://github.com/silverwind/default-gateway/blob/master/index.js#L10-L28 it seems to be also the case for darwin, android, freebsd, openbsd. For win32 is was really another logic.

What order do you have in other OSes you tested? And why don't you have the issue? Maybe you don't have docker on your machine? In what OS did you test your changes @mahdikhashan?

I used a mac - a recent one with silicon chip.

[vincentfretin]

@mahdikhashan on macOS what do you have with os.networkInterfaces() or a subset of it if you add a console.log(networks) in the code? Do you have only one interface?

[mahdikhashan]

@mahdikhashan on macOS what do you have with os.networkInterfaces() or a subset of it if you add a console.log(networks) in the code? Do you have only one interface?

I don't have access to my code setup now, if I remember correctly I had multiple networks - I remember in my first changes, related to this current issue - I had selected the first interface.

[vincentfretin]

In #5255 the code was taking the last one:

  let host;
  Object.values(os.networkInterfaces())
  ...
  .forEach((network) => {
    host = network.address;

    if (host.includes(":")) {
      host = `[${host}]`;
    }
  });
  // host is the last one

and in the latest commit with changes made by @alexander-akait it's now

  let host;
  const networks = Object.values(os.networkInterfaces()) ...
       
  for (const network of networks) {
    host = network.address;

     if (host.includes(":")) {
      host = `[${host}]`;
    }
  }
 // host is the last one

both code are equivalent and take the last one.

In default-gateway, it returned the first one:
https://github.com/silverwind/default-gateway/blob/master/index.js#L20-L24
so to me it's just a regression from what we had in 5.0.4 when we used the default-gateway dependency.

[alexander-akait]

default-gateway is deprecated and no longer supported, so we switched on simple and fast solution, yeah, there may be some complex and exotic exceptions, like you faced, but I don't think we need to change logic (this logic used in other tools), I think better to setup host directly in your case, because your output from os.networkInterfaces() looks good for me and we can't determine what is a bridge and what is not

It would be interesting to know where Node.js takes order for os.networkInterfaces(), maybe fixing order there will be better

[vincentfretin]

https://github.com/nodejs/node/blob/a08129cb0d8ff1df3ce45c3910cae1fc72aaf080/lib/os.js#L271
-> https://github.com/nodejs/node/blob/a08129cb0d8ff1df3ce45c3910cae1fc72aaf080/src/node_os.cc#L180
-> https://github.com/nodejs/node/blob/main/deps/uv/src/unix/linux.c#L1957
-> if (getifaddrs(&addrs)) https://man7.org/linux/man-pages/man3/getifaddrs.3.html

getifaddrs doesn't mention any order, so yeah it seems to be an undefined behavior.

If I remove host key in my webpack.config.js devServer config
I get:

Project running at:
  - https://undefined:8080

that's another issue, but it listens correctly on 192.168.1.15

I switched now from

devServer: {                                                                     
    host: "local-ip",                                                              
    port: 8080,
}

to

devServer: {
    host: "0.0.0.0",
    port: 8080,
}

It shows:

Project running at:
  - https://0.0.0.0:8080

From a developer experience perspective, it would be better imo if it showed this:

Project running at:
  - https://127.0.0.1:8080
  - https://192.168.1.15:8080
  - https://172.17.0.1:8080
  - https://172.19.0.1:8080
  - https://172.18.0.1:8080

similar to how http-server does it.

That's really what my issue it about, be able to click on the address I want without knowing in advance what is my local ip.

[alexander-akait]

Project running at:

• https://undefined:8080

Can you provide reproducible example, because this should never happen

[vincentfretin]

I had this config in my file

    setupMiddlewares: function (middlewares, devServer) {
      console.log("------------------------------------------------------------");
      const port = devServer.options.port;
      const https = devServer.options.server.type === "https" ? "s" : "";
      const domain1 = `http${https}://${devServer.options.host}:${port}`;

      console.log(`Project running at:\n  - ${infoColor(domain1)}`);

      return middlewares;
    },

devServer.options.host was undefined here.
I removed that config, now I get the default output:

<i> [webpack-dev-server] Project is running at:
<i> [webpack-dev-server] Loopback: https://localhost:8080/, https://[::1]:8080/
<i> [webpack-dev-server] On Your Network (IPv4): https://172.18.0.1:8080/
<i> [webpack-dev-server] Content not from webpack is served from 'static, public' directory
<i> [webpack-dev-server] 404s will fallback to '/index.html

[alexander-akait]

You need to take this options from real server, i.e. devServer.server.address(), devServer.options store only options from developers (no options = undefined)

Anyway does your dev server work using 192.168.1.15 IP?

Only thing that we can improve it is output, we don't want to change logic of getting IPs, because it can break logic for other developers

[vincentfretin]

devServer.server.address() is giving me null but anyway I removed that from the config I'm now seeing the default output.
With host 0.0.0.0 yes it works on both https://172.18.0.1:8080 and https://192.168.1.15:8080
What I would like is indeed to show all the ipv4 addresses in the output.

[alexander-akait]

What I would like is indeed to show all the ipv4 addresses in the output.

PR welcome

[dmarcos]

I have the same issue on My Macbook Air M1. Before, the IP showed in console when running the server matched my local IP. Now it's showing the IP of a different interface (en0 is my local IP and server shows en5 as shown by ifconfig command).

My local IP is also the first one in the list as shown by @vincentfretin when logging console.log(networks) as shown in this PR top comment

[vincentfretin]

@alexander-akait I'm curious if we revert back to using the first interface, you said it would be a breaking change. Do you currently use a system where taking the first interface would chose the wrong interface for you?
It seems on Ubuntu and macOS taking the first interface is generally what you want. I also confirmed that with one other user on Ubuntu and two users on macOS who saw the same issue.
Even if getifaddrs doesn't currently specify a specific order of the listed interfaces, from what I saw over more than a decade administrating Ubuntu or debian servers, it always listed the interfaces in the same order.
I can look later at doing a PR that shows all interfaces as suggested but that won't fix the usage of host: 'local-ip' that currently chose the wrong interface for most of us.

[alexander-akait]

@vincentfretin I'm not sure about this because when testing on one of the computers I encountered the opposite problem, where the choice of the first interface was incorrect, I can't judge how common this is, because earlier we had a separate package that also checked the functionality, now as you can see the logic is a little different, I don't mind making changes, but I wouldn't want after the fix and the new release I to receive feedback where I would have to revert and raise the issue again

It seems on Ubuntu and macOS taking the first interface is generally what you want. I also confirmed that with one other user on Ubuntu and two users on macOS who saw the same issue.

Ok let's try your solution to the problem, hopefully we won't break it for many users

[alexander-akait]

I've run the tests, let's see how they go.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.42%. Comparing base (af6bd68) to head (daa191c).
Report is 112 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #5411       +/-   ##
===========================================
- Coverage   90.29%   76.42%   -13.87%     
===========================================
  Files          15       13        -2     
  Lines        1577     1977      +400     
  Branches      601      723      +122     
===========================================
+ Hits         1424     1511       +87     
- Misses        140      403      +263     
- Partials       13       63       +50     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[vincentfretin]

It seems the new test passed.

[alexander-akait]

Yeah, I just don’t understand why coverage is broken, we need to resolve it before merge, maybe testes are not executing after new tests…

[alexander-akait]

Thanks, release will be soon