Add explicit handling for rate-limit errors #1033
Conversation
| @@ -0,0 +1,5 @@ | |||
| export interface RequestException { | |||
There was a problem hiding this comment.
In my initial pass I realized the new RateLimitExceededException was missing status. It seemed worth having something like this in order for all exception classes to be roughly shaped the same.
Arguable we should have a base-class instead, but I opted for an interface for now since it only adds an additional compile-time check, and we can decide later.
| case 429: { | ||
| const retryAfter = headers.get('Retry-After'); | ||
|
|
||
| throw new RateLimitExceededException( | ||
| data.message, | ||
| requestID, | ||
| retryAfter ? Number(retryAfter) : null, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Is this a breaking change?
Existing users may be doing an error instanceof GenericServerException when attempting to handle rate limit errors, which would no longer work with the introduction of this new exception.
There was a problem hiding this comment.
Actually, one option would be to make GenericServerException the actual base-class that the other exceptions extend from. It can enforce that rough shape we want all of them to have, and would mean this is no longer a potential breaking-change.
Though I'd like to hear opinions before I make that change.
EDIT: It might be a breaking change either way since even if the base class stays the same, the name will not, and folks may be relying on that, too.
There was a problem hiding this comment.
Strongly prefer having GenericServerException be the base-class for inheritance to make this not be a breaking change. We want to generally shy away from releasing major versions too frequently.
While someone out there might be relying on the name, I feel that that's a bit of an anti-pattern over checking instanceof. I'm comfortable with releasing this as a minor version instead of a major.
There was a problem hiding this comment.
I made the minimal change of only having the new exception inherit from GenericServerException, though a large change could be to have all of the requests also inherit (and left a note about it): 4af4a79.
| case 429: { | ||
| const retryAfter = headers.get('Retry-After'); | ||
|
|
||
| throw new RateLimitExceededException( | ||
| data.message, | ||
| requestID, | ||
| retryAfter ? Number(retryAfter) : null, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Strongly prefer having GenericServerException be the base-class for inheritance to make this not be a breaking change. We want to generally shy away from releasing major versions too frequently.
While someone out there might be relying on the name, I feel that that's a bit of an anti-pattern over checking instanceof. I'm comfortable with releasing this as a minor version instead of a major.
| it('throws a RateLimitExceededException', async () => { | ||
| fetchOnce( | ||
| { | ||
| message: 'Too many requests', |
There was a problem hiding this comment.
Is this verbatim what we return from the API? I'd like there to be more information in here, like a link to the rate limiting docs or a callout to reach out on support@ if they want to discuss higher rate limits.
There was a problem hiding this comment.
It is, and agreed that we should have more information in the message. Separately, I can look into updating that, adding the additional details you mentioned, so that all SDK's (or no SDK at all) will have the extra context.
| }, | ||
| { | ||
| status: 429, | ||
| headers: { 'X-Request-ID': 'a-request-id', 'Retry-After': '10' }, |
There was a problem hiding this comment.
Is 10 seconds the default or is that just for this test? If it's the default returned by the API then that feels way too high.
There was a problem hiding this comment.
It's just a test value. IIRC, the actual Retry-After is generally larger, and varies by endpoint.
Introduces a new `RateLimitExceededException` to be thrown when a `429` error is encountered, rather than defaulting to a more generic error exception class. This also allows the reading of the `Retry-After` header, which is helpful for clients to retry, as well as debugging. There's also some light refactoring included. Does this require changes to the WorkOS Docs? E.g. the [API Reference](https://workos.com/docs/reference) or code snippets need updates. ``` [ ] Yes ``` If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.
Description
Introduces a new
RateLimitExceededExceptionto be thrown when a429error is encountered, rather than defaulting to a more generic error exception class.This also allows the reading of the
Retry-Afterheader, which is helpful for clients to retry, as well as debugging.There's also some light refactoring included.
Documentation
Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.
If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.