-
-
Notifications
You must be signed in to change notification settings - Fork 34
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
Include a default redis client #203
Comments
I'm on the fence for this one; don't really see a need to add this option. We can fix #202 by explaining the However, if we do implement this, I think we should go with |
Apologies for the delayed response (university problems), but after spending some time troubleshooting... The developers can't simply replicate the solution from rate-limit-mongo (#202) because in this solution the team tailored a MongoDB client specifically for this purpose. Therefore, if we aim to achieve similar functionality, we need to rely on a single Redis client, this might deviate from the original approach. So, I believe that have two sides: 1st side:
2nd side (and I prefer it):
I believe that using the default Redis client can be more transparent for users, avoiding adding a lot of dependencies to their projects. |
No worries. The trouble is that I'd like to avoid a breaking change unless it's really necessary. I also think there are some more advanced use-cases where being able to provide a custom I'm ok with either Improving the documentation is always good :) |
Description
I think we should include a default redis client, and accept a new option for the connection details that is passed directly to the redis client.
I think node-redis and ioredis are the two most popular ones. node-redis has more github stars while ioredis has more npm downloads. I'm leaning a bit towards ioredis since it has a smaller file size, although node-redis has the advantage of only accepting 0 or 1 arguments to it's constructor, whereas ioredis accepts 0-3. (We could accept an array for the 2+ arg versions.)
We should still keep the
sendCommand
option around for backwards compatibility and advanced usage. It should probably throw an error if bothsendCommand
and our new option are set at the same time.Why
Alternatives
Not changing anything
The text was updated successfully, but these errors were encountered: