Remove direct http dependency #3213

wesleytodd · 2017-02-20T17:14:09Z

I think this might resolve the issues brought up in #2812 and #3206, and also some of what we talked about at the most recent TC meeting for compat in the browser. The goal here is that you can do:

var express = require('express')
var uws = require('uws')
var app = express({
  reqProto: uws.http.getRequestPrototype(),
  resProto: uws.http.getResponsePrototype(),
  createServer: uws.http.createServer
})

In the long run, I think this is also what I need for my Nighthawk project as well, so IMO it is a good way forward. But let me know your thoughts!

CC: @dougwilson, @alexhultman, @blakeembrey

ghost · 2017-02-20T17:16:52Z

I've removed uws.http.getRequestPrototype() and uws.http.getResponsePrototype() and instead I provide var app = uws.http.getExpressApp(express) so this won't work anymore

wesleytodd · 2017-02-20T17:17:28Z

Notes:

I tried to apply the "standard" style where I made changes, but it results in mixed files, so I can change that back if we want.
I applied the setPrototypeOf module where we were using __proto__ like we did in the router, this is also something we could discuss, especially if we drop support for older nodes and browsers.
This does nothing to help the larger issue of NOT modifying the prototypes of all req/res, but I think that moving forward with this should not change anything we do there.
Also, not sure if this helps or hinders the efforts for http2 support, so please comment to that regard, thanks!

wesleytodd · 2017-02-20T17:19:29Z

@alexhultman Hmm, well it seems to me to be a better api for your project to not have to know about express, and for express to just support arbitrary extensions like in this PR. With getExpressApp you are now relying on express supporting what you use, but inverting the responsibility here removes that need.

wesleytodd · 2017-02-20T17:27:50Z

Lol, tests that hit google.com and rely on a specific html response? Should I fix these brittle tests? Remove them? Or ignore the failures for this PR?

ghost · 2017-02-20T17:30:00Z

I'll add them back then.

wesleytodd · 2017-02-20T17:35:29Z

@alexhultman You might want to wait until we get some feedback from the greater @expressjs team and community, the above is merely my opinion :)

ghost · 2017-02-20T17:36:18Z

Too late I already added them again

dougwilson · 2017-02-20T19:43:54Z

The test failures are just from a conflict with 2f8ac67 in which the test is expecting the same code names from the http Node.js module but this moved them to use the statuses module. I think the change to use statuses could probably be extracted to a PR (maybe even against 4.x?).

wesleytodd · 2017-02-20T21:55:20Z

Updated to move the status stuff into a separate PR, but left the commit in here just so it is clear the intent. Now that I look I see you did say against 4, so I will update that.

One other thing we should have open for discussion is the idea of passing options to createApplication. Doug mentioned that the current way was a preference of @tj, but that we could bring it up for discussion again. My thoughts are that offering these kind of options while creating the application is nice. There is also no reason we could not offer both api's, but that increases the test and maintenance surface. Also, there are some types of settings that should not be changed after startup, because middleware might rely on them working in a certain way, like query string parsing, so IMO these should not be changeable throughout the lifetime of the app like they are now.

wesleytodd · 2017-02-20T22:06:09Z

Last comment, I think this PR might also be able to land of 4.x. It passes all the tests, and is really just a minor point addition for the new options api.

LinusU

Neat 👍

dougwilson · 2017-02-22T07:45:28Z

lib/express.js

-  app.response = { __proto__: res, app: app };
+  app.createServer = opts.createServer
+  app.request = setPrototypeOf({ app: app }, req(opts.reqProto))
+  app.response = setPrototypeOf({ app: app }, res(opts.resProto))


I believe these two lines could just be Object.create(proto, descriptorsWithJustApp), right?

app.response = Object.create(res(opts.resProto), { app: { configurable: true, enumerable: true, writable: true, value: app } })

dougwilson · 2017-02-22T07:48:16Z

lib/express.js

@@ -32,16 +57,22 @@ exports = module.exports = createApplication;
 * @api public
 */

