Skip to content
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

fix: response correctly when receive an OPTIONS request #4185

Merged
merged 4 commits into from Aug 10, 2022

Conversation

TrickyPi
Copy link
Contributor

@TrickyPi TrickyPi commented Jan 16, 2022

  • 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

fix: #4180

Breaking Changes

No

Additional Info

No

@TrickyPi
Copy link
Contributor Author

TrickyPi commented Jan 16, 2022

I try to run the test on my local machine. But it always hangs whenever it goes to the following steps.

RUNS test/cli/basic.test.js
RUNS test/cli/server-option.test.js
RUNS test/cli/https-option.test.js

@codecov
Copy link

codecov bot commented Jan 16, 2022

Codecov Report

Merging #4185 (da16ee5) into master (83951c8) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4185      +/-   ##
==========================================
- Coverage   92.20%   92.11%   -0.09%     
==========================================
  Files          16       16              
  Lines        1629     1637       +8     
  Branches      615      616       +1     
==========================================
+ Hits         1502     1508       +6     
- Misses        116      118       +2     
  Partials       11       11              
Impacted Files Coverage Δ
lib/Server.js 93.75% <100.00%> (+0.04%) ⬆️
lib/servers/WebsocketServer.js 89.74% <0.00%> (-5.13%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@snitin315
Copy link
Member

snitin315 commented Jan 16, 2022

I try to run the test on my local machine. But it always hangs whenever it goes to the following steps.

RUNS test/cli/basic.test.js RUNS test/cli/server-option.test.js RUNS test/cli/https-option.test.js

Make sure you have linked webpack-dev-server -

sudo npm link && npm link webpack-dev-server

@TrickyPi
Copy link
Contributor Author

TrickyPi commented Jan 17, 2022

I try to run the test on my local machine. But it always hangs whenever it goes to the following steps.
RUNS test/cli/basic.test.js RUNS test/cli/server-option.test.js RUNS test/cli/https-option.test.js

Make sure you have linked webpack-dev-server -

sudo npm link && npm link webpack-dev-server

Oh, i didn't run this command. I'm sorry about i didn't read CONTRIBUTING.md carefully.

lib/Server.js Show resolved Hide resolved
@alan-agius4
Copy link
Contributor

alan-agius4 commented Jul 27, 2022

@alexander-akait, any way we can get this in?

@alexander-akait
Copy link
Member

alexander-akait commented Jul 27, 2022

@alan-agius4 Do you want this as built-in feature?

@alan-agius4
Copy link
Contributor

alan-agius4 commented Jul 27, 2022

Ideally, the dev-server can handle preflight requests out-of-the-box as it did in version 3.

@alexander-akait
Copy link
Member

alexander-akait commented Jul 29, 2022

@alan-agius 4 interesting, because previously we don't handle it (i.e. we don't have code for this), it means one from middlewares handle this, I am not agait this merge, just want to check...

@alan-agius4
Copy link
Contributor

alan-agius4 commented Jul 29, 2022

@alexander-akait understood.

@alexander-akait
Copy link
Member

alexander-akait commented Jul 29, 2022

@alan-agius4 Do you have time to check it (just a little busy right now), thank you

@TrickyPi
Copy link
Contributor Author

TrickyPi commented Jul 29, 2022

Please let me know if some code needs to be changed, i would be happy to change it.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 29, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • login: TrickyPi / name: TrickyPi (a8b39c2, c373320)
  • login: alexander-akait / name: Alexander Akait (3cf7f9f)

@TrickyPi
Copy link
Contributor Author

TrickyPi commented Jul 29, 2022

a lot of differences in types/lib/Server.d.ts, is this normal?

@alan-agius4
Copy link
Contributor

alan-agius4 commented Jul 29, 2022

The OPTIONS request/response is handled by the express router https://github.com/expressjs/express/blob/master/lib/router/index.js#L164-L169

In version 4 however this check is always falsey, which causes the options not be handled properly.

In version 3 this worked because of the additional express Layer that caused the options to be handled.

setupMagicHtmlFeature() {
   this.app.get('*', this.serveMagicHtml.bind(this));
}

This is because the above cause a creation of an additional layer where Boolean(this.methods[name]) is.
https://github.com/expressjs/express/blob/7ec5dd2b3c5e7379f68086dae72859f5573c8b9b/lib/router/route.js#L58-L70 is false.

In fact adding the below in setupBuiltInRoutes in v4, the OPTIONS are also handled correctly.

app.get('*', (req, res, next) => { next(); });    

@alexander-akait
Copy link
Member

alexander-akait commented Jul 29, 2022

@alan-agius4 thank for this, I think we can enable it by default, should be safe for dev env

@TrickyPi Can you enable it by default?

@TrickyPi
Copy link
Contributor Author

TrickyPi commented Jul 30, 2022

Ok

@TrickyPi TrickyPi requested a review from alexander-akait Aug 3, 2022
@alexander-akait alexander-akait merged commit 2b3b7e0 into webpack:master Aug 10, 2022
14 of 17 checks passed
@TrickyPi TrickyPi deleted the fix-4180 branch Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Didn't handle preflight request correctly
4 participants