-
Notifications
You must be signed in to change notification settings - Fork 59
feat: rate limiting information for all responses #562
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
| if (!limitHeader || !remainingHeader || !resetHeader) { | ||
| throw new Error( | ||
| "The rate limit headers are not present in the response, something must've gone wrong, please email us at [email protected]", | ||
| ); | ||
| } |
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.
What if we gracefully fail here? This would be a big problem if we change how the rate limit works in the API
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.
that function is only called from
Lines 60 to 62 in 983cc6a
| const rateLimiting = parseRateLimit(response.headers); | |
and there it errors gracefully, but with an error message that isn't very clear. we could return something else for when it isn't able to get the rate limiting headers, I also thought of just warning, but my worry with not erroring here is that it could break workflows for users silently
imo the best would be to have tests on the SDK's side that tested things directly with the production API ensuring that this rate limiting is working so it would fail early, instead of doing mocking
This reverts commit ee0d2a1.
Closes #189.
Adds a type to use on all responses to avoid repeating the definition for it, and add a
rateLimitingfield there that gives access to theratelimit-limit,ratelimit-resetandratelimit-remainingheaders.The actual changes to use the new
Responsetype were made by claudinho, so take that portion with a grain of saltHere's how an example of the API would feel like:
Unfortunately, there are a few cases that are errors that happen on the client and not on the server, so if
erroris not null,rateLimitingcan be null. Ideally the user would not need to handle that, but seems like it's unavoidable.Additionally, this also adds in a
retryAfterproperty to the ErrorResponse when thenameis 'rate_limit_exceeded' taken from theretry-afterheader that only comes on 429 responses. It feels like this: