-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
http2: Expose Http2ServerRequest/Response #14690
Conversation
@jasnell please take a look. |
@phouri mind sharing why this is needed for express? |
Express decorates the request/response and uses it throughout the application handlers, for http it uses HttpIncomingMessage and HttpServerResponse which are exposed via the http module. I thought I'd mimic the same behavior, can see more details at the relevant express PR: expressjs/express#3390 This is just preliminary suggestion, waiting for more feedback from express, but I think it might be a good idea in any case to expose these basic classes. |
Ideally we wouldn't need to do this. The right thing to do would be to figure out what hook points frameworks like express require and provide APIs for them without requiring us to expose these two classes in this way. Will have to give this one some more thought. |
yeah, I've been following the conversation there. I'm going to be digging in to the PR soon |
@mcollina ... I know you're on vacation right now, but I'd love your opinion on this :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm +1 but this needs tests on the actual use case that express has. We need to check if those needs documenting or not (they should already be there).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since @mcollina is good with it, I'm good with it. We need to strongly encourage frameworks building on top of this to not use any internal properties or mechanisms tho. We need to be strict about it.
Effectively the use-case of Express.js for the class references is to be able to subclass the objects. For a long time, Express.js has wanted to simply provide extra sugar on top of the Node.js platform, and this is done by subclassing the objects. IMO this is nice, because then anything that works with Node.js objects work with Express.js out of the box. Let me know if you would rather us change our approach. |
Subclassing is perfectly fine! The one concern I would have is around performance. If there are more performant ways we could support adding the necessary sugar via supported public APIs, then we should pursue those. |
Absolutely! If you have any particular ideas let us know :) ! |
46afd4d
to
11e742b
Compare
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
11e742b
to
dd930ca
Compare
Unit tests are still missing for this. |
What would you want me to cover in the Unit tests? That the classes are exposed? |
That the classes are exposed and inheritance is supposed (as express does it) |
Can you point me to the correct file to put these tests in? (sorry, pretty new to this) |
You should add a new one under test/parallel/, copy one of the existing one for http2 (they start with test-http2). |
On it, will finish it tomorrow. |
Add test for Http2ServerRequest/Response object
3b32877
to
4e2d3cf
Compare
Done, hopefully this is what you meant :) |
I believe this is the part of Express being referred to: https://github.com/expressjs/express/blob/a4bd4373b2c3b2521ee4c499cb8e90e98f78bfa5/lib/middleware/init.js#L35-L36 Perhaps the test should setup inheritance in a similar fashion. |
@cjihrig I am not sure about that, the setPrototypeOf there is just an iteration of the properties, and not much related to class itself, perhaps additional tests on the properties used by express to decorate the request response would be better Up to you guys - lmk what you think. |
There are also 2 small incompatibilities between Http2 req/res classes and http req/res classes:
FYI. |
Going to land this on green CI unless there is any objection. CI: https://ci.nodejs.org/job/node-test-pull-request/9666 |
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Landed in 3f5d944 |
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
http2