-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Plugins should only be able to directly access UiSettings that they registered #101907
Comments
Pinging @elastic/kibana-core (Team:Core) |
Additional thing to consider: multiple plugins could register the same UiSetting, and we should allow it, as long as they match the same format and validation. |
Could you provide any real-world examples? It's not obvious why we should allow it. It creates ambiguity on ownership and creates implicit dependencies between plugins. |
As I was looking for more use cases, I found that the And it's imported and used all over many other plugins: Then, there's the special use case for the telemetry collector that reads all the non-default values. We might need to move that collector to core (or use a special API) to overcome the limitation.
I agree that the current scenario creates implicit dependencies. And that, currently, a plugin changing the definition of a UI Setting can potentially break other plugins. For that reason, I totally agree that a plugin should register the UI Settings it's going to use. However, having 2 settings for identical purposes, only because 2 different plugins require them is bad UX, and can lead to more inconsistencies between apps in Kibana. That's why I think that we should find a middle ground where plugins can share UI Settings, but there is a validation to ensure that the value is in the expected format. The first thing that comes to mind is having a Personally, I wouldn't simply move those shared settings to Core because that would mean having plugins' specific data in Core. That's why I see consumers declaring somehow the UI Settings they'll need in their plugins (probably when retrieving the client like in SOs hidden types?) 🤷♂️ What do you think? |
I don't say the cases don't exist, but it doesn't mean they are valid. All the cross-plugin interactions should be performed via an explicit contract. All other implicit dependencies (like listed in the list) are hard to follow, maintain and test. If plugins need
In most cases, there will be a single owner of a setting. From the list above only |
What problem are we specifically trying to solve by restricting a settings' access to only the plugin that registers it? I agree that write access should probably be limited but plugins should not be depending on another plugins' settings for any critical functionality. |
@TinaHeiligers I think the main problem this is aiming to solve is introduction of implicit dependencies between plugins, or leaking of APIs that aren't meant to be public. If Plugin A must depend on a uiSetting that's registered by Plugin B, it is better for Plugin A to consume it explicitly via an API exposed on Plugin B's contract, rather than implicitly using the global uiSettings exposed by core. That said, I think we do already have a type of workaround for this that has been used a few places in the codebase, and that's where plugins export a Overall I'm not sure I see this as a particularly high-priority enhancement, but certainly one worth considering if we end up doing a larger refactor (perhaps for user-personalized settings). |
Yea, that's exactly it. That's the same problem as having plugins directly consuming HTTP endpoints exposed from other plugins: It creates implicit (and untraceable) dependencies between plugins. All interaction between plugins should be done using explicit setup/start contracts. That being said, properly isolating uiSettings access to their respective owners, and solving all the current violations of this contract pattern, seems like a large effort, that is, realistically, probably not worth it until it blocks us for any reason. Or at least I agree that this feels like low priority. |
Pinging @elastic/appex-sharedux (Team:SharedUX) |
Summary
As a plugin developer, I want to be able to control who has access to any UiSettings that I register.
Acceptance Criteria
Resources:
Notes
Things to consider:
The text was updated successfully, but these errors were encountered: