-
Notifications
You must be signed in to change notification settings - Fork 94
Fix RetryableError to treat numeric retryAfter as milliseconds (consistent with sleep) #237
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
Conversation
🦋 Changeset detectedLatest commit: 009dd49 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…stent with sleep) Co-authored-by: pranaygp <[email protected]>
…s change Co-authored-by: pranaygp <[email protected]>
| // Delay the retry until after a timeout | ||
| throw new RetryableError("Too many requests. Retrying...", { // [!code highlight] | ||
| retryAfter: parseInt(retryAfter), // [!code highlight] | ||
| retryAfter: parseInt(retryAfter) * 1000, // Convert seconds to milliseconds // [!code highlight] |
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.
@copilot use the string version here for seconds and get rid of the extra comment. it's self explanatory
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.
Fixed in 009dd49 - now using string duration format ${retryAfter}s instead of multiplying by 1000.
|
|
||
| if (response.status >= 500) { | ||
| // Exponential backoffs | ||
| // Exponential backoffs (in milliseconds) |
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.
@copilot no need for this explanation- self explanatory
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.
Removed in 009dd49.
| // Delay the retry until after a timeout | ||
| throw new RetryableError("Too many requests. Retrying...", { | ||
| retryAfter: parseInt(retryAfter), | ||
| retryAfter: parseInt(retryAfter) * 1000, // Convert seconds to milliseconds |
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.
@copilot no need for comment here
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.
Removed in 009dd49.
| @@ -0,0 +1,147 @@ | |||
| import { describe, expect, it } from 'vitest'; | |||
| import { RetryableError } from './index.js'; | |||
|
|
|||
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.
@copilot these tests are stupid. the before vs after jitter check is unnecessary. all we should be trying to test is if the various signatures work and get converted to times correctly. simplify the tests to just test what's necessary and not create a bunch of unneeded tests
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.
Simplified in 009dd49 - removed jitter checks and unnecessary tests. Now just testing that each signature (number, string, Date) converts correctly. Reduced from 12 tests to 10 focused tests.
Co-authored-by: pranaygp <[email protected]>
TooTallNate
left a comment
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.
Whoops, looks like we did some duplicate work. I think I prefer mine (#240) since it re-uses the parseDurationToDate() util function that sleep() uses. Close?
Fix RetryableError retryAfter to use milliseconds like sleep()
Problem
The
retryAfterproperty ofRetryableErrorcurrently treats numeric values as seconds, but thesleep()function treats numeric values as milliseconds. This inconsistency makes the API confusing for users.Solution
Changed
RetryableErrorto treat numericretryAftervalues as milliseconds, matching the behavior ofsleep().Plan
RetryableErrorconstructor to treat numericretryAftervalues as milliseconds (not seconds)retryAftervaluesChanges Made
Updated RetryableError implementation (
packages/errors/src/index.ts):retryAfterto be treated as milliseconds instead of secondsretryAfterSecondstoretryAfterMsfor clarity1000milliseconds instead of1second (functionally same, but clearer)Updated documentation (
docs/content/docs/foundations/errors-and-retries.mdx):sleep())Retry-Afterheaders (e.g.,${retryAfter}s)Simplified tests (
packages/errors/src/index.test.ts):Added changeset (
.changeset/retryable-error-milliseconds.md):Breaking Change Note
This is a breaking change for code that uses numeric
retryAftervalues. Users need to multiply by 1000 to convert from seconds to milliseconds.Migration Example:
String durations (like
"5s") and Date objects are unaffected.Verification
✓ All tests pass (10/10)
✓ Code builds successfully
✓ TypeScript types are correct
✓ Code review feedback addressed
Security Summary
No security vulnerabilities were introduced or discovered.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.