-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Remove header property value validation #1115
Conversation
As a slight modification to your original PR description, this PR doesn't remove validation of header properties, but rather header property values. Citing the spec is also useful when submitting PRs for scheme validation changes: https://trac.tools.ietf.org/html/draft-wright-json-schema-validation-01#page-11 The documentation will have to be changed once this is merged and published: https://webpack.js.org/configuration/dev-server/#devserver-headers- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before we can approve this change, we need to add an additional test for headers set to an array, after this test:
webpack-dev-server/test/Routes.test.js
Line 60 in 69239ce
it('GET request with headers', (done) => { |
@shellscape Thanks for your quick response on this, looking forward to getting this included so we can use webpack-dev-server again. |
Codecov Report
@@ Coverage Diff @@
## master #1115 +/- ##
======================================
Coverage 76.8% 76.8%
======================================
Files 5 5
Lines 470 470
Branches 151 151
======================================
Hits 361 361
Misses 109 109 Continue to review full report at Codecov.
|
@wlindner ping. please see comments. |
1d7ff76
to
1fd4336
Compare
@shellscape I just pushed a new commit that includes tests. I went ahead and restructured the tests in this file so that I could properly do the test setup: one testing headers as a string and one as an array. If this is not how you want it to be structured please let me know. |
@wlindner thanks for making those updates, and all your work on this PR so far. you've still got a linting error in CI that needs addressing, and the branch is out of date and should be updated. |
1fd4336
to
650d9ef
Compare
@shellscape I've made all the changes you requested, and tests and linting passed locally. But, it looks like it's failing for node 6 with a route test in CI. I moved the describe blocks around in that test file, but didn't change any of the content of those tests and I don't think it's related to the header validation change. Any suggestions for what I should do to fix this so CI passes? |
34fb23b
to
650d9ef
Compare
@wlindner looks like you still have an issue with CI. Click the "details" link for the "continuous-integration/travis-ci/pr" item and examine the log to find out what's causing an issue. We can't merge until that's resolved. |
6be876c
to
599442e
Compare
- Allows multiple cookies to be set - Allows Access-Control-Allow-Origin to support multiple domains
599442e
to
7c1abbd
Compare
@shellscape ok, I think I did all the things you needed to prepare this PR. Let me know if I missed anything. |
This PR removes property validation for header properties.
What kind of change does this PR introduce?
bugfix
Did you add or update the
examples/
?No changes were necessary.
Summary
The validation that was being used before this PR was preventing multiple cookies from being set (Set-Cookie), or multiple domains being used (Access-Control-Allow-Origin). The 'http' server supports strings or arrays, so this validation was preventing this functionality from working.
#964
Does this PR introduce a breaking change?
No
Other information