Skip to content

Conversation

@michaelerobertsjr
Copy link

Nice package!!

However using this in production (even with a highly available Redis cluster), means that the developer needs to handle if Redis becomes unavailable.

This is highly unlikely, but we would not want our rate limiter to kill our route if Redis has any issue(s).

It's pretty hard to swap out middleware in a running express app when it is setup like this:

app.post('/api/endpoint', limiter(limitOptions),
  function (req, res) { ... }
);

I this was the simplest workaround I could think of. I will write some tests if you would like but, this patch is pretty easy to manually test...

@ded
Copy link
Owner

ded commented Dec 9, 2015

Hi @michaelerobertsjr thanks for your contribution!
This is a good and viable patch. Would you mind sticking to the existing style of the code. run make lint and be sure to remove the double quotes and add a whitespace after keywords like function and if. Also, please remove the version bump. I'll take care of that myself.
When finished push it back and I'll merge it.

@jpennal
Copy link

jpennal commented Mar 23, 2016

Any update on merging this change in? Its definitely a feature that I need. Thanks.

@michaelerobertsjr
Copy link
Author

@ded Can you have a look now? Removed the double quotes, added white space, and removed the version bump. Thx.

@aaron-straker
Copy link

Any word on this getting merged? This is something I also need. @ded

@vincentkwok
Copy link

vincentkwok commented Sep 27, 2017

any follow up ? @ded

@michaelerobertsjr
Copy link
Author

@ded bump

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants