-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add extension-api facades to cluster & workspace stores #1546
Add extension-api facades to cluster & workspace stores #1546
Conversation
Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>
I like the idea. With this we can refactor internal store without breaking the Extensions API and we don't need to expose all internal methods to extensions. |
/** | ||
* Active cluster id | ||
*/ | ||
activeClusterId = internalClusterStore.activeCluster; |
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.
have you tested that this works? When I was thinking about it yesterday I wasn't convinced that this would work and that we would have to do something like the following:
activeClusterId = internalClusterStore.activeCluster; | |
get activeClusterId() { | |
return internalClusterStore.activeCluster; | |
} | |
set activeClusterId(newId: string) { | |
internalClusterStore.activeCluster = newId; | |
} |
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.
get/set is better.
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, definitely a better way of making sure that we don't leak APIs to extensions.
Both Cluster and Workspace stores currently leak internals to extension-api. This PR adds a facade store for both of them and exposes only usable apis.
This also allows us to work more freely on store internals (we only expose apis explicitly). Good example of accidental leaks is #1453 .