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

Initial support proposal for http2 #3390

Closed
wants to merge 12 commits into
base: master
from

Conversation

@phouri
Copy link

phouri commented Aug 8, 2017

Hi,

PiniH here - used wrong account before (work...).

Regarding this issue: #3388

This is an initial proposal for making http2 work, this requires node to expose the Http2ServerRequest/Response and creates both request/response on the app and attaches the correct one depending on the request.

This is just an initial proposal (and a working example if Http2ServerRequest/Response) is exposed.

@phouri phouri force-pushed the phouri:initial-support-http2 branch 4 times, most recently from 1798c75 to 5d22430 Aug 8, 2017

phouri pushed a commit to phouri/node that referenced this pull request Aug 8, 2017

Expose Http2ServerRequest/Response
In order for express (and possibly other libraries) to get and use
the Http2ServerRequest/Response - expose them in the http2 exports.
Same as is done in http module.

This is only a suggestion - not sure if this is the way to go, but
with this it allows for express to run with http2 server immediately.

ref: expressjs/express#3390
fixes: nodejs#14672

@phouri phouri referenced this pull request Aug 8, 2017

Closed

http2: Expose Http2ServerRequest/Response #14690

2 of 2 tasks complete

@dougwilson dougwilson added the pr label Aug 8, 2017

@phouri phouri force-pushed the phouri:initial-support-http2 branch 2 times, most recently from 8644c2f to fc61715 Aug 9, 2017

@phouri phouri force-pushed the phouri:initial-support-http2 branch from fc61715 to 58b88f1 Aug 9, 2017

phouri added a commit to phouri/node that referenced this pull request Aug 10, 2017

Expose Http2ServerRequest/Response
In order for express (and possibly other libraries) to get and use
the Http2ServerRequest/Response - expose them in the http2 exports.
Same as is done in http module.

This is only a suggestion - not sure if this is the way to go, but
with this it allows for express to run with http2 server immediately.

ref: expressjs/express#3390
fixes: nodejs#14672

Fix Lint
@phouri

This comment has been minimized.

Copy link
Author

phouri commented Aug 11, 2017

@dougwilson once I clear the PR on nodeJS I will continue working on this if you'd like.

My gut feeling is that there will be a need to split it into 2 separate files - httpRequest and http2Request (I just did a quick work here to make it work, I don't think it's the right approach) - and take out all the special decorators into another file and use them accordingly.

WDYT?

benjamingr added a commit to nodejs/node that referenced this pull request Aug 16, 2017

http2: Expose Http2ServerRequest/Response
In order for express (and possibly other libraries) to get and use
the Http2ServerRequest/Response - expose them in the http2 exports.
Same as is done in http module.

PR-URL: #14690
Ref: expressjs/express#3390
Fixes: #14672
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MSLaguana added a commit to nodejs/node-chakracore that referenced this pull request Aug 21, 2017

http2: Expose Http2ServerRequest/Response
In order for express (and possibly other libraries) to get and use
the Http2ServerRequest/Response - expose them in the http2 exports.
Same as is done in http module.

PR-URL: nodejs/node#14690
Ref: expressjs/express#3390
Fixes: nodejs/node#14672
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

@tamaina tamaina referenced this pull request Aug 27, 2017

Closed

HTTP/2 #700 #733

@PiniH

This comment has been minimized.

Copy link

PiniH commented Aug 31, 2017

@dougwilson Hey, any thoughts regarding this? now that the PR in node is in we can move forward on this end :)

@dougwilson

This comment has been minimized.

Copy link
Member

dougwilson commented Aug 31, 2017

Seems to be in the right direction. Probably need to have CI run test suite twice: once for HTTP/1 and once for HTTP/2.

Also the test suite here does leave a lot of the fine details up to the dependencies like send, on-headers, on-finished, finalhandler, etc. and they all need their test suites run against thr HTTP/2 server to make sure they still work right.

@dougwilson

This comment has been minimized.

Copy link
Member

dougwilson commented Aug 31, 2017

P.S. I added the "5.x" label for now since this PR is changing exported API in a non-backwards-compatible way. That's not a bad thing, becasuse a 5.0 for this is fine, just wanted to note the reason.

* @return {String}
* @public
*/
if (req instanceof http.IncomingMessage) {

This comment has been minimized.

@dougwilson

dougwilson Aug 31, 2017

Member

So this is not going to make a compatible API. In Express, req.path is the pathname of thr URL (which, for instance, does not include the query stribg). Th HTTP2 prototype just aliases req.url to req.path, meaning they are the same and the req.path for HTTP2 would include the query string.

If Node.js code is going to insist on keep this change, then this could never land prior to 5.0 and req.path needs to go through a drprecation cycle in 4.x as well. Then this implementation can ignore req.path.

This comment has been minimized.

@phouri

phouri Aug 31, 2017

Author

I see,

What if we override this here and return the pathname like http1 does?
This will however create inconsistencies between http2 and http1, not sure what's better.

This comment has been minimized.

@dougwilson

dougwilson Aug 31, 2017

Member

If it's possible to do, absolutely! If you can't,
please let James in thr Node.js issue know as well. Consistency in the Express.js API is what you are promised when you use Express.js, so I'm not really understanding what inconsistency you mean would come from this. Can you explain more?

This comment has been minimized.

@phouri

phouri Aug 31, 2017

Author

Actually the inconsistency I thought about would be between the IncomingMessage and Http2ServerMessage but you are right - this is not an issue if you use express and you expect express stuff :)

