-
Notifications
You must be signed in to change notification settings - Fork 81
Change RetryableError "retryAfter" option number value to represent milliseconds instead of seconds
#240
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: faaf9eb The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 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.
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
| if (response.status === 429) { | ||
| const retryAfter = response.headers.get('Retry-After'); | ||
| // Customize the duration before retrying | ||
| // Customize the duration before retrying (assume "Retry-After" is an ISO string) |
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.
retry after header js meant to be either seconds, or a HTTP Date format - not ISO
https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Retry-After
.changeset/cute-drinks-swim.md
Outdated
| "@workflow/core": patch | ||
| --- | ||
|
|
||
| Change `RetryableError` "retryAfter" option number value to represent milliseconds instead of seconds |
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.
let's emphasize the breaking change nature. I wanna get in the habit of doing that so that LLMs can easily do migrations (and create migration guides)
| throw new RetryableError( // [!code highlight] | ||
| `Rate limited by ${url}`, // [!code highlight] | ||
| { retryAfter: `${retryAfter} seconds` } // [!code highlight] | ||
| { retryAfter: new Date(retryAfter) } // [!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.
same
| // Delay the retry until after a timeout (assume "Retry-After" is an ISO string) | ||
| throw new RetryableError("Too many requests. Retrying...", { // [!code highlight] | ||
| retryAfter: parseInt(retryAfter), // [!code highlight] | ||
| retryAfter: new Date(retryAfter), // [!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.
same
|
|
||
| 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.
redundant comment
… milliseconds instead of seconds
e3c655b to
faaf9eb
Compare
d0618bc to
bf170ad
Compare

Changed
RetryableErrorto use milliseconds instead of seconds for numericretryAftervalues.What changed?
RetryableErrorclass to interpret numericretryAftervalues as milliseconds instead of seconds@workflow/utilspackage to use theparseDurationToDatefunctionHow to test?
RetryableErrorwith differentretryAfterformats:Why make this change?
Using milliseconds as the unit for numeric time values is more consistent with JavaScript conventions (like
setTimeoutand other timing functions). This change makes the API more intuitive for JavaScript developers and aligns with standard practices in the ecosystem.