fix(server): fallthrough non GET and HEAD request to routes defined in after option

[jvasseur]

• This is a bugfix
• This is a feature
• This is a code refactor
• This is a test update
• This is a docs update
• This is a metadata update

For Bugs and Features; did you add new tests?

Yes

Motivation / Use-Case

Before #2150, non-GET request where forwarded to any middleware defined in the after option, since #2150, they are handled by serve-index instead that always reply with a 405 error.

This fix issue #2370.

Breaking Changes

No

Additional Info

Closes#2370

[jsf-clabot]

All committers have signed the CLA.

[codecov]

Codecov Report

Merging #2374 into master will decrease coverage by 0.2%.
The diff coverage is 62.5%.

@@ Coverage Diff @@## master #2374 +/- ##
==========================================
- Coverage 93.95% 93.75% -0.21% 
==========================================
Files 34 34 Lines 1291 1297 +6 Branches 368 370 +2 ==========================================
+ Hits 1213 1216 +3 - Misses 77 79 +2 - Partials 1 2 +1

Impacted Files	Coverage Δ
lib/Server.js	96.71% <62.5%> (-0.63%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4c8f94...5b86099. Read the comment docs.

[alexander-akait]

Not sure it is good solution, developers can want to handle GET in own middleware too

[jvasseur]

AFAIK, serve-index let GET request that it can't handle fall-back to the next middleware, it's just non-GET request that are blindly replied as 405.

This is basically a revert of the change from app.get to app.use done in #2150 while still keeping the contentBasePublicPath feature.

[unrevised6419]

Suggested change

	if(req.method!=='GET'){
	if(req.method!=='GET'&&req.method!=='HEAD'){

[alexander-akait]

But based on previous implementation, we handle only GET, so i think we can merge this, we need fix it for server-static too https://github.com/webpack/webpack-dev-server/pull/2150/files#diff-15fb51940da53816af13330d8ce69b4eR341

/cc @iamandrewluca

[jvasseur]

@evilebottnawi the serve-static doesn't exhibit this problem because it already forward non-GET request to the next middleware by default : https://github.com/expressjs/serve-static/blob/master/index.js#L73-L76

[unrevised6419]

I'm taking a look into serve-index implementation

[unrevised6419]

serve-static has fallthrough, but we don't use it in our case, it must be added.
serve-indexdoes not have fallthrough, and I agree with the workaround in this PR.

@jvasseur also make sure you check also for HEAD method

if(req.method!=='GET'&&req.method!=='HEAD'){// ...

Also I think serveIndex needs to be returned

returnserveIndex(item)(req,res,next);

[jvasseur]

@iamandrewluca true is already the default value for the fallthrough parameter of serve-static.

I've added the check for HEAD in the code.

[unrevised6419]

recommend to rename PR into

fix(server): fallthrough non `GET` and `HEAD` request to routes defined in after option

[alexander-akait]

Good job, thanks!

/cc @hiroppy@Loonride need review

[unrevised6419]

@jvasseur pls add a comment in code before if, that serve-index does not fallthrough like serve-static

[jvasseur]

@iamandrewluca done

[unrevised6419]

@jvasseur add at the end of PR description this. It will automatically close the issue when PR is merged.

Closes #2370

[jvasseur]

Done

[wizardofhogwarts]

lgtm

[hiroppy]

Thanks!

[unrevised6419]

Released in https://github.com/webpack/webpack-dev-server/releases/tag/v3.10.2