Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Commit

Permalink
If "rawP" option is passed to internal request.request...
Browse files Browse the repository at this point in the history
…don’t try to parse JSON — this is the cause of the mysterious
“ledger-geoip warning: {}” messages!
  • Loading branch information
mrose17 committed Dec 20, 2016
1 parent ef4eab7 commit bb56af5
Showing 1 changed file with 2 additions and 1 deletion.
3 changes: 2 additions & 1 deletion app/ledger.js
Original file line number Diff line number Diff line change
Expand Up @@ -1127,6 +1127,7 @@ var roundtrip = (params, options, callback) => {
var parts = typeof params.server === 'string' ? url.parse(params.server)
: typeof params.server !== 'undefined' ? params.server
: typeof options.server === 'string' ? url.parse(options.server) : options.server
var rawP = options.rawP

if (!params.method) params.method = 'GET'
parts = underscore.extend(underscore.pick(parts, [ 'protocol', 'hostname', 'port' ]),
Expand Down Expand Up @@ -1170,7 +1171,7 @@ var roundtrip = (params, options, callback) => {
}

try {
payload = (response.statusCode !== 204) ? JSON.parse(body) : null
payload = rawP ? body : (response.statusCode !== 204) ? JSON.parse(body) : null
} catch (err) {
return callback(err)
}
Expand Down

3 comments on commit bb56af5

@mrose17
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auditor: @bsclifton

@bsclifton
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 😄 If you wanted to be consistent, you could make it match the one here (or expose a wrapper for the check in the ledger-geoip module):
https://github.com/brave/ledger-geoip/blob/master/index.js#L246

looks like:
payload = (options.rawP) ? body : (response.statusCode !== 204) ? JSON.parse(body) : null

@mrose17
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, but the problem is that rawP is stripped out of the options by that point...

Please sign in to comment.