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

Enable RedisCache to have ioredis client injected #4870

Closed
RichardWright opened this issue Jan 26, 2021 · 3 comments
Closed

Enable RedisCache to have ioredis client injected #4870

RichardWright opened this issue Jan 26, 2021 · 3 comments

Comments

@RichardWright
Copy link

Current code from RedisCache

`export class RedisCache implements TestableKeyValueCache {
readonly client: any;
readonly defaultSetOptions: KeyValueCacheSetOptions = {
ttl: 300,
};

private loader: DataLoader<string, string | null>;

constructor(options?: RedisOptions) {
const client = new Redis(options);
this.client = client;

this.loader = new DataLoader(keys => client.mget(...keys), {
  cache: false,
});

}
`

Could we inject the ioredis and new it up if it's not there?

@RichardWright
Copy link
Author

I opened up a pr to fix this.

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 20, 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