Skip to content
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

Add note about using multiple Redis versions #342

Merged
merged 7 commits into from
Dec 22, 2021
Merged

Conversation

abhijoshi2k
Copy link
Contributor

Added new option to connect using connection string.

redisUrl - A Redis connection string to be used for the default client connection. Ignored when the client option is provided. Refer Redis Client connection string format.

Closes #341

@wavded
Copy link
Collaborator

wavded commented Dec 16, 2021

Thanks for contributing! Unfortunately, this approach causes an issue with users who don't use redis now being required to have that installed. This used to be the approach when that was the only clients but now you bring your own client.

The other issue, is this doesn't really solve all the compatibility issues with how Redis is actually used in this project. See #337 (comment) for more details.

@abhijoshi2k
Copy link
Contributor Author

Thanks for response. I'm not suggesting this as permanent option. However, it can be a temporary fix so that users would be able to use the package even with node-redis v4. We can specify in documentation that the client option is only supported for v3 and redisUrl option will use node-redis v3 by default.
I think, that is how rate-limit-redis package can still be used with redisURL option even though the client option is affected.
After things get sorted with breaking changes of node-redis v4, this option can be removed.

@abhijoshi2k
Copy link
Contributor Author

As per recent comment on related issue, I think the previous changes proposed in this PR will not be necessary. However, I have updated the documentation showing a note about usage with redis v4. If these changes are okay, feel free to merge this PR or suggest edits. It is also fine if these updates to documentation are not acceptable. In that case, close this PR.

@wavded wavded changed the title Added redisUrl option to provide connection string Add note about using multiple Redis versions Dec 22, 2021
@wavded wavded added the chore label Dec 22, 2021
@wavded wavded merged commit 31248e2 into tj:master Dec 22, 2021
@wavded
Copy link
Collaborator

wavded commented Dec 22, 2021

Thx @abhijoshi2k !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using redis connection string to cope with node-redis v4 breaking changes
2 participants