feat: Added passOnStoreError to skip rate limitter if the store is not available

[Gp2mv3]

This PR adds the parameter passOnStoreError to allow requests if the store is not available.
For instance, if using Redis and the Redis server disconnects, the request will still pass.

This feature can be important for HA issues.

[nfriedly]

Good idea, thanks!

A few thoughts:

1. There's a lot of "stuff" inside the try/catch besides the store.increment() call, so I think it should either be renamed to passOnError or else the try/catch should be more narrowly scoped to only the increment call.
   • I'm leaning towards the rename option, but I could see a good argument for the other side.
2. If we go with the rename option, it should probably hook into the existing handleAsyncErrors:

   express-rate-limit/source/lib.ts

   Line 284 in e3e3ca9

   const handleAsyncErrors =

3. Regardless, I think it should log the error, not just silently allow the request.

What do you think @gamemaker1 ?

[nfriedly]

Oh, and this PR closes express-rate-limit/rate-limit-redis#193 and fixes express-rate-limit/rate-limit-redis#190

[gamemaker1]

Hi @Gp2mv3, thanks for making this PR!

1. There's a lot of "stuff" inside the try/catch besides the store.increment() call, so I think it should either be renamed to passOnError or else the try/catch should be more narrowly scoped to only the increment call.

I think we should keep it passOnStoreError, and scope it to the increment call as you said.

The passOnStoreError option has already been requested in rate-limit-redis, and I think it'd be better if we just provide it as part of express-rate-limit.

3. Regardless, I think it should log the error, not just silently allow the request.

I agree, we should make this addition to handleAsyncErrors regardless of the passOnStoreError option.

[Gp2mv3]

Hello,
Thanks for the feedback. I moved the try catch to only enclose increment.
However, I'm not sure of what you mean for the point 3. Can you elaborate ?

[gamemaker1]

By point 3, we mean logging the error before calling next().

Also logging the error in handleAsyncError can be done in a separate pr.

[nfriedly]

Also logging the error in handleAsyncError can be done in a separate pr.

handleAsyncError currently passes the error onto express.js to handle however it's configured to (show an error page by default), so no need to change it unless we add an option that ignores those errors.

express-rate-limit/source/lib.ts

Line 291 in e3e3ca9

next(error)

[nfriedly]

Suggested change

	next()
	console.error('express-rate-limit: error from store, allowing request without rate-limiting.', error)
	next()

This is what I meant by point 3.

[nfriedly]

I forgot to say it last time, but thank you for adding these tests!