bittrex snapshot use sequence, https add headers to result #235
Conversation
| @@ -17,7 +17,10 @@ async function get(uri) { | |||
| if (res.statusCode !== 200) { | |||
| return reject(new Error(results.toString())); | |||
| } else { | |||
| return resolve(JSON.parse(results)); | |||
| const resultsParsed = JSON.parse(results); | |||
There was a problem hiding this comment.
I'm thinking it would be cleaner to have an options argument passed to http.get that signals you want a result object instead of the parsed result. The result object should include a result property with the parsed result and a headers property that includes the response headers. This way we are not taking the hit of a full properties copy and attaching a non-sematntic hidden property to it.
I would also be ok making a second method https.getResult that has the same logic as http.get but returns a result object as described above.
|
alrirght @bmancini55 new changes pushed |
|
I also noticed that your build requires prettier, but there isn't any easy way to have prettier fix things automatically so I added a |
There was a problem hiding this comment.
Awesome thanks for the changes!
I also noticed that your build requires prettier, but there isn't any easy way to have prettier fix things automatically so I added a format:fix script.
Perfect, thanks!
Added one nit if you have a minute to fix. Otherwise will merge shortly.
| let raw = await https.get(uri); | ||
| let { data, response } = await https.getResponse(uri); | ||
| let raw = data; | ||
| const sequence = +response.headers.sequence; |
There was a problem hiding this comment.
nit: prefer Number cast over +
fixes #220