-
Notifications
You must be signed in to change notification settings - Fork 0
Add rate limiting support #2
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
niemyjski
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.
Overall looks good. I'm wondering how I would use a middleware to update existing rate limit config based on header values for both rate limit remaining and reate limit reset time
I don't really see it as something we should be updating dynamically. You can do that by just going to the provider.rateLimiter.config but this is just really meant more to keep us from DoS ourselves due to bad reactive code. |
|
Our api returns the rare limit in real time, would be nice to use that. Also some routes are not rate limited. |
|
Yeah, that would be kind of nice to respond to that. Although, wasn't it just for posting events? |
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.
Pull Request Overview
This PR adds support for rate limiting by introducing a new RateLimiter class and middleware, integrating it across the provider and helper APIs, and updating documentation and tests accordingly.
- Added
RateLimiterclass andcreateRateLimitMiddlewareinsrc/RateLimiter.ts - Integrated rate limiting into
FetchClientProviderandDefaultHelpers - Expanded tests, exports in
mod.ts, documentation inreadme.md, and updated build/config files
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/RateLimiter.ts | New rate limiter implementation and middleware |
| src/RateLimiter.test.ts | Unit tests for limiting logic and middleware behavior |
| src/FetchClientProvider.ts | Added enableRateLimit method and provider integration |
| src/FetchClient.test.ts | Updated Zod import and schema type for zod/mini |
| src/DefaultHelpers.ts | Exposed global enableRateLimit helper |
| scripts/build.ts | Removed importMap from build configuration |
| readme.md | Documented rate limiting in README |
| mod.ts | Exported RateLimitConfig and RateLimiter |
| deno.json | Bumped dependency versions |
| .github/copilot-instructions.md | Added Copilot instructions |
Comments suppressed due to low confidence (4)
src/RateLimiter.ts:159
- The
getTimeUntilNextRequestmethod isn't covered by existing tests; add unit tests to verify that it returns correct millisecond durations and integrates with theRetry-Afterheader.
public getTimeUntilNextRequest(): number {
src/FetchClientProvider.ts:201
- [nitpick] Consider returning
thisfromenableRateLimitto allow method chaining (e.g.,provider.enableRateLimit(...).useMiddleware(...)).
public enableRateLimit(config: RateLimitConfig) {
readme.md:10
- [nitpick] The table entry uses lowercase 'Rate limiting' while the section header is 'Rate Limiting'; consider matching the capitalization for consistency.
* [Rate limiting](#rate-limiting)
| blob: () => Promise.resolve(new Blob()), | ||
| formData: () => Promise.resolve(new FormData()), | ||
| clone: () => { | ||
| throw new Error("Not implemented"); |
Copilot
AI
Jul 17, 2025
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.
The clone method currently throws a generic error; consider implementing a proper clone or removing this unimplemented stub to avoid runtime failures if clone() is used.
| throw new Error("Not implemented"); | |
| return { | |
| ...context.response, | |
| headers: new Headers(context.response.headers), | |
| problem: context.response.problem | |
| ? { ...context.response.problem } | |
| : null, | |
| json: () => Promise.resolve({ ...context.response.problem }), | |
| text: () => Promise.resolve(JSON.stringify(context.response.problem)), | |
| arrayBuffer: () => Promise.resolve(new ArrayBuffer(0)), | |
| bytes: () => Promise.resolve(new Uint8Array()), | |
| blob: () => Promise.resolve(new Blob()), | |
| formData: () => Promise.resolve(new FormData()), | |
| clone: context.response.clone, // Reference the same clone method | |
| }; |
We rate limit every endpoint but a few: https://github.com/exceptionless/Exceptionless/blob/main/src/Exceptionless.Web/Utility/Handlers/ThrottlingMiddleware.cs#L107 |
|
Closing this one in favor of #3 |
|
should we delete this branch or are you looking at it? |
No description provided.