-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
chore: update webpack and webpack-cli #2992
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2992 +/- ##
=======================================
Coverage 92.39% 92.39%
=======================================
Files 37 37
Lines 1262 1262
Branches 326 326
=======================================
Hits 1166 1166
Misses 91 91
Partials 5 5 Continue to review full report at Codecov.
|
test/cli/cli.test.js
Outdated
done(); | ||
}) | ||
.catch(done); | ||
}); |
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.
We should not delete this tests, let's improve testing here
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.
Now we don't default host so we can change the assertion to false.
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.
host should be in IPv6 here
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.
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.
Bug 😄
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.
Ideally we should print message like Project is running at https://server.com
or Project is running at IPv4 (IPv6)
, it is in my roadmap, give me tomorrow to improve this
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.
Do we need an update here?
webpack-dev-server/lib/Server.js
Lines 592 to 609 in b0868c3
const prettyPrintUrl = (newHostname) => | |
url.format({ protocol, hostname: newHostname, port, pathname: '/' }); | |
let prettyHostname; | |
let lanUrlForTerminal; | |
if (hostname === '0.0.0.0' || hostname === '::') { | |
prettyHostname = 'localhost'; | |
const localIP = | |
hostname === '::' ? internalIp.v6.sync() : internalIp.v4.sync(); | |
if (localIP) { | |
lanUrlForTerminal = prettyPrintUrl(localIP); | |
} | |
} else { | |
prettyHostname = hostname; | |
} |
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.
Yes, we need improve it here
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.
if (typeof hostname === 'undefined') {
prettyHostName = ?? // what should be printed here?
}
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.
hm, I need to look at code, why hostname here undefined 😕
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.
Let's merge as is, to unblock other deps updating and solve messages in the next PR
For Bugs and Features; did you add new tests?
Yes, updated tests.
Motivation / Use-Case
update
webpack
andwebpack-cli
to lates version.Breaking Changes
None
Additional Info
No