Skip to content

Fix http2 request with node 10. - #1414

Merged
kornelski merged 3 commits into
forwardemail:masterfrom
sogaani:http2-with-node10
Sep 17, 2018
Merged

kornelski merged 3 commits into
forwardemail:masterfrom
sogaani:http2-with-node10

Conversation

@sogaani

@sogaani sogaani commented Sep 13, 2018

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

sogaani commented Sep 14, 2018

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown

@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
Copy Markdown

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

sogaani commented Sep 17, 2018

Copy link
Copy Markdown
Contributor Author

@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

sogaani commented Sep 17, 2018

Copy link
Copy Markdown
Contributor Author

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 forwardemail:master Sep 17, 2018
@kornelski

Copy link
Copy Markdown
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