-function createApplication() {
+function createApplication(opts) {
+  opts = opts || {}


Probably don't want to reassign values linked to arguments, since it silently alters arguments[0]. Usually I would just name the argument options and this line as var opts = options || {}

$ node -pe '(function(foo){foo=foo||{};return arguments[0] === null}(null))' false

Good call, fixed.

dougwilson · 2017-02-22T07:49:51Z

lib/express.js

+  opts = opts || {}
+  opts.reqProto = opts.reqProto || http.IncomingMessage.prototype
+  opts.resProto = opts.resProto || http.ServerResponse.prototype
+  opts.createServer = opts.createServer || http.createServer


Usually not best practice to alter the object someone is passing in; the object could potentially be frozen, even, causing an exception here. var createServer = opts.createServer || http.createServer is probably fine (and then changing the vars below).

mjsalinger · 2017-05-04T11:15:50Z

@dougwilson Any way we can get this merged?

dougwilson · 2017-05-05T03:40:43Z

H @mjsalinger before merging, the CI should at least pass for the PR.

kibertoad · 2018-02-11T23:38:16Z

@mjsalinger Could you review those CI failures and address if needed?

wesleytodd · 2018-02-13T19:50:44Z

I have not had a chance to look at the failures in this PR, which I think needs a bit of revision in respect to the discussion here.

LucyMaber · 2023-05-20T11:14:00Z

what the time line on this

wesleytodd · 2024-05-14T02:57:53Z

Hey @LucyMaber, I know this is likely not the timeline you hoped for, but we are now trying to organize an effort to land these before the final v5 release (see the issue mention right above this). If you are still looking for this, I would love if you could post why you wanted it. It is hard enough to pick up work we thought was valuable back then, but it is even harder to decide if it is still worth doing when most of the comments do not contain enough information about why folks asked for this.

wesleytodd · 2024-05-14T02:59:29Z

@kibertoad I started scrolling up and should have waited to post that to also mention you! Can you help give a bit of context on what this change helps with? I know it was something which helped me in 2017, but I am not sure I really understand why this might still be helpful in 2024.

kibertoad · 2024-05-14T16:04:19Z

@wesleytodd If I'm not mistaken, Node.js wasn't able to remove certain http warts precisely because express relied on them. If that helps to unblock Node.js, I'm all for it.

wesleytodd · 2024-05-15T16:27:34Z

This particular change was a bit unrelated to that. It was more about enabling non-node core (but api compatible) implementations. Sadly, until we can get the api's in place and stop monkey-patching req/res it does not help node core move forward yet.

I will work on the other pending todo topics first, and hopefully we can make a decision on if landing this is a good idea in the mean time.

wesleytodd · 2024-05-17T20:52:19Z

I have decided this is worth landing. The main reason I think so is because it gives us an interesting opportunity to experiment with the http-next effort ongoing in the Node.js Web Server Frameworks Team. We could use this to provide new compatibility layers to allow using that work within express which might help us validate that work. Just wanted to post that update here and give folks one last chance to oppose landing this. I am going to get the branch into a conflict free updated state now and then if I don't hear back I will merge early next week.

blakeembrey

I think this requires more thought before we land it for 5.x because I don't think it's the ideal API to achieve the goal, and I wouldn't want to churn it again for 6.x 🤷

blakeembrey · 2024-05-22T22:54:10Z

lib/express.js

-exports.request = req;
-exports.response = res;
+exports.request = req.proto
+exports.response = res.proto


I think these exports are meaningless now because they're just empty objects?

Yeah probably, hard to remember if I had even looked at that since it was so long ago, but I think you are right. I will hold off on addressing these because the other review comments are much more important for us to address first imo.

blakeembrey · 2024-05-22T22:58:45Z

lib/request.js

-
-var req = Object.create(http.IncomingMessage.prototype)
+exports = module.exports = function (proto) {
+  return setPrototypeOf(req, proto)


It's almost functionally the same for a single server, but this would be clearer by making all of req a function that returns the existing behavior (and can have multiple instances created). My main concern with setPrototypeOf here is that you'd be changing the prototype if you actually intended to use this feature to run two versions of e.g. HTTP1 and HTTP2 servers. This seems like a bit of a foot gun for anyone running tests, especially if they tried to parallelize or something.

Edit: This isn't required if you instead do the mixin approach in createApplication instead.

Yeah I agree with this including the edit. The use case of multiple servers was always bad with express patching things though so I am not sure this makes it worse, but the mixin way seems like it would make it better in general. I am +1 for going that route and calling these other comments resolved.

blakeembrey · 2024-05-22T23:04:17Z

lib/express.js

+  var options = opts || {}
+  options.reqProto = options.reqProto || http.IncomingMessage.prototype
+  options.resProto = options.resProto || http.ServerResponse.prototype
+  options.createServer = options.createServer || http.createServer


Instead of making the http object above, why not just require('http') here instead? You'd avoid three requires. Instead, since all 3 of these options should be expected to be set together (why would you mix http native with implementation?), just make them all required or fallback on something like var options = opts || getHttpOptions().

I like this idea and honestly any use cases where you might use node's server but custom req/res is probably not really that viable anyway. Maybe to nitpick the name idea though, what about overrideHttp() or something in that vien? My idea here is that it is an it should be clear you shouldn't use this unless you really want to override something which is more of a power user. I was trying to think of a name that sends that message more without being over done.

blakeembrey · 2024-05-22T23:12:28Z

lib/application.js

@@ -607,7 +606,7 @@ app.render = function render(name, options, callback) {
 */

 app.listen = function listen () {
-  var server = http.createServer(this)
+  var server = this.createServer(this)


Honestly I think this should just deprecate this in favor of people explicitly writing http.createServer(app).listen(...). It also removes one more option from the createApplication options which is only ever used once here. It took me a while to follow the indirect assignments around and I think that's bad for developers and users to understanding what's going on.

But I do understand why it's done after 30 minutes of review and won't block on this issue. I would be in favor of refactoring this to assign app.listen within the create function instead. That way there's less indirect dependencies that's hard to reason about and createServer is used right where it's defined.

I would be in favor of refactoring this to assign app.listen within the create function instead.

Of the options to achieve this I think this one is my preference. I believe any of these loose the once behavior, but I am not sure it makes a big difference but I agree simplifying things like this is good. I honestly cannot remember if there used to be cases where the server could emit error events more than once, but maybe that was an old node behavior?

blakeembrey · 2024-05-22T23:20:09Z

lib/express.js

  // expose the prototype that will get set on requests
-  app.request = Object.create(req, {
+  app.request = Object.create(req(options.reqProto), {


Why not Object.create(options.reqProto, ...) and then mixin(app.request, req) instead? Seems easier to follow and similar performance implications since this is only initialized once.

🤷 no idea what I was thinking when I wrote this. I agree that feels more clear to read. I cannot make time to make these changes now but I will when I get time to revisit this pr.

blakeembrey

We also need some tests for any of this behavior, including the concerns around prototype re-assignment not allowing multiple app instances to exist.

Can we get at least https tests and/or a simple proxy example of how someone would use this with e.g. one method working?

wesleytodd force-pushed the remove-http-dep branch from 20830c8 to 94cfec6 Compare February 20, 2017 17:14

wesleytodd force-pushed the remove-http-dep branch from 94cfec6 to 10fe874 Compare February 20, 2017 17:22

wesleytodd mentioned this pull request Feb 20, 2017

Small changes needed for uws compatibility #3205

Closed

wesleytodd mentioned this pull request Feb 20, 2017

Moves req/res out of the repo #3214

Open

dougwilson added 5.x pr labels Feb 20, 2017

wesleytodd mentioned this pull request Feb 20, 2017

Replace http.STATUS_CODES with the statuses module #3215

Closed

wesleytodd force-pushed the remove-http-dep branch from 10fe874 to ad70cbd Compare February 20, 2017 21:48

wesleytodd force-pushed the remove-http-dep branch from ad70cbd to ce4a7fb Compare February 20, 2017 22:04

dougwilson added the ideas label Feb 20, 2017

wesleytodd force-pushed the remove-http-dep branch from ce4a7fb to 0e0a8e7 Compare February 21, 2017 00:30

LinusU approved these changes Feb 21, 2017

View reviewed changes

dougwilson suggested changes Feb 22, 2017

View reviewed changes

wesleytodd force-pushed the remove-http-dep branch 2 times, most recently from cab07fb to d66bba4 Compare February 24, 2017 15:01

dougwilson mentioned this pull request Aug 21, 2017

Properly setting up UWS and Express #3400

Closed

UlisesGascon mentioned this pull request Apr 24, 2024

Express 5.0 - last push! expressjs/discussions#233

Open

33 tasks

wesleytodd force-pushed the remove-http-dep branch 4 times, most recently from bf3b980 to 6e32dae Compare May 17, 2024 21:08

Removed direct dependency on http module

aa90fc8

wesleytodd force-pushed the remove-http-dep branch from 6e32dae to aa90fc8 Compare May 17, 2024 21:14

wesleytodd requested a review from a team May 22, 2024 21:33

blakeembrey requested changes May 22, 2024

View reviewed changes

blakeembrey reviewed May 22, 2024

View reviewed changes

Remove direct http dependency #3213

Are you sure you want to change the base?

Remove direct http dependency #3213

Conversation

wesleytodd commented Feb 20, 2017

ghost commented Feb 20, 2017

wesleytodd commented Feb 20, 2017 • edited

wesleytodd commented Feb 20, 2017

wesleytodd commented Feb 20, 2017 • edited

ghost commented Feb 20, 2017

wesleytodd commented Feb 20, 2017

ghost commented Feb 20, 2017

dougwilson commented Feb 20, 2017

wesleytodd commented Feb 20, 2017 • edited

wesleytodd commented Feb 20, 2017

LinusU left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjsalinger commented May 4, 2017 • edited

dougwilson commented May 5, 2017

kibertoad commented Feb 11, 2018

wesleytodd commented Feb 13, 2018

LucyMaber commented May 20, 2023

wesleytodd commented May 14, 2024

wesleytodd commented May 14, 2024

kibertoad commented May 14, 2024

wesleytodd commented May 15, 2024

wesleytodd commented May 17, 2024

blakeembrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakeembrey May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakeembrey May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakeembrey left a comment

Choose a reason for hiding this comment

wesleytodd commented Feb 20, 2017 •

edited

wesleytodd commented Feb 20, 2017 •

edited

wesleytodd commented Feb 20, 2017 •

edited

mjsalinger commented May 4, 2017 •

edited

blakeembrey May 22, 2024 •

edited

blakeembrey May 22, 2024 •

edited