Build new version of HTTP client and logging #111
Conversation
mattmeyerink
left a comment
mattmeyerink
left a comment
There was a problem hiding this comment.
So much better than what we had before! Overall I like the approach of passing through the axiosConfig and then letting axiosRetry handle the retry logic.
Left a few small comments and some broader questions to talk about in the next sync.
package.json
Outdated
| "axios-retry": "^4.5.0", | ||
| "bluebird": "^3.5.0", | ||
| "extend": "^3.0.2", | ||
| "lodash": "^4.17.21", |
There was a problem hiding this comment.
Why are we importing lodash?
There was a problem hiding this comment.
I had pulled it in at one point for some utilities, and while i'd love to migrate off underscore, i'll just remove for now.
| export const shouldRetry = (error: AxiosError<SmartsheetErrorResponseData>): boolean => { | ||
| // If we have a response with an error code, check if it's retryable | ||
| const responseData = error.response?.data; | ||
|
|
||
| if (responseData?.errorCode) { | ||
| const errorCode = responseData.errorCode; | ||
| return ( | ||
| errorCode === errorCodes.RATE_LIMIT || | ||
| errorCode === errorCodes.GATEWAY_TIMEOUT || | ||
| errorCode === errorCodes.INTERNAL_SERVER_ERROR || | ||
| errorCode === errorCodes.SERVICE_UNAVAILABLE | ||
| ); | ||
| } | ||
|
|
||
| // Default to axios-retry's default retry condition | ||
| return axiosRetry.isNetworkOrIdempotentRequestError(error); |
There was a problem hiding this comment.
So much nicer than the old retry logic 😄
| export const createClient: CreateClient = function (clientOptions) { | ||
| const requestor = buildRequestor(clientOptions); | ||
|
|
||
| const options: CreateOptions = { | ||
| apiUrls: apiUrls, | ||
| requestor: requestor, | ||
| clientOptions: { | ||
| accessToken: clientOptions.accessToken || process.env.SMARTSHEET_ACCESS_TOKEN, | ||
| userAgent: clientOptions.userAgent, | ||
| baseUrl: clientOptions.baseUrl, | ||
| }, | ||
| }; | ||
|
|
||
| return { | ||
| constants: require('./lib/utils/constants.js'), | ||
| contacts: require('./lib/contacts/').create(options), | ||
| events: createEvents(options), | ||
| favorites: require('./lib/favorites/').create(options), | ||
| folders: require('./lib/folders/').create(options), | ||
| groups: require('./lib/groups/').create(options), | ||
| home: require('./lib/home/').create(options), | ||
| images: require('./lib/images/').create(options), | ||
| reports: require('./lib/reports/').create(options), | ||
| request: require('./lib/request/').create(options), | ||
| search: require('./lib/search/').create(options), | ||
| server: require('./lib/server/').create(options), | ||
| sheets: require('./lib/sheets/').create(options), | ||
| sights: require('./lib/sights/').create(options), | ||
| templates: require('./lib/templates/').create(options), | ||
| tokens: require('./lib/tokens/').create(options), | ||
| users: require('./lib/users/').create(options), | ||
| webhooks: require('./lib/webhooks/').create(options), | ||
| workspaces: require('./lib/workspaces/').create(options), | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Part of the discussion surface in the events module section of the changes. I'm not sure which way we want to go, but we may want to retain this legacy path until the new createApiClient is validated and good to ship in the case that we want to roll them out side by side.
lib/client/buildHttpRequestor.ts
Outdated
| 'Content-Type': 'application/json', | ||
| 'User-Agent': `smartsheet-javascript-sdk/${version}`, | ||
| Authorization: `Bearer ${fullConfiguration.smartsheetClientConfig.accessToken}`, | ||
| ...(fullConfiguration.axiosConfig.headers || {}), |
There was a problem hiding this comment.
We probably want to prevent them from overriding the User-Agent and Authorization headers so those could be moved below the spread of the axoisConfig.headers.
There was a problem hiding this comment.
In this case I think I prefer splat then override to be very clear which headers we are hard setting. I think we should be very prescriptive with the User-Agent and Authorization. Accept and Content-Type should be overridable though.
lib/client/buildHttpRequestor.ts
Outdated
| baseURL: fullConfiguration.smartsheetClientConfig.apiHost, | ||
| headers: defaultHeaders, | ||
| // Set all axios options (excluding headers as they were previously set) | ||
| ...restAxiosConfig, |
There was a problem hiding this comment.
I think we would want the spread at the top of the function to make sure the headers we add are not overwritten.
There was a problem hiding this comment.
i pulled headers out of the axiosConfig on line 11 to avoid that, was there something else you were worried about clobbering or are you just saying simpler to splat and then override.
IDK if there's some value in "intention" of pulling out headers vs splat and override.
There was a problem hiding this comment.
I completely missed line 11 my bad! I typically default to splat and override, but you are right headers are the only thing we really need to be careful with here so I'm good with either.
lib/client/buildHttpRequestor.ts
Outdated
| createInternalRequestLogger(fullConfiguration.loggingConfig.loggerInstance) | ||
| ); | ||
|
|
||
| configureInterceptors(axiosClient, loggingCallbacks); |
There was a problem hiding this comment.
This could just be withLogging or withLogCallbacks
lib/client/logging/utils.ts
Outdated
| @@ -0,0 +1,57 @@ | |||
| import { AxiosRequestConfig } from 'axios'; | |||
There was a problem hiding this comment.
put the getHttpVerb back into the internalRequestLogger and name this file logSanitizer
smar-ben-feigin
force-pushed
the
integration/v5
branch
from
b1e03f2 to
aaa70a3
Compare
No description provided.