Reverse dependency of home plugin and apm/ml/cloud #52883
Conversation
|
Jenkins, test this. |
💔 Build FailedHistory
To update your PR or re-run it, just comment with: |
flash1293
added
Feature:Home
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
flash1293
added
the
release_note:skip
|
@elasticmachine merge upstream |
| test('allows multiple update calls', () => { | ||
| const setup = new EnvironmentService().setup(); | ||
| setup.update({ ml: true }); | ||
| setup.update({ apmUi: true }); |
There was a problem hiding this comment.
There's an expect missing in this test, isn't it?
There was a problem hiding this comment.
Sort of, the test is that this doesn't throw. I added an explicit expect(...).not.toThrow()
| @@ -93,7 +92,6 @@ export class ApmPlugin | |||
|
|
|||
| // Take the DOM element as the constructor, so we can mount the app. | |||
| public setup(_core: CoreSetup, plugins: ApmPluginSetupDeps) { | |||
| plugins.home.featureCatalogue.register(featureCatalogueEntry); | |||
There was a problem hiding this comment.
This is in here for the NP plugin to register itself, and will run both as a shim and when we move it to a pure NP plugin. The legacy_register_feature.ts above is only run in the legacy plugin.
We would only be removing this here if the featureCatalog.register is not wanted in the NP plugin. Is that the case?
There was a problem hiding this comment.
It is wanted, but currently it's registered twice because the legacy_register_feature file is pulled into the apm bundle as well. I traced back the change and this happens because the home ui export is a dependency of visualize which is loaded everywhere. It seems like this is not necessary and removing the import there also fixes this problem.
| @@ -23,7 +23,6 @@ import 'uiExports/docViews'; | |||
| import 'uiExports/embeddableActions'; | |||
| import 'uiExports/fieldFormatEditors'; | |||
| import 'uiExports/fieldFormats'; | |||
| import 'uiExports/home'; | |||
There was a problem hiding this comment.
@maryia-lapata this caused all home ui export entrypoints to be included in all bundles (because the visualize embeddable is loaded as a "hack" entrypoint). Could you confirm that it's not actually needed to render visualizations?
There was a problem hiding this comment.
I don't think that I'm a right person to confirm that. I just removed these exports during centralizing dependancies and then returned it back during Visualize shimming.
I also found that these exports were introduced in https://github.com/elastic/kibana/pull/39126/files#diff-53ca92fe2de1ead9193e8c05dc91d415R29. Maybe it helps.
There was a problem hiding this comment.
Sorry, just looked one level deep in the git blame :) @stacey-gammon is there a dependency of the visualize embeddable code to the home exports you know of? I'm only seeing feature registrations but maybe I'm missing something.
There was a problem hiding this comment.
I don't think it's needed, I was probably over-zealous in my copy/pasting that ended that line up here.
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* upstream/master: (72 commits) [ML] Calculate model memory limit API integration tests (elastic#54557) Skip flakey index template component integration tests. (elastic#54878) Add label and icon to nested fields in the doc table (elastic#54199) Reverse dependency of home plugin and apm/ml/cloud (elastic#52883) [SIEM][Detection Engine] Order JSON keys, fix scripts, update pre-packaged rules update invalid snapshot add readme note about alerting / manage_api_key cluster privilege (elastic#54639) [SIEM] New Overview Page (elastic#54783) [Uptime] Feature/refactor context initialization (elastic#54494) Upgrade EUI to v18.2.0 (elastic#54786) [SIEM] [Detection engine] from signals to timeline (elastic#54769) [Index Management] Add Mappings Editor to Index Template Wizard (elastic#47562) [SIEM][Detection Engine] Removes deprecated filter from mapping [Maps] Add categorical styling (elastic#54408) Add mapbox-gl-rtl-text library (elastic#54842) [SIEM][Detection Engine] Adds actions to Rule Details (elastic#54828) Lexicographically sort location tags (elastic#54832) [Maps] expand extent filter to tile boundaries (elastic#54276) [Maps] Use v7.6 Elastic Maps Service API (elastic#54399) [DOCS] Adds monitoring setting (elastic#54819) ...
Currently the home plugin depends on several x-pack plugins to be available. This PR reverses the dependency by providing a function to set the environment the home app is rendering via the setup contract of the home app. Now the x-pack plugins have an optional dependency of the home app instead of the other way around.
This approach is still not 100% clean because the home app implicitly knows about these x-pack plugins, but it solves the problem of explicit dependency. If this portion of the home app is re-done in the future, the API should be replaced by display-specific extension points (e.g.
registerPanel) instead of explicit knowledge of the x-pack functionality. Till then I marked these APIs as deprecated to discourage extension.