-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Upgrade to ioredis@5 #260
Upgrade to ioredis@5 #260
Conversation
|
||
export interface RedisModuleOptions { | ||
config: Redis.RedisOptions & { url?: string }; |
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.
There's a chance they had a specific reason to do it this way. * as Redis
can fix compatibility issues with common module default exports.
Hopefully fixed with the ioredis upgrade.
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.
I found a note about this in the v4 -> v5 upgrade guide for ioredis: https://github.com/luin/ioredis/wiki/Upgrading-from-v4-to-v5
That being said, if there's a good way to test / confirm any concerns, I can perform them / add them. I did get the few tests in the project running locally successfully, as well as minimally installed @nestjs/microservices and @nestjs/bull next to this on a test project without issue.
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.
Nice !
I guess the only other thing to test would be use with a plain js nest project?
I think that's a possibility.
I would love to make use of this. Anything I can do to help get this change in? |
Hello All, |
Bump - also interested in this |
Gave the upgrade a shot, happy to make adjustments and help get this ready for merge / release.