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

apollo-server-cache-redis: Add redis connection uri interface to RedisCache #5006

Closed
airtoxin opened this issue Mar 8, 2021 · 3 comments
Closed

Comments

@airtoxin
Copy link

airtoxin commented Mar 8, 2021

RedisCache currently only accept RedisOptions interface of ioredis, but Redis constructor of ioredis can accept more variadic arguments.
https://github.com/luin/ioredis#connect-to-redis

new (port?: number, host?: string, options?: IORedis.RedisOptions): IORedis.Redis;
new (host?: string, options?: IORedis.RedisOptions): IORedis.Redis;
new (options?: IORedis.RedisOptions): IORedis.Redis;

In my use case, my application already connect to redis via connection uri redis://..., and RedisCache also want to connect its redis, but RedisCache can't accept connection uri.

@glasser
Copy link
Member

glasser commented Mar 8, 2021

Hi! Rather than continuing down the road of making our package's API even more tightly coupled to the constructor API of ioredis, I'd rather see us expect the user to construct their own ioredis object and pass it to us. #4871 is a start at implementing this but as mentioned in my comment there the API is a bit awkward. I'm not sure that the original author on that PR is interested in continuing on it so somebody else might want to take a shot at it.

@glasser glasser closed this as completed Mar 8, 2021
glasser pushed a commit that referenced this issue Apr 6, 2021
- Allow Redis client to be injected by using the `BaseRedisCache` class which contains the existing implementation logic without the client initialisation.
- Removes duplication between `RedisCache` and `RedisClusterCache`
- Fixes a bug where the result of delete is returned as a number, rather than a boolean. (Our TypeScript types have always declared that this function returns a boolean as `KeyValueCache.delete` should, but the actual implementation returned a number. This was not noticed because of the use of an `any` type.)

In my opinion, `BaseRedisCache` should really be `RedisCache` and the existing `RedisCache` + `RedisCacheCluster` should either be renamed to identify that they are using the ioredis client, or removed and left for the consumer of the library to implement/ inject the client using this method. But this would obviously be a breaking change so maybe one to think about for the future?

I've created an interface for the `RedisClient` rather than rely on the ioredis type as i do not believe apollo should be tied to the ioredis interface.

Fixes #4871. Fixes #4870. Fixes #5006.
@glasser
Copy link
Member

glasser commented Apr 9, 2021

I've released a prerelease with this fix (apollo-server-cache-redis@1.4.0-alpha.0, part of the Apollo Server 2.23.0 release). Try out the alpha and see if it works for you! Please provide any feedback in #5094.

@glasser
Copy link
Member

glasser commented Apr 14, 2021

This has been released in apollo-server-cache-redis@1.4.0.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants