feat: allow the message option to be a function

[colderry]

Allow for manual response to ratelimit by function message.

[gamemaker1]

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 statusCode and message options.

[gamemaker1]

Also, I love how you planned and split the changes into three commits :]

[colderry]

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 statusCode and message options.

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.

[nfriedly]

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 handler option, so I don't think it would make sense to merge in it's current form.

That said, I like the idea allowing message to be a function - I can see value there, and it fits with the way most other options that are allowed to be functions or values.

However, I think that it would make more sense and be more consistent with the rest of the API to allow message to return an arbitrary value, and then use that value as the response rather than expect message to sent a response itself.

Also, await instead of passing in next because we do want to allow for async message functions, but not in a way that bypasses the rest of express-rate-limit. (In the rare case a user actually needs to call next, they can supply a custom handler option.)

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);
}

(headersSent isn't technically a guarantee that a response was sent, but it is a good proxy. There's probably some other property that would be better to check, but I don't remember what off the top of my head.)

Finally, we should probably set the status code before calling message, in case message() sends a response without setting the status. So the final handler might look something like this:

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.

[nfriedly]

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.

I think handler could manage those cases as easily as message, but an EventEmitter might be an even better fit. I remember looking at that once before, but perhaps it's time to look at it again.

[colderry]

Hey @nfriedly, first of all, thank you for your time.

I have made the changes according to your message. Please check it.

[gamemaker1]

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.

For preventing someone from making requests at all, you could set max to 1 based on the request:

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)
	},
})

[gamemaker1]

However I do agree with @nfriedly, message being a function could be beneficial to our users.

[colderry]

Hey @gamemaker1, I have made the changes according to your requested changes.

[nfriedly]

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.

1. In the readme, the description for handler has some example code that no longer matches what the default handler 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:

2. 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.

[colderry]

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.

1. In the readme, the description for handler has some example code that no longer matches what the default handler 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:

2. 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.

You're welcome, @nfriedly.

Also, I have just now made the changes to get rid of the nitpicks you mentioned above.

[gamemaker1]

This looks good to go! Thank you so much @colderry for all the great work :]

[gamemaker1]

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 tests/external folder. That should hopefully make it faster.

[gamemaker1]

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!

[nfriedly]

Op, sorry. Will do.