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

Upgrade to use redis v4 #43

Closed
wants to merge 4 commits into from
Closed

Upgrade to use redis v4 #43

wants to merge 4 commits into from

Conversation

FoxxMD
Copy link

@FoxxMD FoxxMD commented Sep 7, 2022

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

  • The test mset.should return an error if there is an error acquiring a connection is failing
    • Due to a timeout with the callback. I think its because the multi object is a new instance and not yet connected?
  • The test defaults are set by redis itself is failing
    • Because AFAIK redis no longer makes defaults accessible -- I think they are determined by the system now

* 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
Copy link

@MatthiasKunnen MatthiasKunnen left a 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.

index.js Outdated Show resolved Hide resolved
@FoxxMD
Copy link
Author

FoxxMD commented Sep 26, 2022

For the second failing test -- defaults are set in RedisSocket.ts but I don't see a way to access this from the main client.

@mdwekat
Copy link

mdwekat commented Oct 6, 2022

+1

@FoxxMD
Copy link
Author

FoxxMD commented Oct 14, 2022

@MatthiasKunnen I'd prefer if you addressed the failing tests. The url is generated correctly regardless, at this point.

@dabroek
Copy link
Owner

dabroek commented Oct 15, 2022

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 redis@^4.3.1.

@dabroek dabroek closed this Oct 15, 2022
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.

4 participants