feat: Preserve some non-enumerable properties from request #379
Conversation
kamilogorek
force-pushed
the
preserve-nonenums
branch
from
9913f3b to
8504064
Compare
lib/utils.js
Outdated
|
|
||
| module.exports.prepareInitialRequest = function prepareInitialRequest() { | ||
| var request = {}; | ||
| var nonEnumberables = ['ip']; |
There was a problem hiding this comment.
This could be passed as an argument, but I wasn't sure if that's worth it, as it's one-off util.
MaxBittker
left a comment
MaxBittker
left a comment
There was a problem hiding this comment.
this looks like a good fix, but it's still slightly confusing and I think the comments + test would help.
lib/client.js
Outdated
| parseUser returns a partial kwargs object with a `request` property and possibly a `user` property | ||
| */ | ||
| var initialRequest = utils.prepareInitialRequest( | ||
| this._globalContext.request, |
There was a problem hiding this comment.
would you mind adding some comments here? I'm having trouble understanding why it is needed to use both prepareInitialRequest and extend on these groups of request & req objects, instead of replacing in extend with prepareInitialRequest
There was a problem hiding this comment.
This was just me missing it. Fixed :)
test/raven.utils.js
Outdated
| }); | ||
| }); | ||
|
|
||
| describe('#prepareInitialRequest', function() { |
There was a problem hiding this comment.
these tests on the prepareInitialRequest utility seem good to me, but would you mind writing a slightly broader test that recreates the original problem outlined in #360?
kamilogorek
force-pushed
the
preserve-nonenums
branch
from
8504064 to
6ad56e6
Compare
|
@MaxBittker everything has been addressed |
kamilogorek
requested review from
a team
and removed request for
benvinegar
kamilogorek
force-pushed
the
preserve-nonenums
branch
from
6ad56e6 to
a9f3fb3
Compare
Fixes #360