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 http2 request with node 10. #1414

Merged
merged 3 commits into from
Sep 17, 2018
Merged

Conversation

sogaani
Copy link
Contributor

@sogaani sogaani commented Sep 13, 2018

HTTP/2 with node 10 did not work, because we can not use setNoDelay with HTTP/2 socket.
This PR disable request.setNoDelay on http2.

@sogaani sogaani mentioned this pull request Sep 13, 2018
5 tasks
@kornelski
Copy link
Contributor

Thanks for the fix!

Do you know if express plans to release HTTP/2 soon? I'd rather use a dependency from npm than git.

@sogaani
Copy link
Contributor Author

sogaani commented Sep 14, 2018

Sorry, I don't know, but I guess it need some more time, because there still have a nodejs bug with Express.

As Express expose there internal request and response class, we can use Express released on npm with porting http2 implements in git version. I will try it later!

@dougwilson
Copy link

@sogaani is actively working to get Express.js working with HTTP/2. Of course, Express tests itself with supertest (which uses superagent) and thus why getting superagent working with HTTP/2 is important to us in order to actually get HTTP/2 test suite.

And yes, we've been slowing grinding through Node.js bugs as we uncover them, as Express.js exercises a lot of the HTTP server implementation.

@dougwilson
Copy link

I don't know why superagent's express dependneyes needs to be your fork @sogaani . Can you ellaborate on why that is necessary?

If express needs to have HTTP/2 support in order to hand HTTP/2 support in superagent and Express needs superagent to have HTTP/2 support in order to land the support, I think we're in a catch 22 situation here.

@sogaani
Copy link
Contributor Author

sogaani commented Sep 17, 2018

@dougwilson superagent use Express in test. For testing same test cases with HTTP/2, we need Express with HTTP/2 support.

If express needs to have HTTP/2 support in order to hand HTTP/2 support in superagent and Express needs superagent to have HTTP/2 support in order to land the support, I think we're in a catch 22 situation here.

Don't worry. Express does not need waiting HTTP/2 support in superagent, because superagent expose their request object and we can exchange them with HTTP2 supported request object. I did it in Express PR.
Similarly Express expose their request and response object and we can exchange them with those supporting HTTP/2. I'll make this change today(in Japan timezone).

@sogaani
Copy link
Contributor Author

sogaani commented Sep 17, 2018

I deleted dependency with forked Express by porting Express codes supporting HTTP/2. If release Express supporting HTTP/2, I would make PR to delete duplicate codes.

@kornelski kornelski merged commit 5e3632f into ladjs:master Sep 17, 2018
@kornelski
Copy link
Contributor

Thanks!

@sogaani sogaani deleted the http2-with-node10 branch September 18, 2018 00:44
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.

3 participants