-
-
Notifications
You must be signed in to change notification settings - Fork 73
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
Add function to manually check rate limit (#346) #392
base: main
Are you sure you want to change the base?
Conversation
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 reviewed 2 out of 2 changed files in this pull request and generated no suggestions.
Comments skipped due to low confidence (3)
index.js:285
- The variable timeWindowString is only assigned a value if params.timeWindow is a number. Ensure it is always defined or handle the undefined case properly.
timeWindowString = ms.format(params.timeWindow, true)
index.js:193
- [nitpick] The function name createLimiterArgs is ambiguous. Consider renaming it to generateLimiterArguments for better clarity.
function createLimiterArgs (pluginComponent, globalParams, options) {
index.js:216
- Ensure that the new behavior introduced by the applyRateLimit function is covered by tests.
async function applyRateLimit (pluginComponent, params, req) {
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.
Looks good! Finish it up (be careful that CI is failing).
Hi @mcollina, after @umakantp has closed their PR #410 because of the "authorship problem", I thought of putting more pressure on myself to finally get this done. I added unit tests, type tests, as well as documentation about the usage of this feature. I also merged the latest commit of At this point, I also would like to appreciate @umakantp again for continuing this PR and putting time and effort into it. Please excuse me if I caused any trouble on your side. |
Hi @mcollina If you can please take a look and merge if things are good? |
It looks interesting, I'll take a look at this tomorrow |
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.
Technically this change is awesome!
A minor suggestion, but LGTM!
if (!fastify.hasDecorator('createRateLimit')) { | ||
fastify.decorate('createRateLimit', (options) => { | ||
const args = createLimiterArgs(pluginComponent, globalParams, options) | ||
return (req) => applyRateLimit(...args, req) |
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.
Instead of decostruct args
we can:
return (req) => applyRateLimit(...args, req) | |
return (req) => applyRateLimit.apply(this, args.concat(req)) |
(and even better, avoid the concat
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.
Can you run a benchmark using the basic example in the examples folder with autocannon
?
https://github.com/fastify/fastify-rate-limit/blob/main/example/example-simple.mjs
Hey fastify contributors,
Following on from issue #346 and my comment, I finally had some spare time to implement a draft version of my idea as suggested by @mcollina. I realized that any store (child) instance receives all the
options
provided tofastify.rateLimit
when it is constructed, which is why I had to adjust my initial idea and come up with a slightly different solution.I did not provide any documentation or additional tests yet (all existing tests run green tho), because I wanted to ask for your feedback first.
Please let me know what you think and as soon as everything looks good to you, I will finalize my PR with proper docs and tests.
Thanks and cheers
Patrick
How to Use
Checklist
npm run test
andnpm run benchmark
and the Code of conduct