fix: normalize for allowedHosts #3720
Conversation
anshumanv
changed the title
| ) { | ||
| options.allowedHosts = options.allowedHosts[0]; | ||
| } | ||
|
|
There was a problem hiding this comment.
I think we should also account for allowedHosts: ['host1.com', 'host2.com', 'all']:
webpack-dev-server/lib/Server.js
Line 1516 in 8a3f0d8
- if (this.options.allowedHosts === "all") {
+ if (
+ this.options.allowedHosts === "all" ||
+ (Array.isArray(this.options.allowedHosts) && this.options.allowedHosts.includes("all"))
+ ) { What do you think?
There was a problem hiding this comment.
all and other hosts are mutually exclusive, I don't think there would be a use case for specifying other hosts with all 🤔 Maybe this would be the case for auto when we need to allow localhost along with other hosts
There was a problem hiding this comment.
Let's hear more opinions here 👍🏻
There was a problem hiding this comment.
/cc @alexander-akait need slight attention
is this okay or should we handle 'auto' too here
There was a problem hiding this comment.
I will look at this in near future
There was a problem hiding this comment.
will add tests once you confirm
There was a problem hiding this comment.
I think it was mistake adding auto (because we already allow to handle from localhost/ip/etc), we should revisit this in the next major release, I will finish PR
Codecov Report
@@ Coverage Diff @@
## master #3720 +/- ##
==========================================
+ Coverage 93.20% 93.23% +0.03%
==========================================
Files 15 15
Lines 1324 1331 +7
Branches 458 463 +5
==========================================
+ Hits 1234 1241 +7
Misses 83 83
Partials 7 7
Continue to review full report at Codecov.
|
For Bugs and Features; did you add new tests?
not yet, will add once this fix is validated
Motivation / Use-Case
Fix webpack/webpack-cli#2909
Breaking Changes
no
Additional Info