-
-
Notifications
You must be signed in to change notification settings - Fork 220
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
feat: allow the message
option to be a function
#311
feat: allow the message
option to be a function
#311
Conversation
Hi @colderry, Thanks for taking the time to make this PR! Could you please describe a use case that would benefit from making message a function? The use case that you have added to the readme is already possible by setting the |
Also, I love how you planned and split the changes into three commits :] |
It can be beneficial for someone who wants to check who is making these requests on their app and ban them. Another use case is if someone wants to log these requests, they can do it. |
Hey @colderry, thanks for taking the time to do this, and for the thorough job documenting and testing it! This seems seems very similar to the That said, I like the idea allowing However, I think that it would make more sense and be more consistent with the rest of the API to allow Also, So something like this: const message: any = (typeof options.message === 'function') ? await options.message(request, response) : message; But, we should probably also guard against crashing if someone does it your way - so make the handler check if a response was sent before sending message. Something like this: if (!response.headersSent) {
res.send(message);
} ( Finally, we should probably set the status code before calling res.status(options.status);
const message: any = (typeof options.message === 'function') ? await options.message(request, response) : message;
if (!response.headersSent) {
res.send(message);
} I haven't tested any of this, and I'm pretty sleep-deprived right now, so I may be missing something dumb. But that should at least give you the gist of what I'm thinking. This adds a bit of internal complexity over your solution, and might merit a couple more tests, but I think it will make for a better and more consistent API for our users. |
I think |
Hey @nfriedly, first of all, thank you for your time. I have made the changes according to your message. Please check it. |
For preventing someone from making requests at all, you could set const bannedIps = ['192.168.0.10', '192.168.0.23']
const limiter = rateLimit({
// ...
max: async (request, response) => {
if (bannedIps.includes(request.ip)) return 1
else return 5
},
}) To log requests that are rate-limited, you could use the handler function instead: const limiter = rateLimit({
// ...
handler: async (request, response, next, config) => {
console.log(`http: ${request.ip} was rate limited`)
response.status(config.statusCode).send(config.message)
},
}) |
However I do agree with @nfriedly, |
Hey @gamemaker1, I have made the changes according to your requested changes. |
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.
Thank you so much @colderry!
I have a couple of nitpicks below, but I think this looks good overall, so I'm going to go ahead and approve it now.
- In the readme, the description for
handler
has some example code that no longer matches what the defaulthandler
actually does. I think that's fine, but perhaps we should change the text to make it more clear that the example isn't actually the default.
- By default, sends back the `statusCode` and `message` set via the `options`:
+ By default, sends back the `statusCode` and `message` set via the `options`, similar to this:
- The suggestion for
_message
in the test below
@gamemaker1 if it looks good to you, feel free to merge it in and do the npm version minor
to trigger a release.
Co-authored-by: Vedant K <gamemaker0042@gmail.com>
You're welcome, @nfriedly. Also, I have just now made the changes to get rid of the nitpicks you mentioned above. |
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.
This looks good to go! Thank you so much @colderry for all the great work :]
message
option to be a function
These external tests take sooooo long to complete :sad: I'll try playing around with cache settings for npm so it covers the node modules in the |
I tried creating a release, but npm errored out saying it needed an OTP. @nfriedly could you please update the npm token in GitHub secrets with an automation token, and then push a release? Thanks! |
Op, sorry. Will do. |
Allow for manual response to ratelimit by function message.