Geolocation #81
Geolocation #81
Conversation
lib/internal/make-api-call.js
Outdated
| // Geolocation API which uses HTTP POST for its request, and HTTP | ||
| // status codes for determining success. | ||
| var isSuccessResponse = function(response) { | ||
| if (!isPost) { |
There was a problem hiding this comment.
I have a slight preference for
if (isPost) {
...
} else {
...
}
I had to read this twice because the ! is easy to miss.
lib/internal/make-api-call.js
Outdated
| }); | ||
| if (isPost) { | ||
| // Remove key from query for HTTP POST, since query is the JSON being posted. | ||
| delete query.key; |
There was a problem hiding this comment.
Just to check I've understood, this will mutate the object that was passed as a parameter, and the caller is relying on that? That's very subtle.
I'd much prefer if we could move this logic about POST out of this function, so that it continues to just take the query, return the URL, and not have side-effects.
lib/internal/make-api-call.js
Outdated
| @@ -156,10 +175,15 @@ exports.inject = function(options) { | |||
| throw 'Missing either a valid API key, or a client ID and secret'; | |||
| } | |||
|
|
|||
| var isPost = method == 'POST'; | |||
There was a problem hiding this comment.
Use triple equals
===
lib/index.js
Outdated
|
|
||
| var makeApiMethod = function(apiConfig) { | ||
| return function(query, callback, customParams) { | ||
| query = apiConfig.validator(query); | ||
| query.supportsClientId = apiConfig.supportsClientId !== false; | ||
| query.method = apiConfig.method; |
There was a problem hiding this comment.
I have a preference for making the method an extra parameter in makeApiCall(), rather than adding it as a query parameter that you have to later delete.
It's not really a query param.
If we're worried that makeApiCall() is getting too many arguments, we can make the third arg an options object, containing both useClientId and method.
lib/internal/make-api-call.js
Outdated
|
|
||
| function rateLimitedGet() { | ||
| return requestQueue.add(function() { | ||
| return Task.start(function(resolve, reject) { | ||
| return makeUrlRequest(requestUrl, resolve, reject); | ||
| return makeUrlRequest(requestUrl, resolve, reject, postData); |
There was a problem hiding this comment.
It worries me a little that we only support GET and POST, but never check that method isn't anything else.
Should makeUrlRequest take the method, as well as the postData ?
lib/internal/make-api-call.js
Outdated
| @@ -93,6 +98,7 @@ exports.inject = function(options) { | |||
| until: function(response) { | |||
| return !( | |||
| response == null | |||
| || response.status === 403 // Geolocation API only. | |||
There was a problem hiding this comment.
Also, I didn't understand this bit. It looks like it happens for all APIs, not just geolocation. And we shouldn't retry for 403 generally.
|
@stephenfarrar I made all the changes we discussed offline in 5ef38ec apart from the order of arguments in |
No description provided.