feat: improve host output and fix open #2892
Conversation
|
/cc @hiroppy |
|
When you use When it is not IP: |
lib/Server.js
Outdated
| if (hostname === '0.0.0.0' || hostname === '::') { | ||
| prettyHostname = 'localhost'; | ||
|
|
||
| const localIP = internalIp.v4.sync(); |
There was a problem hiding this comment.
| const localIP = internalIp.v4.sync(); | |
| const localIP = hostname === '::' ? internalIp.v6.sync() : internalIp.v4.sync(); |
There was a problem hiding this comment.
It is for terminal output, developer should copy past it from terminal, IPv6 is not supported in many browsers
There was a problem hiding this comment.
url automatically formats IPv6 hostname with brackets, which most browsers support including IE 11, e.g., http://[::]:8080. I think IPv6 should take precendence over IPv4, if available.
There was a problem hiding this comment.
make sense, I hope this does not scare developers
lib/Server.js
Outdated
| if ( | ||
| /^10[.]|^172[.](1[6-9]|2[0-9]|3[0-1])[.]|^192[.]168[.]/.test(localIP) | ||
| ) { | ||
| // Address is private, format it for later use |
There was a problem hiding this comment.
I think this check is not needed, as 0.0.0.0 (::) will bind to a public address, if available.
There was a problem hiding this comment.
You can have local public IP (it is very rare edge case, but it is possible), for example I can run it on my server with public IP
There was a problem hiding this comment.
In that case, I think the public IP should be printed.
There was a problem hiding this comment.
In this case we print two IP - local and network
There was a problem hiding this comment.
IMHO, I see no reason not to print the internal IP (the IP address of the NIC) if it's a public one. It's still a network IP, just not a LAN.
There was a problem hiding this comment.
I don't have strong opinion here, it was inspired by CRA and their feedback, but maybe you are right
There was a problem hiding this comment.
Let's simplify it, we will change it in future after feedback
| @@ -28,7 +28,7 @@ exports[`CLI --hot webpack 4 1`] = ` | |||
| `; | |||
|
|
|||
| exports[`CLI --hot webpack 5 1`] = ` | |||
There was a problem hiding this comment.
Note snapshots should be updated twice, each for webpack 4 and 5.
| @@ -8,27 +8,27 @@ function createDomain(options, server) { | |||
| // use location hostname and port by default in createSocketUrl | |||
There was a problem hiding this comment.
WIth showStatus() now using its own logic, this is only used in client entries. I think this can be refactored in the future PR.
There was a problem hiding this comment.
I think logic for terminal output and for entries is different, it is similar in some places, but it is for different purpose
Codecov Report
@@ Coverage Diff @@
## master #2892 +/- ##
==========================================
+ Coverage 92.45% 92.52% +0.07%
==========================================
Files 38 37 -1
Lines 1246 1258 +12
Branches 324 333 +9
==========================================
+ Hits 1152 1164 +12
Misses 89 89
Partials 5 5
Continue to review full report at Codecov.
|
|
/cc @ylemkimon Done |
|
IPv6 is not supported by github actions, we can't test it 😞 |
| this.options.dev && | ||
| typeof this.options.dev.publicPath !== 'undefined' | ||
| ) { | ||
| this.options.info( |
There was a problem hiding this comment.
@alexander-akait looks like this.options.info should be replaced with this.logger.info
There was a problem hiding this comment.
Already fixed in master
For Bugs and Features; did you add new tests?
Yes
Motivation / Use-Case
fixes #2874
Breaking Changes
No
Additional Info
inspired by CRA