feat: enable injection of redis client into redis cache

[RichardWright]

Feat: enable injection of redis client. Stops multiple clients being invocated.

[Nedomas]

Also need this. Anything we could do to move this forward?

[RichardWright]

I've done everything I think I can, just need some attention from the maintenance people

[glasser]

This seems like a reasonable thing to want to do — in fact, it would be better to just have designed this as taking a client in the first place rather than being responsible for initializing a third-party library.

I'm not a big fan of the API in this change though... two arguments and you have to pass exactly one non-null. It would be good to just have two entry points, one for "here's the client" and one for "please make a client for me".

Ideally, "here's the client" would be the constructor and "please make a client for me" would be a (not particularly useful) factory function. But if we want to be backwards-compatible with the existing API, that's tough (since factory functions to have to go through the single constructor).

What if we just create a new class, RedisClientCache (or maybe a better name exists), which takes a client as its argument? Then the old RedisCache can just be a subclass with a different constructor.

While you're at it, it looks like RedisClusterCache is basically a copy-and-paste of RedisCache that mostly just differs in the constructor, so I bet you could reduce the duplication here by making the two existing classes into small wrappers around the new class.

Please also make sure there is test coverage of the new API.

[RichardWright]

I'm really not into refactoring other people's code bases. Ideally I would make a change and people with better knowledge and ideas about the design would refactor how they envisage it. Otherwise you'll never accept my pr

[glasser]

Isn't that the process that's happening? You made a change and my suggestion is that instead of complicating the constructor of the existing classes, you make a new class with a simple constructor.

You're welcome to take that suggestion or wait for somebody else to do so. As a maintainer, my approach to packages like this one that exist to interface with external projects like Redis is that improvements should be driven and QA'd by folks who actually run Redis themselves. We don't use this package ourselves at Apollo so I don't feel like we are able to complete this PR ourselves.