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

feat: added support for IORedis in TypeORM caching #3289

Conversation

HussainAliAkbar
Copy link
Contributor

No description provided.

@HussainAliAkbar
Copy link
Contributor Author

@pleerock @Kononnable , this PR is against the master branch. I have also refactored the code a bit so that there is no duplication. almost all the methods were reused except for the connect method which i have handled.

Do let me know if this works for you guys or not.

this.client = this.redis.createClient();
}
} else if (this.clientType === "ioredis") {
if (cacheOptions && cacheOptions.options) {this.client = this.redis.createClient();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it typo? this.client is being assigned two times?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed it was. i have fixed it.

@HussainAliAkbar
Copy link
Contributor Author

@pleerock , if you are going ahead with the merge, should i update the caching documentation as well?

@pleerock
Copy link
Member

yes, please :)

/**
* Type of the Redis Client (redis or ioredis).
*/
protected clientType: string;
Copy link
Contributor

@Kononnable Kononnable Dec 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type can be set to "redis" | "ioredis" - that way we can omit a comment which can become outdated in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the type to protected clientType: "redis" | "ioredis";. However, i have still kept the comment to make it consistent with other protected properties. I thought that if in the future, the type was changed, the comment could be changed with it as well.

Let me know if the comment still needs to be removed.

@HussainAliAkbar
Copy link
Contributor Author

@pleerock documentation updated.

@ignacioblit
Copy link
Contributor

Hey guys, how are you?

Now that i've read the documentation update, and the options. I'm thinking that this new implementation does not support the Cluster options for ioredis is that correct?

https://github.com/luin/ioredis/blob/master/API.md#Cluster

if thats the case, do you have any plans on implementing this? If not, may I take a look into supporting such feature?

Thanks!

@ignacioblit
Copy link
Contributor

Hey guys, any news on this?

@pleerock @HussainAliAkbar

@pleerock
Copy link
Member

pleerock commented Jan 3, 2019

Think we can merge it, thanks for the contribution!

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.

None yet

4 participants