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.
test/retry.ts
Outdated
| }); | ||
|
|
||
| const result = await ky(server.url, {retry: {afterStatusCodes: [500]}}).text(); | ||
| console.log(result); |
There was a problem hiding this comment.
console.log() should be removed
test/retry.ts
Outdated
| const retryAfterOn413 = 2; | ||
| const lastTried500access = Date.now(); |
There was a problem hiding this comment.
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 awaited 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.
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.
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.
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.