-
-
Notifications
You must be signed in to change notification settings - Fork 134
feat: Preserve some non-enumerable properties from request #379
Conversation
9913f3b
to
8504064
Compare
lib/utils.js
Outdated
@@ -56,6 +56,22 @@ module.exports.getAuthHeader = function getAuthHeader(timestamp, apiKey, apiSecr | |||
return header.join(', '); | |||
}; | |||
|
|||
module.exports.prepareInitialRequest = function prepareInitialRequest() { | |||
var request = {}; | |||
var nonEnumberables = ['ip']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be passed as an argument, but I wasn't sure if that's worth it, as it's one-off util.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks like a good fix, but it's still slightly confusing and I think the comments + test would help.
lib/client.js
Outdated
@@ -228,14 +228,29 @@ extend(Raven.prototype, { | |||
so we only parse a `req` property if the `request` property is absent/empty (and hence we won't clobber) | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was just me missing it. Fixed :)
test/raven.utils.js
Outdated
@@ -98,6 +98,66 @@ describe('raven.utils', function() { | |||
}); | |||
}); | |||
|
|||
describe('#prepareInitialRequest', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
8504064
to
6ad56e6
Compare
@MaxBittker everything has been addressed |
6ad56e6
to
a9f3fb3
Compare
Fixes #360