Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
alexander-akait merged 1 commit intowebpack:masterfromvincentfretin:fix-ip-used
Mar 24, 2025

Conversation

@vincentfretin
Copy link
Contributor

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 abugfix
  • This is afeature
  • This is acode refactor
  • This is atest update
  • This is adocs update
  • This is ametadata 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
Copy link

linux-foundation-easyclabot commentedJan 28, 2025•
edited
Loading

CLA Signed

  • ✅login: vincentfretin / (daa191c)

The committers listed above are authorized under a signed CLA.

@vincentfretin
Copy link
ContributorAuthor

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

@alexander-akait
Copy link
Member

@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
Copy link
ContributorAuthor

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

constos=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
Copy link
Contributor

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

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

In the previous code it also returned the first matching network addresshttps://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
Copy link
ContributorAuthor

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

@mahdikhashan
Copy link
Contributor

@mahdikhashan on macOS what do you have withos.networkInterfaces() or a subset of it if you add aconsole.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
Copy link
ContributorAuthor

In#5255 the code was taking the last one:

lethost;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

lethost;constnetworks=Object.values(os.networkInterfaces())...for(constnetworkofnetworks){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
Copy link
Member

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 fromos.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 foros.networkInterfaces(), maybe fixing order there will be better

@vincentfretin
Copy link
ContributorAuthor

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
Copy link
Member

Project running at:

Can you provide reproducible example, because this should never happen

@vincentfretin
Copy link
ContributorAuthor

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
Copy link
Member

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

Anyway does your dev server work using192.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
Copy link
ContributorAuthor

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 bothhttps://172.18.0.1:8080 andhttps://192.168.1.15:8080
What I would like is indeed to show all the ipv4 addresses in the output.

@alexander-akait
Copy link
Member

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

PR welcome

@dmarcos
Copy link

dmarcos commentedFeb 10, 2025•
edited
Loading

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 loggingconsole.log(networks) as shown inthis PR top comment

@vincentfretin
Copy link
ContributorAuthor

@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
Copy link
Member

@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
Copy link
Member

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

@codecov
Copy link

codecovbot commentedFeb 10, 2025•
edited
Loading

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
Copy link
ContributorAuthor

It seems the new test passed.

@alexander-akait
Copy link
Member

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-akaitalexander-akait merged commitffd0b86 intowebpack:masterMar 24, 2025
52 of 53 checks passed
@alexander-akait
Copy link
Member

Thanks, release will be soon

@vincentfretinvincentfretin deleted the fix-ip-used branchMarch 25, 2025 07:52
github-merge-queuebot pushed a commit to lynx-family/lynx-stack that referenced this pull requestApr 3, 2025
…ne (#457)<!--  Thank you for submitting a pull request!We appreciate the time and effort you have invested in making thesechanges. Please ensure that you provide enough information to allowothers to review your pull request.Upon submission, your pull request will be automatically assigned withreviewers.If you want to learn more about contributing to this project, pleasevisit:https://github.com/lynx-family/lynx-stack/blob/main/CONTRIBUTING.md.-->## Summary<!-- Can you explain the reasoning behind implementing this change? Whatproblem or issue does this pull request resolve? -->Following up to#91, portwebpack/webpack-dev-server#5411<!-- It would be helpful if you could provide any relevant context, suchas GitHub issues or related discussions. -->## Checklist<!--- Check and mark with an "x" -->- [x] Tests updated (or not required).- [ ] Documentation updated (or **not required**).
colinaaa pushed a commit to lynx-family/lynx-stack that referenced this pull requestApr 5, 2025
This PR was opened by the [Changesetsrelease](https://github.com/changesets/action) GitHub action. Whenyou're ready to do a release, you can merge this and the packages willbe published to npm automatically. If you're not ready to do a releaseyet, that's fine, whenever you add more changesets to main, this PR willbe updated.# Releases## @lynx-js/rspeedy@0.9.0### Minor Changes- Bundle Rspeedy with Rslib for faster start-up times.([#395](#395))This would be a **BREAKING CHANGE** for using [global version ofRspeedy](https://lynxjs.org/rspeedy/cli#using-the-global-rspeedy-version).Please ensure that you update your globally installed version ofRspeedy:    ```bash    npm install --global @lynx-js/rspeedy@latest    ```- Bump Rsbuild v1.3.2 with Rspack v1.3.1([#446](#446))- **BREAKING CHANGE**: Added explicit TypeScript peer dependencyrequirement of 5.1.6 - 5.8.x.([#480](#480))This formalizes the existing TypeScript version requirement in`peerDependencies` (marked as optional since it is only needed forTypeScript configurations). The actual required TypeScript version hasnot changed.Note: This may cause installation to fail if you have strict peerdependency checks enabled.Node.js v22.7+ users can bypass TypeScript installation using`--experimental-transform-types` or `--experimental-strip-types` flags.Node.js v23.6+ users don't need any flags. See [Node.js -TypeScript](https://nodejs.org/api/typescript.html) for details.### Patch Changes- Support cli flag `--base` to specify the base path of the server.([#387](#387))- Support cli option `--environment` to specify the name of environmentto build ([#462](#462))- Select the most appropriate network interface.([#457](#457))This is a port of[webpack/webpack-dev-server#5411](webpack/webpack-dev-server#5411).- Support Node.js v23.6+ native TypeScript.([#481](#481))See [Node.js - TypeScript](https://nodejs.org/api/typescript.html) formore details.- Support cli option `--env-mode` to specify the env mode to load the`.env.[mode]` file.([#453](#453))- Support `dev.hmr` and `dev.liveReload`.([#458](#458))- Updated dependencies\[[`df63722`](df63722),[`df63722`](df63722)]:    -   @lynx-js/chunk-loading-webpack-plugin@0.2.0## @lynx-js/chunk-loading-webpack-plugin@0.2.0### Minor Changes- **BREAKING CHANGE**: Requires `@rspack/core` v1.3.0.([#400](#400))- **BREAKING CHANGE**: Remove the deprecated `ChunkLoadingRspackPlugin`,use `ChunkLoadingWebpackPlugin` with `output.chunkLoading: 'lynx'`instead. ([#400](#400))    ```jsimport { ChunkLoadingWebpackPlugin } from"@lynx-js/chunk-loading-webpack-plugin";    export default {      output: {        chunkLoading: "lynx",      },      plugins: [new ChunkLoadingWebpackPlugin()],    };    ```## @lynx-js/react@0.106.3### Patch Changes- Do some global var initialize in hydrate, which fixes error like`cannot read property '-21' of undefined` and some style issue.([#461](#461))- fix: ensure ref lifecycle events run after firstScreen in thebackground thread([#434](#434))This patch fixes an issue where ref lifecycle events were running beforefirstScreen events in the background thread async render mode, whichcould cause refs to be undefined when components try to access them.## @lynx-js/qrcode-rsbuild-plugin@0.3.5### Patch Changes- Build with Rslib([#396](#396))## @lynx-js/react-rsbuild-plugin@0.9.5### Patch Changes- fix: add enableCSSInvalidation for encodeCSS of css HMR, this will fixpseudo-class (such as `:active`) not working in HMR.([#435](#435))- Disable `module.generator.json.JSONParse` option as it increases thebundle size of `main-thread.js`. For more detail, please see this[issue](webpack/webpack#19319).([#402](#402))- Updated dependencies\[[`3e7988f`](3e7988f),[`7243242`](7243242)]:    -   @lynx-js/css-extract-webpack-plugin@0.5.3    -   @lynx-js/template-webpack-plugin@0.6.8    -   @lynx-js/react-alias-rsbuild-plugin@0.9.5    -   @lynx-js/react-refresh-webpack-plugin@0.3.2    -   @lynx-js/react-webpack-plugin@0.6.10    -   @lynx-js/web-webpack-plugin@0.6.3## @lynx-js/web-constants@0.10.1### Patch Changes- feat: onNapiModulesCall function add new param: `dispatchNapiModules`,napiModulesMap val add new param: `handleDispatch`.([#414](#414))Now you can use them to actively communicate to napiModules (backgroundthread) in onNapiModulesCall (ui thread).-   Updated dependencies \[]:    -   @lynx-js/web-worker-rpc@0.10.1## @lynx-js/web-core@0.10.1### Patch Changes- docs: fix documents about lynx-view's properties([#412](#412))    Attributes should be hyphen-name: 'init-data', 'global-props'.    now all properties has corresponding attributes.- feat: onNapiModulesCall function add new param: `dispatchNapiModules`,napiModulesMap val add new param: `handleDispatch`.([#414](#414))Now you can use them to actively communicate to napiModules (backgroundthread) in onNapiModulesCall (ui thread).- Updated dependencies\[[`1af3b60`](1af3b60)]:    -   @lynx-js/web-constants@0.10.1    -   @lynx-js/web-worker-runtime@0.10.1    -   @lynx-js/web-worker-rpc@0.10.1## @lynx-js/web-elements@0.5.3### Patch Changes- feat: add `layoutchange` event support for x-view and x-text([#408](#408))## @lynx-js/web-mainthread-apis@0.10.1### Patch Changes- Updated dependencies\[[`1af3b60`](1af3b60)]:    -   @lynx-js/web-constants@0.10.1## @lynx-js/web-worker-runtime@0.10.1### Patch Changes- feat: onNapiModulesCall function add new param: `dispatchNapiModules`,napiModulesMap val add new param: `handleDispatch`.([#414](#414))Now you can use them to actively communicate to napiModules (backgroundthread) in onNapiModulesCall (ui thread).- Updated dependencies\[[`1af3b60`](1af3b60)]:    -   @lynx-js/web-constants@0.10.1    -   @lynx-js/web-mainthread-apis@0.10.1    -   @lynx-js/web-worker-rpc@0.10.1## @lynx-js/css-extract-webpack-plugin@0.5.3### Patch Changes- Fix CSS HMR not working with nested entry name.([#456](#456))- fix: add enableCSSInvalidation for encodeCSS of css HMR, this will fixpseudo-class (such as `:active`) not working in HMR.([#435](#435))## @lynx-js/template-webpack-plugin@0.6.8### Patch Changes- fix: add enableCSSInvalidation for encodeCSS of css HMR, this will fixpseudo-class (such as `:active`) not working in HMR.([#435](#435))## create-rspeedy@0.9.0## @lynx-js/react-alias-rsbuild-plugin@0.9.5## upgrade-rspeedy@0.9.0## @lynx-js/web-worker-rpc@0.10.1Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@alexander-akaitalexander-akaitalexander-akait approved these changes

@hiroppyhiroppyAwaiting requested review from hiroppy

@snitin315snitin315Awaiting requested review from snitin315

@anshumanvanshumanvAwaiting requested review from anshumanv

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vincentfretin@alexander-akait@mahdikhashan@dmarcos
[8]ページ先頭
©2009-2025 Movatter.jp