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

chore: add ioredis dep, test #191

Open
wants to merge 2 commits into
base: release
Choose a base branch
from

Conversation

jamesholcomb
Copy link

...update [email protected]

Summary

(If you have not already please refer to the contributing guideline as described
here
)

If so, please mention them to keep the conversations linked together.

Other Information

If there's anything else that's important and relevant to your pull
request, mention that information here. This could include
benchmarks, or other information.

Your PR will be reviewed by a core team member and they will work with you to get your changes merged in a timely manner. If merged your PR will automatically be added to the changelog in the next release.

If your changes involve documentation updates please mention that and link the appropriate PR in feathers-docs.

Thanks for contributing to Feathers! ❤️

@daffl
Copy link
Member

daffl commented Apr 10, 2023

It looks like the tests are passing, so is #190 still an issue?

@jamesholcomb
Copy link
Author

So far the changes have fixed #190 for me (I have not released a production build internally however). As I don't utilize node-redis but need sentinel support, I removed node-redis in favor of ioredis for the adapter impl.

@daffl
Copy link
Member

daffl commented Apr 11, 2023

That's great. Though I don't think the original Redis driver would still work with this change. I'm thinking it'd either should be its own adapter or feature detect the driver. I'm still not sure what exactly it does differently 🤔

@jamesholcomb
Copy link
Author

The difference is that node-redis does not support Sentinels redis/node-redis#302 which is necessary for HA

feathers-sync worked implicitly with both through 2.4.0. The changes in 3.0.0 broke the redisClient compat with ioredis

I actually started the PR with the intent of supporting both, but got fed up working in js. The repo needs a TS conversion.

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.

2 participants