Add plugin start contract to getStartServices return value #61216

pgayvallet · 2020-03-25T08:08:10Z

Summary

Fix #61106

Add the plugin's own start contract as a third element in the getStartServices return type
Adapt existing calls and cleanup a few plugin declarations

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2020-03-25T08:08:30Z

Pinging @elastic/kibana-platform (Team:Platform)

pgayvallet

While doing this PR, I saw that our Plugin class contract types (TSetup and TStart) defaults to void

export interface Plugin<
  TSetup = void,
  TStart = void,
  TPluginsSetup extends object = object,
  TPluginsStart extends object = object
>

I was wondering if that should be changed to TSetup extends objects = {}

Upsides I see:

more consistent, every plugin has to return a contract, even if an empty one
some plugins are using their optional dependencies contract to check if the dependency is enabled or not. Allowing plugins to return void breaks this, which is atm the only way to perform such check.

Second point, should TPluginsSetup and TPluginsStart also defaults to {} instead of object?

WDYT, should I create an issue for this?

pgayvallet · 2020-03-25T08:56:46Z

examples/alerting_example/public/plugin.tsx

 export class AlertingExamplePlugin implements Plugin<Setup, Start, AlertingExamplePublicSetupDeps> {
  public setup(
-    core: CoreSetup<AlertingExamplePublicStartDeps>,
+    core: CoreSetup<AlertingExamplePublicStartDeps, Start>,
    { alerting, triggers_actions_ui }: AlertingExamplePublicSetupDeps
  ) {


Would be nice if somedays TS understand that setup is an override/implementation of Plugin.setup to avoid having to manually redefine setup's parameters...

It was attempted once, but it was decided that it broke too many things: microsoft/TypeScript#6118 (comment)

They probably know better than me, however their ugly section seems like invalid argumentation, and I don't see anything in the bad section that wouldn't have been fixed in their example with a generic type Base<T extends Item>. That's how that would have been achieved/fixed in any generic-using language... But anyway, they probably know better 😄

pgayvallet · 2020-03-25T08:58:34Z

examples/embeddable_examples/public/index.ts

-export const plugin: PluginInitializer<void, void> = () => new EmbeddableExamplesPlugin();
+export const plugin: PluginInitializer<
+  void,
+  void,
+  EmbeddableExamplesSetupDependencies,
+  EmbeddableExamplesStartDependencies
+> = () => new EmbeddableExamplesPlugin();


I'm really not sure how/why, but adding the TStart type to CoreSetup forced to be more explicit with the setup and start dependencies when defining a PluginInitializer. That doesn't looked to me like a bad thing though.

pgayvallet · 2020-03-25T09:00:55Z

src/core/public/plugins/plugins_service.ts

-  private readonly plugins = new Map<PluginName, PluginWrapper<unknown, Record<string, unknown>>>();
+  private readonly plugins = new Map<PluginName, PluginWrapper<unknown, unknown>>();


Had to do that to match the createPluginSetupContext type in the call line 117

pgayvallet · 2020-03-25T09:02:29Z

src/legacy/core_plugins/vis_type_vislib/public/plugin.ts

-export class VisTypeVislibPlugin implements Plugin<Promise<void>, void> {
+export class VisTypeVislibPlugin implements Plugin<void, void> {


Unwrapped a few useless promise types as setup(core: CoreSetup, plugins: TPluginsSetup): TSetup | Promise<TSetup> already handles it.

azasypkin

Security/Spaces changes LGTM (with green CI).

flash1293

Kibana App changes LGTM

mikecote

Alerting changes LGTM

nchaulet

+1 for the ingestManager plugin

lukeelmers

Code LGTM - thanks @pgayvallet for adding this!

joshdover · 2020-03-25T21:16:23Z

src/core/public/index.ts

@@ -183,7 +183,7 @@ export { MountPoint, UnmountCallback, PublicUiSettingsParams } from './types';
 * navigation in the generated docs until there's a fix for
 * https://github.com/Microsoft/web-build-tools/issues/1237
 */
-export interface CoreSetup<TPluginsStart extends object = object> {
+export interface CoreSetup<TPluginsStart extends object = object, TStart = unknown> {


We may want to add some @typeParam docs for these now. It's not exactly obvious what should be passed in here.

Did not know about @typeParam, will add.

Added, however this annotation doesn't seem to be reflected in the generated doc

joshdover · 2020-03-25T21:33:51Z

src/core/public/mocks.ts

+}: {
+  basePath?: string;
+  pluginStartDeps?: object;
+  pluginStartContract?: object;


super nit: doesn't technically have to be an object

darnautov

ML changes LGTM

…ervice-plugin-contract

kibanamachine · 2020-03-26T09:41:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 88533f9

History

💚 Build #36131 succeeded f6988a7
💔 Build #36109 failed 22569fe

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@typeparam

…1216) * add plugin own contract as third element of getStartServices result * adapt plugins code * update tests * revert unknown to use void again * update generated doc * fix UT * update mock to allow non-object `pluginStartContract` * add @typeparam documentation

@typeparam

…61415) * add plugin own contract as third element of getStartServices result * adapt plugins code * update tests * revert unknown to use void again * update generated doc * fix UT * update mock to allow non-object `pluginStartContract` * add @typeparam documentation

pgayvallet added 2 commits March 25, 2020 07:54

add plugin own contract as third element of getStartServices result

3ac3e7a

adapt plugins code

22569fe

pgayvallet added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Mar 25, 2020

pgayvallet added Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes v7.8 v8.0.0 labels Mar 25, 2020

pgayvallet requested a review from lukeelmers March 25, 2020 08:09

pgayvallet added 3 commits March 25, 2020 09:34

update tests

edaa9f4

revert unknown to use void again

796e51d

update generated doc

064c595

pgayvallet commented Mar 25, 2020

View reviewed changes

pgayvallet marked this pull request as ready for review March 25, 2020 09:08

pgayvallet requested a review from a team March 25, 2020 09:08

pgayvallet requested a review from a team as a code owner March 25, 2020 09:08

pgayvallet requested a review from a team March 25, 2020 09:08

pgayvallet requested review from a team as code owners March 25, 2020 09:08

azasypkin approved these changes Mar 25, 2020

View reviewed changes

flash1293 approved these changes Mar 25, 2020

View reviewed changes

fix UT

f6988a7

mikecote approved these changes Mar 25, 2020

View reviewed changes

nchaulet approved these changes Mar 25, 2020

View reviewed changes

lukeelmers approved these changes Mar 25, 2020

View reviewed changes

joshdover approved these changes Mar 25, 2020

View reviewed changes

darnautov approved these changes Mar 26, 2020

View reviewed changes

update mock to allow non-object pluginStartContract

5e7c375

pgayvallet added 2 commits March 26, 2020 08:46

add @typeparam documentation

3d8cb03

Merge remote-tracking branch 'upstream/master' into kbn-61106-start-s…

88533f9

…ervice-plugin-contract

pgayvallet merged commit 468de51 into elastic:master Mar 26, 2020

pgayvallet mentioned this pull request Mar 26, 2020

[7.x] Add plugin start contract to getStartServices return value (#61216) #61415

Merged

pgayvallet mentioned this pull request Apr 1, 2020

[Discuss]: Providing plugin's own start contract from core.getStartServices #61106

Closed

rayafratkina added v7.8.0 and removed v7.8 labels Apr 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add plugin start contract to getStartServices return value #61216

Add plugin start contract to getStartServices return value #61216

pgayvallet commented Mar 25, 2020 •

edited

Loading

elasticmachine commented Mar 25, 2020

pgayvallet left a comment •

edited

Loading

pgayvallet Mar 25, 2020

joshdover Mar 25, 2020

pgayvallet Mar 26, 2020

pgayvallet Mar 25, 2020

pgayvallet Mar 25, 2020

pgayvallet Mar 25, 2020

azasypkin left a comment

flash1293 left a comment

mikecote left a comment

nchaulet left a comment

lukeelmers left a comment

joshdover Mar 25, 2020

pgayvallet Mar 26, 2020

pgayvallet Mar 26, 2020

joshdover Mar 25, 2020

darnautov left a comment

kibanamachine commented Mar 26, 2020

		private readonly plugins = new Map<PluginName, PluginWrapper<unknown, Record<string, unknown>>>();
		private readonly plugins = new Map<PluginName, PluginWrapper<unknown, unknown>>();

		export class VisTypeVislibPlugin implements Plugin<Promise<void>, void> {
		export class VisTypeVislibPlugin implements Plugin<void, void> {

Add plugin start contract to getStartServices return value #61216

Add plugin start contract to getStartServices return value #61216

Conversation

pgayvallet commented Mar 25, 2020 • edited Loading

Summary

Checklist

elasticmachine commented Mar 25, 2020

pgayvallet left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin left a comment

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

mikecote left a comment

Choose a reason for hiding this comment

nchaulet left a comment

Choose a reason for hiding this comment

lukeelmers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darnautov left a comment

Choose a reason for hiding this comment

kibanamachine commented Mar 26, 2020

💚 Build Succeeded

History

pgayvallet commented Mar 25, 2020 •

edited

Loading

pgayvallet left a comment •

edited

Loading