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