Use modern mount context in Dev Tools and Console #70379
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cjcenizal
changed the title
cjcenizal
added
Feature:Console
cjcenizal
commented
Jun 30, 2020
src/plugins/console/public/plugin.ts
Outdated
| @@ -46,23 +43,32 @@ export class ConsoleUIPlugin implements Plugin<void, void, AppSetupUIPluginDepen | |||
| category: FeatureCatalogueCategory.ADMIN, | |||
| }); | |||
|
|
|||
| devTools.register({ | |||
| return devTools.register({ | |||
There was a problem hiding this comment.
The original use of async meant that the return value of calling devTools.register was published as this setup method's contract.
There was a problem hiding this comment.
Not 100% sure I follow 😅, how did async cause the call to devTools.register to be the return value of setup? AFAICT the return value of async setup was Promise<void>.
In fact, I don't think we should be returning the value of devTools.register as the public contract. Not sure it makes sense to expose our registration to dev tools to other plugins, thus giving them a way to enable and disable console?
There was a problem hiding this comment.
To clarify, I think the return value of setup should go from Promise<void> to just void which should be achieved by just removing async and not returning anything.
There was a problem hiding this comment.
You're right! I confused the implicit return of a promise with implicitly returning a value. Thanks for pointing this out!
cjcenizal
commented
Jun 30, 2020
| element, | ||
| appBasePath: '', | ||
| onAppLeave: () => undefined, | ||
| // TODO: adapt to use Core's ScopedHistory | ||
| history: {} as any, | ||
| }; | ||
| const unmountHandler = isAppMountDeprecated(activeDevTool.mount) |
There was a problem hiding this comment.
None of the dev tools have deprecated mount methods any more.
cjcenizal
commented
Jun 30, 2020
| @@ -66,7 +67,7 @@ export class DevToolApp { | |||
| constructor( | |||
| id: string, | |||
| title: string, | |||
| mount: App['mount'], | |||
| mount: AppMount, | |||
There was a problem hiding this comment.
I had to make this change in order for TS to pass. It kept thinking I was trying to use the deprecated interface.
cjcenizal
force-pushed
the
dev-tools/get-start-services
branch
from
10dac82 to
1c1ec2f
Compare
cjcenizal
commented
Jun 30, 2020
| @@ -59,15 +63,18 @@ export class DevToolsPlugin implements Plugin<DevToolsSetup, void> { | |||
| euiIconType: 'devToolsApp', | |||
| order: 9001, | |||
| category: DEFAULT_APP_CATEGORIES.management, | |||
| mount: async (appMountContext, params) => { | |||
| if (!this.getSortedDevTools) { | |||
There was a problem hiding this comment.
This looks like a typo. Instead of invoking this method we're just checking that it exists, so this condition will always evaluate to true. If this logic were to be changed to invoke the method and respect a false return value, we'd just throw an error and Kibana would crash. Which seems like worse behavior than rendering Dev Tools with an incomplete list of apps. So I think removing this condition makes the most sense.
There was a problem hiding this comment.
Yeah it looks like this check should have been:
this.getSortedDevTools().length === 0
But I think your proposal makes more sense 👍
|
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
cjcenizal
added
the
release_note:skip
cjcenizal
force-pushed
the
dev-tools/get-start-services
branch
from
1c1ec2f to
9118fe5
Compare
…n definitions of Grok Debugger, Search Profiler, and Painless Lab.
cjcenizal
force-pushed
the
dev-tools/get-start-services
branch
from
9118fe5 to
e05c909
Compare
jloleysens
requested changes
Jul 1, 2020
There was a problem hiding this comment.
These are some really welcome changes @cjcenizal !
I left one comment regarding console setup return value that I would like to block on, but for the most part this is looking great 👍
src/plugins/console/public/plugin.ts
Outdated
| @@ -46,23 +43,32 @@ export class ConsoleUIPlugin implements Plugin<void, void, AppSetupUIPluginDepen | |||
| category: FeatureCatalogueCategory.ADMIN, | |||
| }); | |||
|
|
|||
| devTools.register({ | |||
| return devTools.register({ | |||
There was a problem hiding this comment.
Not 100% sure I follow 😅, how did async cause the call to devTools.register to be the return value of setup? AFAICT the return value of async setup was Promise<void>.
In fact, I don't think we should be returning the value of devTools.register as the public contract. Not sure it makes sense to expose our registration to dev tools to other plugins, thus giving them a way to enable and disable console?
| @@ -59,15 +63,18 @@ export class DevToolsPlugin implements Plugin<DevToolsSetup, void> { | |||
| euiIconType: 'devToolsApp', | |||
| order: 9001, | |||
| category: DEFAULT_APP_CATEGORIES.management, | |||
| mount: async (appMountContext, params) => { | |||
| if (!this.getSortedDevTools) { | |||
There was a problem hiding this comment.
Yeah it looks like this check should have been:
this.getSortedDevTools().length === 0
But I think your proposal makes more sense 👍
|
@jloleysens Thanks for the review! I addressed your feedback. Could you take another look? |
jloleysens
approved these changes
Jul 1, 2020
|
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-oss-agent / Chrome UI Functional Tests.test/functional/apps/saved_objects_management/edit_saved_object·ts.saved objects management saved objects edition page allows to delete a saved objectStandard OutStack TraceBuild metrics
History
To update your PR or re-run it, just comment with: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes a warning from the browser console:
As part of this I also cleaned up the plugin definitions of Grok Debugger, Search Profiler, and Painless Lab.