express
express copied to clipboard
expressjs
Move req / res prototype extension to a separate repo
Extraction of the logic is almost done, but we also need to decide on where to put the properties.
The suggestion was to bundle these into "packs".
List of req / res prototype properties in express 4.10.6:
// Request
[ 'header',
'get',
'accepts',
'acceptsEncodings',
'acceptsEncoding',
'acceptsCharsets',
'acceptsCharset',
'acceptsLanguages',
'acceptsLanguage',
'range',
'param',
'is',
'protocol',
'secure',
'ip',
'ips',
'subdomains',
'path',
'hostname',
'host',
'fresh',
'stale',
'xhr' ]
// Response
[ 'status',
'links',
'send',
'json',
'jsonp',
'sendStatus',
'sendFile',
'sendfile',
'download',
'type',
'contentType',
'format',
'attachment',
'header',
'set',
'get',
'clearCookie',
'cookie',
'location',
'redirect',
'vary',
'render' ]
The one very easy one is that render() should come with the views abstraction. (but it depends on send() still...)
As long as the req / res prototypes are being moved to separate repos, I wanted to ask if there was a compelling reason to keep these as middleware processes rather than just directly modifying the http streams before ever making a server?
- It would cut down on the amount of
__proto__replacement logic that has been brought up in issues before. - It would thin out the expressInit 'mandatory' middleware. It could be similar to some of the changes in the query middleware(?)
- The req & res objects, to be extended onto http, could still be exposed off of express (and be modular etc).
Stupid simple example: example.js
var _ = require('lodash');
var http = require('http');
var res = require('response');
_.extend(http.ServerResponse.prototype, res);
var server = http.createServer(function(request,response){
response.render('example',{'hello':'hello world'});
});
server.listen(8000);
I just tested a quick and dirty implementation with express by
- removing
req.__proto__ = app.request;andres.__proto__ = app.response;, in lib/middleware/init.js - adding the prototypes directly onto http, in application.js.
app.listen = function(){
// replace the servers response stream and request stream
http.IncomingMessage.prototype = this.request;
http.ServerResponse.prototype = this.response;
var server = http.createServer(this);
return server.listen.apply(server, arguments);
};
Is there a compelling reason to keep req.__proto__ = app.request; and res.__proto__ = app.response; as a middleware process? I've always thought the current middleware implementation was possibly slower, uses deprecated __proto__, and has more mental overhead to understand/modify.
No, we won't be removing the use of __proto__. The reason I these cannot be globals. Users should be able to have an Express server in the same process as a plain http server and the plain http server should not have any of the Express extensions visible to it, which is what would happen if you modified the global prototypes.
I've always thought the current middleware implementation was possibly slower, uses deprecated
__proto__, and has more mental overhead to understand/modify.
You can always benchmark it, but JavaScript doesn't provide any other good way to do this without global modifications. Other methods like calling a ton of Object.defineProperty on every request is slower.
@NickStefan the pillarjs module this is being extracted to is going to be public soon; I can assure you the implementation is far cleaner than the current. :)
@dougwilson Being able to have an unmodified http res req process alongside of an express app makes sense. I'm sure it gets old answering about the __proto__, but thanks for the explanation.
@Fishrock123 Thanks. Hadn't seen pillarjs yet. Looks promising.
@NickStefan Another reason is that certain modules (like node-http2) expect certain http res/req methods.
@yoshokatana , so node-http2 inherits its req from another IncomingMessage, right? And two prototype injections sorta conflict with each other?
We probably need a way to specify which class req/res should be inherited from then.
Yep. I'm not sure whose responsibility that is, though. I get why express uses its own req/res objects, but I also understand why node-http2 assumes it can inherit it. Not sure what the sanest convention should be in this case.
Regarding the http2 discussion, I put a comment over at their repo saying we are willing to make any changes/fixes necessary here if someone wants to open a new issue about this and perhaps an explanation of what the issue is :)
Yeah the only issue is that node-http2 has a different class (and because of this, different proto for instances) for requests and responses than the built-in HTTP1 implementation. Express changes the proto to an extended version of the built-in IncomingMessage. As per @dougwilson 's comment in https://github.com/molnarg/node-http2/issues/100, I will expect express 5.0 to solve this problem, and make a note about this in the node-http2 readme. Thanks :)
