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

[data] Expose uiSettings in plugin contract #67060

Closed
Tracked by #55919
lukeelmers opened this issue May 19, 2020 · 6 comments
Closed
Tracked by #55919

[data] Expose uiSettings in plugin contract #67060

lukeelmers opened this issue May 19, 2020 · 6 comments
Labels
chore impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:large Large Level of Effort technical debt Improvement of the software architecture and operational architecture

Comments

@lukeelmers
Copy link
Member

In #66040 we migrated some uiSettings out of the legacy platform. At the time, setting names were converted to statically-exported consts which other plugins imported when using core's uiSettings service.

Ideally we would follow a pattern closer to what was suggested here, where the settings are exposed via a getter on the service's runtime contract, e.g.

// this...
data.settings.getSearchQueryLanguage();

// ...instead of this
core.uiSettings.get('search:queryLanguage');

Then applications relying on this setting would create an explicit dependency on the plugin which registered the setting, and retrieve the value via that plugin's contract.

This would involve a few things:

  1. Create getters on the data plugin's runtime contract which call uiSettings.get under the hood
  2. Update downstream usages of each of the settings to retrieve them from the data plugin instead
  3. Remove constants for each of the setting names from the public contract of the data plugin, as they now become an internal implementation detail.
@lukeelmers lukeelmers added chore loe:large Large Level of Effort technical debt Improvement of the software architecture and operational architecture Team:AppArch impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. labels May 19, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@streamich
Copy link
Contributor

Do we still need this? As now even static imports create an explicit dependency in plugin tree.

@lukeelmers
Copy link
Member Author

As now even static imports create an explicit dependency in plugin tree.

Yeah I think the original thread that started this discussion is less relevant now that you can no longer do static imports without explicitly declaring a dependency on that plugin. It would just be a matter of whether we'd want to recommend folks expose settings from plugin contracts in the future as a general pattern.

IMHO we can probably close this as the enforcement on static imports solves the biggest issue from my perspective, and we are thinking about how uiSettings will evolve longer term anyway.

cc @pgayvallet @TinaHeiligers @mshustov for any other thoughts on that

@mshustov
Copy link
Contributor

mshustov commented May 4, 2021

IMHO we can probably close this as the enforcement on static imports solves the biggest issue from my perspective, and we are thinking about how uiSettings will evolve longer term anyway.

Can we verify that all the plugins use imported keys? I didn't check all the data plugin keys, but for timepicker:quickRanges I the found the next cases

commonlyUsedRanges: core.uiSettings.get(DEFAULT_TIMEPICKER_QUICK_RANGES),

const [quickRanges] = useUiSetting$<Range[]>(DEFAULT_TIMEPICKER_QUICK_RANGES);

@lukeelmers
Copy link
Member Author

Can we verify that all the plugins use imported keys?

They definitely don't all use imported keys yet, so perhaps this issue could be rescoped to that instead (ensure imported keys are used anywhere in Kibana that relies on uiSettings registered by the data plugin)

@ppisljar
Copy link
Member

ppisljar commented Aug 8, 2022

Thank you for contributing to this issue, however, we are closing this issue due to inactivity as part of a backlog grooming effort. If you believe this feature/bug should still be considered, please reopen with a comment.

@ppisljar ppisljar closed this as completed Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:large Large Level of Effort technical debt Improvement of the software architecture and operational architecture
Projects
None yet
Development

No branches or pull requests

5 participants