New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add http and https agent support #15
Conversation
@bchengwfd thanks for your contribution! I think this PR highlights the huge amount of repetition in class Bynder {
constructor(options) {
this.api = new API({
baseUrl,
httpAgent,
httpsAgent,
consumerToken,
accessToken
});
}
getCollection({ id }) {
this.api.get(`v4/collections/${id}`);
}
addMediaToCollection({ id, data }) {
this.api.post(`v4/collections/${id}/media/`, { data });
}
...
} |
Hi @osener I have done some refactoring to the Bynder and APICall class without changing the code structure too much to make it a bit more tidy. |
This is great, thank you! |
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.
Thanks for the refactoring, it makes the code way more readable indeed! Left some notes on commented code and small inaccuracies, nothing major.
@@ -90,21 +91,27 @@ class APICall { | |||
* @return {Promise} - Returns a Promise that, when fulfilled, will either return an JSON Object with the requested | |||
* data or an Error with the problem. | |||
*/ | |||
send() { | |||
send(method, url, dataObj) { |
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.
Can you please update the documentation for this method?
* @param {Object} options - An object containing the consumer keys, access keys and the base URL. | ||
*/ | ||
constructor(options) { | ||
this.consumerToken = options.consumer; | ||
this.accessToken = options.accessToken; | ||
// this.consumerToken = options.consumer; |
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.
Can you please remove these comments?
); | ||
return request.send(); | ||
return this.api.send('POST', 'v4/metaproperties/', { data: JSON.stringify(object) }); | ||
// The API requires an object with the query content stringified inside |
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.
I think it makes more sense that the comment goes above the return statement since it makes the line too big.
@@ -38,16 +38,17 @@ class APICall { | |||
* Create a APICall. | |||
* @constructor | |||
* @param {string} baseURL - A string with the base URL for account. | |||
* @param {string} url - A string with the name of the API method. | |||
* @param {string} method - A string with the method of the API call. | |||
* @param {string} httpsAgent - A https agent. |
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.
An http(s) agent is an object, right?
This change fixes a small bug introduced in #15
Add support for http and https-agent in Axios