Pass timeout param to request.js #27
Conversation
|
❌ Author of the following commits did not sign a Contributor Agreement: Please, read and sign the above mentioned agreement if you want to contribute to this project |
|
Thanks, I'll review this today. Please sign the CLA |
|
I signed it yesterday, am not sure why the bot is still marking it as incomplete. |
There was a problem hiding this comment.
I made one comment about the current API.
Additionally, the way people typically instantiate the client is with the AppSearchClient class, which means you'd need to update that as well to accept a timeout.
That's a little tricky, since we already have an optional third parameter.
I suppose we could change the third parameter to be a configuration object, with backwards compatible support for the string baseUrlFn.
constructor(accountHostKey, apiKey, options = {}) {
const barUrlFn = options.baseUrlFn || DEFAULT_BASE_URL_FN
const baseUrl = baseUrlFn(accountHostKey)
const { timeout } = options
this.client = new Client(apiKey, baseUrl, options)
}We'd also need to update the README to show the new usage.
const client = new AppSearchClient(undefined, apiKey, { baseUrlFn })And also a separate example showing setting a timeout
const client = new AppSearchClient(undefined, apiKey, { baseUrlFn, timeout })Does that make sense?
| @@ -42,11 +42,12 @@ function getErrorMessages(responseBody) { | |||
| } | |||
| } | |||
| class Client { | |||
| constructor(apiKey, baseUrl) { | |||
| constructor(apiKey, baseUrl, timeout) { | |||
There was a problem hiding this comment.
Thank you so much for making this change. This is great. Could you change this to be an optional config object? That just future proofs this method in case we have additional configuration needs.
| constructor(apiKey, baseUrl, timeout) { | |
| constructor(apiKey, baseUrl, { timeout } = {}) { |
Then later:
this.requestConfig = { timeout };And finally:
_jsonRequest(method, path, params) {
return this._wrap({
method: method,
url: `${this.baseUrl}${path}`,
json: params,
auth: {
bearer: this.apiKey
},
headers: {
'X-Swiftype-Client': this.clientName,
'X-Swiftype-Client-Version': this.clientVersion,
'Content-Type': 'application/json'
},
{ ...this.requestConfig }
})
}
|
Also, see if you can sign the CLA one more time ... maybe you mistyped your Github name or email? If that doesn't work we might need to open a new PR. Sorry for the inconvenience. Again, I really appreciate this PR. |
Solves for the problem brought up in issue #26
Fixes #26