http2: Expose Http2ServerRequest/Response

[phouri]

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

http2

[phouri]

@benjamingr

[benjamingr]

@jasnell please take a look.

[benjamingr]

@phouri mind sharing why this is needed for express?

[phouri]

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.

[jasnell]

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.

[benjamingr]

@jasnell@phouri is working on getting express to work with http2

[jasnell]

yeah, I've been following the conversation there. I'm going to be digging in to the PR soon

[jasnell]

@mcollina ... I know you're on vacation right now, but I'd love your opinion on this :-)

[mcollina]

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).

[jasnell]

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.

[dougwilson]

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.

[jasnell]

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.

[dougwilson]

Absolutely! If you have any particular ideas let us know :) !

[benjamingr]

CI: https://ci.nodejs.org/job/node-test-pull-request/9606/

[mcollina]

Unit tests are still missing for this.

[phouri]

What would you want me to cover in the Unit tests? That the classes are exposed?

[mcollina]

That the classes are exposed and inheritance is supposed (as express does it)

[phouri]

Can you point me to the correct file to put these tests in? (sorry, pretty new to this)

[mcollina]

You should add a new one under test/parallel/, copy one of the existing one for http2 (they start with test-http2).

[phouri]

On it, will finish it tomorrow.

[phouri]

Done, hopefully this is what you meant :)

[cjihrig]

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.

[phouri]

@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
https://github.com/expressjs/express/blob/a4bd4373b2c3b2521ee4c499cb8e90e98f78bfa5/lib/request.js , I am just not sure how deep we want to link this PR to express internals etc.

Up to you guys - lmk what you think.
@dougwilson@mcollina

[phouri]

There are also 2 small incompatibilities between Http2 req/res classes and http req/res classes:

• Response - statusMessage property has no setter in http2 and has both setter and getter for http1 which resulted in errors when trying to set statusMessage - perhaps the same warning should be issued as with the getter?
• Request - the path property is defined on http2 ServerRequest but not on incoming message - this caused a stackoverflow error in express - but in this case I believe the handling should be on the express part.

FYI.

[benjamingr]

Going to land this on green CI unless there is any objection. CI: https://ci.nodejs.org/job/node-test-pull-request/9666

[benjamingr]

Landed in 3f5d944