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

Remove header property value validation #1115

Merged
merged 2 commits into from Oct 9, 2017

Conversation

refactornator
Copy link
Contributor

@refactornator refactornator commented Sep 25, 2017

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

@jsf-clabot
Copy link

jsf-clabot commented Sep 25, 2017

CLA assistant check
All committers have signed the CLA.

@shellscape
Copy link
Contributor

shellscape commented Sep 25, 2017

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-

Copy link
Contributor

@shellscape shellscape left a 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:

it('GET request with headers', (done) => {

@refactornator
Copy link
Contributor Author

@shellscape Thanks for your quick response on this, looking forward to getting this included so we can use webpack-dev-server again.

@codecov
Copy link

codecov bot commented Sep 26, 2017

Codecov Report

Merging #1115 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          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.

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

@shellscape
Copy link
Contributor

@wlindner ping. please see comments.

@refactornator refactornator changed the title Remove header property validation Remove header property value validation Sep 27, 2017
@refactornator
Copy link
Contributor Author

@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.

@shellscape
Copy link
Contributor

@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.

@refactornator
Copy link
Contributor Author

refactornator commented Sep 29, 2017

@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?

@shellscape
Copy link
Contributor

@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.

- Allows multiple cookies to be set
- Allows Access-Control-Allow-Origin to support multiple domains
@refactornator
Copy link
Contributor Author

@shellscape ok, I think I did all the things you needed to prepare this PR. Let me know if I missed anything.

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.

None yet

3 participants