-
Notifications
You must be signed in to change notification settings - Fork 1
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
Cache colony clients #38
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Nice optimization 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. This is a very nice improvement.
If this was already in production, I suspect this change would have been felt in response times.
* Method attempting to return a cached colony client, | ||
* or get one from the networkClient if there's no cached entry | ||
*/ | ||
export const getCachedColonyClient = async ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very good start and I like your approach.
But for future improvements I would extend this to also be able to fetch/cache all types of contract clients: network, colony, tokens, but more important extensions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestions. I thought about doing it in this PR, but here's what I'm proposing we do instead:
- Network client - for now, it looks like
networkClient.ts
instantiates the client once and exports the same instance everywhere; This will only need to be addressed when we support multi-chain - Token client - opened Add token clients caching #43, I know Will is refactoring how we get different types of client and I don't want to make his life harder by introducing more changes
- Extensions client - opened Add extensions client caching #44, this is "blocked" by Will's motion work (nothing else uses extension clients)
Track past actions
This PR adds caching of colony clients. It was originally suggested by Raul in one of my PRs and it made sense to do it now as I found myself calling
getColonyClient
in different methods for the same colonyResolves #25