dial-once/node-cache-manager-redis

Expose the connection pool

moxious opened this issue · 3 comments

Right now this module keeps track of the sol-redis-pool instance under _pool. If you have an application that needs cache manager for simple get/put, but also wants to use redis for things like pub/sub, it would be nice to make this item not private, or provide an "approved" method for accessing the pool to draw other connections from it for other purposes. It seems unnecessary to create one connection pool for the cache-manager, and a second pool for other purposes.

Would you be open to accepting a pull request to define say a function getPool() or to rename pool so that it doesn't start with an underscore and isn't considered private to permit this?

Hi @moxious
Thanks for your feedback, your use case is interesting.

I prefer to expose a getPool() method, I think it's more cleaner and testable. I'm of course open to accept pull request, just make sure it's well tested to keep the coverage to 100% :-)

So...this would be absolutely no problem to write up and cover with a test, but after I forked/cloned this repo to do it, for me the test suite doesn't pass as-is.

[mda@zeke:~/sw/node-cache-manager-redis]1 npm test

> cache-manager-redis@0.2.2 test /Users/mda/sw/node-cache-manager-redis
> jasmine

Started
....node_redis: Deprecated: The SETEX command contains a "undefined" argument.
This is converted to a "undefined" string now and will return an error from v.3.0 on.
Please handle this in your code to make sure everything works as you intended it to.
undefined:1
undefined
^

SyntaxError: Unexpected token u
    at Object.parse (native)
    at Command.callback (/Users/mda/sw/node-cache-manager-redis/index.js:74:23)
    at normal_reply (/Users/mda/sw/node-cache-manager-redis/node_modules/redis/index.js:662:21)
    at RedisClient.return_reply (/Users/mda/sw/node-cache-manager-redis/node_modules/redis/index.js:722:9)
    at JavascriptReplyParser.RedisClient.reply_parser.Parser.returnReply (/Users/mda/sw/node-cache-manager-redis/node_modules/redis/index.js:146:18)
    at JavascriptReplyParser.run (/Users/mda/sw/node-cache-manager-redis/node_modules/redis-parser/lib/javascript.js:137:18)
    at JavascriptReplyParser.execute (/Users/mda/sw/node-cache-manager-redis/node_modules/redis-parser/lib/javascript.js:112:10)
    at Socket.<anonymous> (/Users/mda/sw/node-cache-manager-redis/node_modules/redis/index.js:223:27)
    at emitOne (events.js:90:13)
    at Socket.emit (events.js:182:7)
npm ERR! Test failed.  See above for more details.

I'm using a docker image of redis 2.8.23 -- and yes I did configure config.json in spec to make sure my host/port settings were right.

@moxious Tests should no longer be failing on the latest master.