-
Notifications
You must be signed in to change notification settings - Fork 8
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
Supports [email protected] #165
Conversation
Bumps [axios](https://github.com/axios/axios) from 0.27.2 to 1.6.0. - [Release notes](https://github.com/axios/axios/releases) - [Changelog](https://github.com/axios/axios/blob/v1.x/CHANGELOG.md) - [Commits](axios/axios@v0.27.2...v1.6.0) --- updated-dependencies: - dependency-name: axios dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]>
if (error.isLambdaInvokeTimeout) return true; | ||
|
||
return error.code !== 'ECONNABORTED' && | ||
(!error.response || (error.response.status >= 500 && error.response.status <= 599)); | ||
return error.code !== 'ECONNABORTED' && isServerSideError(error); |
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.
Should this be !
isServerSideError
?
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.
No, I don't think so. My mistake
return ( | ||
error.response | ||
&& ( | ||
_inRange(_get(error.response, 'StatusCode', 0) as number, 500, 600) |
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.
Before the addition of StatusCode
checks, was this function failing to detect some failures?
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.
this function is refactored from isRetryableError()
. it checked the statusCode of http request via Axios API (AxiosResponse
) only.
These lines are for checking the status code of lambda function invocation via Lambda API (InvocationResponse
). I could remove this one if aws internal error during lambda invocation is not retryable.
return ( | ||
error.response | ||
&& ( | ||
_inRange(_get(error.response, 'StatusCode', 0) as number, 500, 600) |
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.
this function is refactored from isRetryableError()
. it checked the statusCode of http request via Axios API (AxiosResponse
) only.
These lines are for checking the status code of lambda function invocation via Lambda API (InvocationResponse
). I could remove this one if aws internal error during lambda invocation is not retryable.
Progressafter bd7aa1c it will pass all unit tests (fell short on code coverage a little bit).
|
e7c5ddd
to
cbdd5e9
Compare
cbdd5e9
to
20c701b
Compare
Can you push a commit marked as breaking so the version number will be bummed? |
f2457c7
to
eefbbf1
Compare
eefbbf1
to
c042dae
Compare
will do in a coming commit that fixes types. |
Merging this PR will result in a major version bump. Created by lifeomic-probot (Enforce Semantic Commits) |
src/types.ts
Outdated
@@ -14,7 +14,7 @@ type SignatureV4Constructor = SignatureV4Init & SignatureV4CryptoInit; | |||
type SignatureV4Optionals = 'credentials' | 'region' | 'sha256' | 'service'; | |||
|
|||
export interface AlphaResponse<ResponseData = any, ConfigData = any> extends AxiosResponse<ResponseData> { | |||
config: AlphaOptions<ConfigData>; | |||
config: AlphaOptions<ConfigData> & InternalAxiosRequestConfig; |
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.
This change means it could re-use AlphaOptions
as the type of config in AlphaResponse
but response headers always exist.
"@lifeomic/eslint-config-standards": "^3.0.0", | ||
"@lifeomic/jest-config": "^1.1.2", | ||
"@lifeomic/typescript-config": "^1.0.3", | ||
"@lifeomic/eslint-config-standards": "3.0.0", |
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 locked it to 3.0.0
to be able to run yarn lint
on node.js v14.
Notice that it will need to port this change to |
I had the same thought, but it's a bummer that the upgrade is a breaking change. Ideally we'd somehow introduce a major version change between 4 and 5 😄 |
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.
Nice. This upgrade has been a long time coming. Thanks for doing the tedious work!
🎉 This PR is included in version 6.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Follow up of #164
There're several break changes on types and API.
it will need to implement
axios/lib/helpers/buildURL
since it could not be imported from axios anymore. see kuitos/axios-extensions#99