Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Support Http2 #29

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Support Http2 #29

wants to merge 5 commits into from

Conversation

sogaani
Copy link

@sogaani sogaani commented Jul 30, 2018

Support http2 to enable express tests with http2.
expressjs/express#3390.

dougwilson
dougwilson suggested changes Aug 2, 2018
View reviewed changes
Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should prefer the Host header in a http2 request over the :authority header. That seems like potential setup here for some kind of issue down the road.

@expressjs expressjs deleted a comment from sogaani Aug 2, 2018
@sogaani
Copy link
Author

sogaani commented Aug 2, 2018

I refered rfc and thought :authority header seems to have compatibility with Host header. And I'm not sure issues. But it's ok. I create this PR for express tests with http2. So we can disable tests depend on this. I feel free to close this.

@sogaani
Copy link
Author

sogaani commented Aug 7, 2018

Above comment is my misunderstanding.
Your concern is "There is possibility to refer Host header even if http2 request has :authority header." right? So, I addressed issue.

@sogaani sogaani mentioned this pull request Aug 28, 2018
Open
5 tasks
@sogaani
Copy link
Author

sogaani commented Sep 17, 2018

I fixed PR installing deps only from npm.

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@dougwilson dougwilson dougwilson requested changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sogaani @dougwilson @addaleax