-
Notifications
You must be signed in to change notification settings - Fork 52
Adds conditional to hanlde if redis becomes unavailable #22
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
Conversation
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
commented
Mar 23, 2016
Any update on merging this change in? Its definitely a feature that I need. Thanks.
68a3df5 to
7622797
Compare
michaelerobertsjr
commented
Mar 24, 2016
@ded Can you have a look now? Removed the double quotes, added white space, and removed the version bump. Thx.
aaron-straker
commented
May 9, 2016
Any word on this getting merged? This is something I also need. @ded
any follow up ? @ded
michaelerobertsjr
commented
Oct 30, 2017
@ded bump
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:
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...