fix(middleware): add HEAD method support #393
Conversation
Codecov Report
@@ Coverage Diff @@
## master #393 +/- ##
=======================================
Coverage 96.86% 96.86%
=======================================
Files 13 13
Lines 287 287
Branches 83 83
=======================================
Hits 278 278
Misses 9 9
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #393 +/- ##
=======================================
Coverage 96.86% 96.86%
=======================================
Files 13 13
Lines 287 287
Branches 83 82 -1
=======================================
Hits 278 278
Misses 9 9
Continue to review full report at Codecov.
|
lib/middleware.js
Outdated
| @@ -44,7 +44,7 @@ module.exports = function wrapper(context) { | |||
| }); | |||
| } | |||
|
|
|||
| const acceptedMethods = context.options.methods || ['GET']; | |||
| const acceptedMethods = context.options.methods || ['GET', 'HEAD']; | |||
There was a problem hiding this comment.
What about support all method by defaults?
|
Let's wait CI green and release as new version Honestly, I don’t know why before that we only allowed |
|
@evilebottnawi I think because the most used method of requests is |
|
/cc @hiroppy What do you think about supporting all method by default? |
| @@ -69,7 +69,7 @@ _Note: The `publicPath` property is required, whereas all other options are opti | |||
| ### methods | |||
|
|
|||
| Type: `Array` | |||
| Default: `[ 'GET' ]` | |||
| Default: `undefined` | |||
There was a problem hiding this comment.
I think this change seems to be breaking change. Is this wrong?
There was a problem hiding this comment.
@hiroppy I don't think so here is the check :
const acceptedMethods = context.options.methods;
if (acceptedMethods && acceptedMethods.indexOf(req.method) === -1) {
return goNext();
}
and in tests I changed the post request test that supposed to return 404 in the previous settings to be like get now it passes the test without issues.
I changed the documentation a little in the last commit, so now it makes more sense.
There was a problem hiding this comment.
Yes, it is not breaking change, it is feature
Yeah, me too. So I think this change is good. |
|
After investigate in other middleware code (one of popular exmaples https://github.com/expressjs/serve-static/blob/master/index.js#L73) i find what i was wrong, we should handle only |
|
I send a PR, sorry for this |
|
Close in favor #398, sorry again for misleading |
What kind of change does this PR introduce?
bugfix
Did you add tests for your changes?
yes
Summary
fixes: webpack/webpack-dev-server#1833
Does this PR introduce a breaking change?
no
Other information
when I run tests I get lint errors :
Expected linebreaks to be 'LF' but found 'CRLF'andDelete '␍'and when I disable lint and run tests only requests' tests fail in expecting
Content-LengthandContent-RangeI'm using windows and I tried this on Ubuntu and it's the same. so is it normal?