-
Notifications
You must be signed in to change notification settings - Fork 59
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
Upgrade to use redis v4 #43
Conversation
* Update to redis v4 (and types) * Bump node to >=14, required for redis * Bump jest version to fix queueMicrotask missing jestjs/jest#9139
* Export url generation function that can take existing cache options to create a valid connection url for redis v4 * Wrap all cache functions in a connect promise since v4 does not auto connect https://github.com/redis/node-redis/blob/master/docs/v3-to-v4.md * Include 'legacyMode' option in redis client creation so most of existing code can be used in place * Fix isCacheableValue usage * Use v4 client for disconnect in tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I can find the time I'll review and test the PR.
For the second failing test -- defaults are set in |
+1 |
@MatthiasKunnen I'd prefer if you addressed the failing tests. The url is generated correctly regardless, at this point. |
First of all, thank you for your support and your pull request. It is much appreciated. I am happy to inform you that I have just released v3.0.0 of this package on npm which upgrades the redis dependency to |
Updates the package to work with redis v4 #40
While the package doesn't change functionally the dependencies have major changes (minimum node 14 for example) so would be better off bumping node-cache-manager-redis-store to v3.0.0
TODO
mset.should return an error if there is an error acquiring a connection
is failingmulti
object is a new instance and not yet connected?defaults are set by redis itself
is failing