-
-
Notifications
You must be signed in to change notification settings - Fork 366
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
feat(retry): allow respecting Retry-After for user-defined status codes #598
feat(retry): allow respecting Retry-After for user-defined status codes #598
Conversation
The test is just a copy of the existing 413 one, but with a 500 and required config instead. Closes #473.
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.
Could you also add a negative test, which verifies that Retry-After
is not respected by default for a 500
response?
test/retry.ts
Outdated
}); | ||
|
||
const result = await ky(server.url, {retry: {afterStatusCodes: [500]}}).text(); | ||
console.log(result); |
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.
console.log()
should be removed
test/retry.ts
Outdated
const retryAfterOn413 = 2; | ||
const lastTried500access = Date.now(); |
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 realize you're just being consistent with the other, existing variables here but it seems like these should be local variables within the tests where they are needed.
If we had compute-intensive or async setup hooks/macros that were await
ed before the tests, it could make a big difference as to what the start time is determined to be.
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.
Yeah, to be honest I had tunnel vision and barely paid attention beyond getting it working with the existing paradigm. Apologies!
const result = await ky(server.url, {retry: {afterStatusCodes: [500]}}).text(); | ||
console.log(result); | ||
t.true(Number(result) >= retryAfterOn500 * 1000); | ||
|
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.
Missing an assertion for the requestCount
.
Looks like some of the other retry tests also have this mistake.
readme.md
Outdated
|
||
If `retry` is a number, it will be used as `limit` and other defaults will remain in place. | ||
|
||
If the response provides an HTTP status contained in `afterStatusCodes`, Ky will wait until the date or timeout given in the [`Retry-After`](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After) header has passed to retry the request. If the provided status code is not in the list, the [`Retry-After`](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After) header will be ignored (retries sent immediately). |
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.
retries sent immediately
That really depends on the retry configuration, so I would drop this part.
You also need to update Lines 118 to 142 in e044626
|
Thanks for the review! I haven't had a lot of free time this past week, but I hope to get to this soon. Feel free to bump me if it gets stale, I do check notifications. |
How embarassing.
Made sure they were defined parametrically, where appropriate. Magic values on a per-test basis were left untouched.
Implemented as recommended in related issue, with appropriate testing and documentation.
The included test is just a copy of the existing 413 one, but with a 500 and required config instead.
Closes #473.