search

feathersjs-ecosystem / feathers-sync

Synchronize service events between Feathers application instances
MIT License
221 stars 41 forks source link

chore: add ioredis dep, test #191

Open jamesholcomb opened 1 year ago

jamesholcomb commented 1 year ago

...update feathers@5.0.3

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! :heart:

daffl commented 1 year ago

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

jamesholcomb commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

The difference is that node-redis does not support Sentinels https://github.com/redis/node-redis/issues/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.