Skip to content
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

conflicting req/res scoped variable names #142

Open
WORMSS opened this issue Apr 28, 2017 · 3 comments
Open

conflicting req/res scoped variable names #142

WORMSS opened this issue Apr 28, 2017 · 3 comments

Comments

@WORMSS
Copy link

WORMSS commented Apr 28, 2017

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
@dougwilson
Copy link
Contributor

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?

@danthegoodman
Copy link

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

@dougwilson
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants