morgan icon indicating copy to clipboard operation
morgan copied to clipboard

conflicting req/res scoped variable names

Open WORMSS opened this issue 8 years ago • 3 comments

I read that you guys are thinking of a version 2.0 for morgan on one of the other issues..

I didn't see a 2.0 branch so I wasn't sure what your plans are, but I was wondering if you would be implementing the ES6 Symbol technique as to not conflict with any other express middleware that could be trying to store _startTime and such on req/res.

So some something like

const START_TIME = Symbol("startTime");
const START_AT = Symbol("startAt");
const REMOTE_ADDRESS = Symbol("remoteAddress");
....
function recordStartTime() {
  this[START_AT] = process.hrtime()
  this[START_TIME] = new Date()
}
...
// And expose the Symbols incase other modules may want to hook into morgan
module.exports.START_TIME = START_TIME;
module.exports.START_AT = START_TIME;
module.exports.REMOTE_ADDRESS  = REMOTE_ADDRESS;
  • Colin

WORMSS avatar Apr 28 '17 14:04 WORMSS

That is a good idea. In order to use Symbol, it would have to wait until Morgan stopped supporting version of Node.js without Symbol support I would guess. There is no roadmap on that, but that's only because there hasn't been a compelling reason. I wonder if there is any other possible way to accomplish this without Symbols. If they are the only possible way to do so, then perhaps that would be a compelling reason :) !

Any thoughts on alternative solutions instead of Symbol?

dougwilson avatar May 17 '17 02:05 dougwilson

@dougwilson According to node.green, Symbol is available for use in node 0.12 and above. A lot of other node projects have started to drop support for node versions less than 4, so I don't think you'd be an outlier for following suite.

That said, it looks like you still maintains 0.10 compatibility in express, so I can understand wanting to look for alternatives to Symbol. I think there are some polyfills out there, but I've not used one myself.

danthegoodman avatar Oct 10 '17 13:10 danthegoodman

So I've been looking at #141 today and I believe the changes to fix that issue would ultimately fix this issue too, as it will move all these to private and instance based, so wouldn't conflict with other modules either.

dougwilson avatar May 06 '18 16:05 dougwilson