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

Support for ioredis #137

Closed
wants to merge 5 commits into from
Closed

Conversation

thepipster
Copy link

Added support for ioredis, issue #120, by converting uppercase function names to lowercase and reducing connection detection strictness

@maritz
Copy link
Owner

maritz commented Aug 21, 2018

Thank you very much for this PR!

A couple of questions/requests, since I never really looked into ioredis in depth:

  • does it not offer a .connected equivalent?
  • could you do a check for whether connected is undefined or boolean, if it's boolean do the check (old way) if it's undefined just go ahead (current PR way)
  • could you add a check for an env var (e.g. NOHM_TEST_IOREDIS="true") to run all tests with ioredis instead of node-redis? This is a bit more work and I totally understand if you don't want to do it. In that case I will add it, but it'll take some time. I would then add it to travis.yml so that it runs all tests once with node-redis and once with ioredis.

@maritz
Copy link
Owner

maritz commented Aug 21, 2018

Also, .DS_Store is a macOS specific file, right? Should probably be added to .gitignore instead of adding it to the repo :-)

@thepipster
Copy link
Author

Thanks, sorry for delay - I'll take a look.

@thepipster
Copy link
Author

So I think ioredis has the equivalent;

(client.connected || client.status === 'connect')

@thepipster
Copy link
Author

I made those changes, but the tests are failing (I think) at deleteNonExistant - might need some help to debug...

@maritz
Copy link
Owner

maritz commented Aug 30, 2018

Wasn't quite as simple as I had hoped, but now all tests pass with node_redis and ioredis.

See #138 for the changes I added. I will probably close this one and then merge #138, unless you want to do some more work on any of this, then I'd recommend pulling in my 1 commit and then we can continue here.

@maritz
Copy link
Owner

maritz commented Aug 30, 2018

One thing that could still be added, but is entirely optional and could also go into its own PR is ioredis support in extra/stress.js.

Documentation is also missing for now.

@thepipster
Copy link
Author

Awesome, thanks!

@maritz maritz closed this Aug 31, 2018
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