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

Shim dev tools #49349

Merged
merged 52 commits into from
Nov 14, 2019
Merged

Shim dev tools #49349

merged 52 commits into from
Nov 14, 2019

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Oct 25, 2019

This PR shims the dev tools app and removes the last bits of angular. By providing a registry similar to the application service dev tools can register themselves without using uiRoutes or directives.

Dev docs

The ui/registry/dev_tools is removed in favor of the DevTools plugin which exposes a register method in the setup contract. Registering app works mostly the same as registering apps in core.application.register. Routing will be handled by the id of the dev tool - your dev tool will be mounted when the URL matches /app/kibana#/dev_tools/<YOUR ID>. This API doesn't support angular, for registering angular dev tools, bootstrap a local module on mount into the given HTML element.

@flash1293 flash1293 added release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v8.0.0 Feature:NP Migration v7.6.0 labels Oct 25, 2019
@flash1293 flash1293 changed the title [skip ci ]Shim dev tools [skip ci]Shim dev tools Oct 25, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, there are some browser console errors that should be investigated
The Console settings seem to have some problems. no error in master
Bildschirmfoto 2019-11-11 um 09 51 55
The grok debugger also throws some errors, but these are unrelated to this PR, are also on master, seems the EuiCodeEditors are missing the theme setting.
Bildschirmfoto 2019-11-11 um 09 52 25

@streamich streamich mentioned this pull request Nov 11, 2019
@jloleysens
Copy link
Contributor

jloleysens commented Nov 11, 2019

@kertal @flash1293

Code LGTM, there are some browser console errors...

This one looks kinda weird, was Kibana server running?

The grok debugger also throws some errors, but...

Fwiw, it looks like may be an EUI problem; created this issue: elastic/eui#2517

Update:
Ah, ok so it requires a theme setting 😅 thought TS would pick that up?

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Platform glue code LGTM

});
}

start(core: CoreStart, { newPlatformDevTools }: DevToolsPluginStartDependencies) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
start(core: CoreStart, { newPlatformDevTools }: DevToolsPluginStartDependencies) {
public start(core: CoreStart, { newPlatformDevTools }: DevToolsPluginStartDependencies) {

start(core: CoreStart, { newPlatformDevTools }: DevToolsPluginStartDependencies) {
this.getSortedDevTools = newPlatformDevTools.getSortedDevTools;
if (this.getSortedDevTools().length === 0) {
core.chrome.navLinks.update('kibana:dev_tools', {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI this API will be replaced by the AppUpdater mechanism proposed in #45291

if (!this.getSortedDevTools) {
throw new Error('not started yet');
}
const { renderApp } = await import('./render_app');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By convention, this module should live in ./application

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maryia-lapata @kertal we probably have to fix that in a few places ;)

@kertal
Copy link
Member

kertal commented Nov 12, 2019

@kertal @flash1293

Code LGTM, there are some browser console errors...

This one looks kinda weird, was Kibana server running?

could be the case, I kbn bootstrapped today, no more weird errors 👍

@kertal
Copy link
Member

kertal commented Nov 12, 2019

@elasticmachine merge upstream

@kertal kertal self-requested a review November 12, 2019 09:03
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM 👍 , tested today, a kbn bootstrap in between, works

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@flash1293 flash1293 added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Nov 13, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

I just created a PR with a design suggestion.

@flash1293
Copy link
Contributor Author

Merging this, @miukimiu s suggestion can get in as a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:NP Migration release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants