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

feat: ioredis cache handler #810

Open
wants to merge 4 commits into
base: canary
Choose a base branch
from

Conversation

jsmylnycky
Copy link

The primary projects I work on leverage ioredis as the client of choice, primarily due to a history of having better results connecting to AWS ElastiCache.

The goal here was to copy the redis-strings cache handler, and update the calls to the ioredis variation.

I'm sharing this back in hopes that others might find this particular client useful.

I confirmed all e2e tests pass.

I considered explicitly calling this ioredis-strings but opted for shorthand ioredis. I'm happy to change it if anyone feels strongly, just let me know!

Copy link

changeset-bot bot commented Oct 15, 2024

⚠️ No Changeset found

Latest commit: aaf134a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@better-salmon
Copy link
Contributor

Hi @jsmylnycky!

Thank you for your contribution to the @neshca/cache-handler!

It seems like the Redis team has dropped the support of the ioredis package half a year ago. They claim that node-redis V5 is their main focus. Maybe instead of adding this Handler we'll create a place for community Handlers. It can be a new package in a monorepo, wiki or docs page. What do you think about it?

@jsmylnycky
Copy link
Author

Hi @better-salmon ! Thanks for sharing that info about ioredis. Admittedly, that revelation will push me to look deeper into using node-redis with Elasticache and just work through the hurdles I ran into last time I tried (been a couple years).

If you still want to proceed with a community handlers package, I'm happy to push this into that instead, in case it may help someone who can't quite as easily move away from ioredis.

Just let me know how you'd look to proceed with this!

@better-salmon
Copy link
Contributor

For now supporting Next.js 15 is top priority. I'll return to you later 🙂

@jsmylnycky
Copy link
Author

@better-salmon 100% understood and agree on the Next15 priority!

Just wanted to let you know that I would like to go ahead with publishing this as a community handlers package, so I'll work with you when you're ready to put some time into that.

I spent a good amount of hours last week trying to get node-redis to work, but unfortunately it still doesn't play very nice with ElastiCache. Their Cluster support is pretty lacking of certain features (isReady doesn't exist, and isOpen is too broad), and their published events only seem to work for error and nothing else, so it's extremely clunky trying to determine if the client is actually active and connected or not. I'll give it another look with v5, but I haven't seen a published timeline to know when that will be released.

@gergokee
Copy link

gergokee commented Nov 27, 2024

This is awesome @jsmylnycky , we indeed need this cause unfortunately node-redis lacks a lot of things. For example sentinel support (i know they are working on it but its still not added) which we for example rely on. I started to write my own solution but thankfully i stumbled upon this code which is a great help. A big 👍 from me to support the PR merge !

@jsmylnycky
Copy link
Author

I'll get the conflicts resolved once I get the green light on how to proceed :) Just giving this project some breathing room while they focus on Next15 support.

@gergokee
Copy link

gergokee commented Nov 27, 2024

Yeah i see suport for nextjs15 is still in progress so we are not upgrading to 14 until we see it's safe. thank you anyways ! :)

ps.: hope node-redis will also add/fix their cluster and sentinel support by then

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.

3 participants