I will add an override that will return the pathname like normal req.path does in express in the weekend.

This comment has been minimized.

@phouri

phouri Aug 31, 2017

Author

I will also work on reverting the api change, get all the common decorators into a util helper and create new requestHttp2.js files instead of same file returning both requests/responses.

This comment has been minimized.

@dougwilson

dougwilson Aug 31, 2017

Member

Ah, gotcha. Yea, even the http1 class has slightly changed across Node.js versions. To that end we don't document the Node.js given APIs and just tell users to refer to the docs for their Node.js version.

This comment has been minimized.

@dougwilson

dougwilson Aug 31, 2017

Member

Regarding the refactor: cool. The changes made to the test/exports file is the indication that the PR was not backwards-compatible, if you're wondering how I could tell :)

@phouri

This comment has been minimized.

Copy link
Author

phouri commented Aug 31, 2017

I tried looking up how to make the test utils use http2 client instead of http1, seems like it's not supported at the moment: visionmedia/supertest#429

As to breaking the API - I thought that maybe I can rewrite this to make separate request and requestHttp2 files - and put the common decorators in a common code, that way it won't break existing API.

As to increasing the testing suites, I think I will need a little help with that :)

@phouri

This comment has been minimized.

Copy link
Author

phouri commented Aug 31, 2017

@phouri

This comment has been minimized.

Copy link
Author

phouri commented Sep 1, 2017

Finished the refactor for this.

Moved all the code that was inside request.js / response.js to the requestDecorator/responseDecorator.

request.js and response.js now again returns the httpRequest without breaking the api.

Adjusted the req.path on http2 to return only pathname - had to create a new mini object for the parseurl library (it only ever used the url part of the request, and cached the results).

@dougwilson LMK what you think

@wesleytodd

This comment has been minimized.

Copy link
Member

wesleytodd commented Jan 23, 2018

Hey, I have not been involved in this discussion, just reading along mostly, but I was wondering if a better overall approach would be removing the prototype extension and replace it with our own custom req/res objects?

There has been discussion about doing that anyway, and it would greatly simplify this pull request I think. What do people think about doing that before we finalize this?

@phouri

This comment has been minimized.

Copy link
Author

phouri commented Jan 24, 2018

This PR was supposed to be a relatively small change that will also support http2.
Refactoring req/res sounds like a big change, and will make this redundant 👍

@wesleytodd

This comment has been minimized.

Copy link
Member

wesleytodd commented Jan 24, 2018

Not redundant, just that the work you did here would also need to be applied there. I am unsure if this could have been done in a way which didn't break backward compatibility, so it was probably going to only land in 5.x anyway. With that in mind, it seems like resolving these then applying this on top is the best way forward:

#2432
#3214

@atdiff

This comment has been minimized.

Copy link

atdiff commented Apr 4, 2018

@phouri any updates on when this PR will be finished and the http2 functionality available?

@prateekm33

This comment has been minimized.

Copy link

prateekm33 commented May 16, 2018

bump @atdiff's question.

if work on this is paused, what are some workarounds or alternative solutions that people are using?

@dougwilson

This comment has been minimized.

Copy link
Member

dougwilson commented May 16, 2018

I'm happy to pick up the work that was left off. I asked if I could get a copy in #3390 (comment) but never received anything so far. Anyone is welcome to pick this up as well, and I don't have a lot of time to put into this which is why I was really hoping to get that code.

@cainrus cainrus referenced this pull request Jul 11, 2018

Merged

Support HTTP/2 #322

2 of 6 tasks complete
@sogaani

This comment has been minimized.

Copy link

sogaani commented Jul 29, 2018

Hello @dougwilson
I'm working on this PR.
My forked branch have been working tests with http2. Would you review my codes and help me to apply my commit on this PR?

Run tests with following commands.

npm install
npm run test-http2

I changed following packages to support http2.
We need to apply PR those packages first.

@benjamingr

This comment has been minimized.

Copy link

benjamingr commented Jul 29, 2018

cc @phouri

@phouri

This comment has been minimized.

Copy link
Author

phouri commented Jul 30, 2018

Feel free to take over this PR and continue what ever is needed, I unfortunately do not have time for this :(

@phouri

This comment has been minimized.

Copy link
Author

phouri commented Jul 30, 2018

Can close this if needed

@sogaani

This comment has been minimized.

Copy link

sogaani commented Aug 28, 2018

I create PR #3730 to rebase and add tests.

@sogaani sogaani referenced this pull request Aug 29, 2018

Open

Support http2 #1145

@p3x-robot

This comment was marked as off-topic.

Copy link

p3x-robot commented Oct 17, 2018

so can we use it with branch 5.x now? even http2 websocket is implemented, it is 2018 october is. with koa, it is working, but using a legacy express system, would be good to be able to use it.

@Abourass

This comment was marked as off-topic.

Copy link

Abourass commented Nov 6, 2018

So, now that Node 10 is LTS, and the HTTP2 module is included by default, what's going on with this?

@dougwilson

This comment has been minimized.

Copy link
Member

dougwilson commented Nov 6, 2018

Since the pull request has merge conflicts and the author asked for it to be closed and a new pull request was created to take it's place, I am closing this now that it is effectively a duplicate and the author won't be moving it forward anyhow.

@dougwilson dougwilson closed this Nov 6, 2018

@expressjs expressjs locked and limited conversation to collaborators Nov 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.