Skip to content

Commit

Permalink
[chore] Make plugin dependencies explicit; point out lifecycle incons…
Browse files Browse the repository at this point in the history
…istencies (elastic#173297)

> Changes identified using the script from
elastic#171483

## Summary

The script from elastic#171483 can
identify inconsistencies and untyped dependencies in Kibana plugins.
This PR fixes the obvious:

- `notifications` - move `actions` and `licensing` to `optionalPlugins`.
- `serverless` - move `kibanaReact` to `requiredBundles`.
- `serverlessObservability` - delete dead code and `kibanaReact`
dependency.
- `reporting` - move `esUiShared` and `embeddable` plugins to
`requiredBundles`.
- `uiActions` - remove `dataViews` dependency, (only a type is being
used).
- `urlDrilldowns` - move `uiActions` to `requiredBundles`.
- Type all plugins using the `Setup` and `Start` generics on the core
`Plugin` interface.
- Consistently name them.
- The exports needed to be named their original names; this will be
addressed in follow up work, (to avoid pinging teams)
- Add a `_` to unused parameters.

## Remaining Issues

### `licensing` and `licensingApiGuard`

Both of these plugins introduce side-effects, rather than dependent
logic. These need to be refactored to be consumed instead.

<img width="735" alt="Screenshot 2023-12-13 at 10 08 00 AM"
src="https://github.com/elastic/kibana/assets/297604/57916ffd-299d-4ca8-b796-dea2d06dca4a">
<img width="740" alt="Screenshot 2023-12-13 at 10 08 08 AM"
src="https://github.com/elastic/kibana/assets/297604/a2af254f-adec-4bf9-869a-8acf34c0c9b4">

## Resolved issues

### `reporting`

~~The `reporting` plugin requires `embeddable` and `esUiShared`, but
it's unclear if these still apply, or if they are required for
side-effects. Perhaps @tsullivan can help clarify?~~

Both are being used for static code. Moving to `requiredBundles`, and
need to follow-up to create packages.

<img width="800" alt="Screenshot 2023-12-13 at 10 08 23 AM"
src="https://github.com/elastic/kibana/assets/297604/7629fb92-d28e-43de-bfeb-97410cff424e">

### `uiActions`

~~The `uiActions` plugin requires `dataViews`. We need to determine if
this is a side-effect dependency, or a direct dependency, and remove or
refactor as necessary.~~

It's only using a type. Removing the dependency entirely,
(`requiredBundles` requires actual code be used).

<img width="622" alt="Screenshot 2023-12-13 at 10 08 33 AM"
src="https://github.com/elastic/kibana/assets/297604/39916f05-dafc-4f42-b5d8-1abcb1267b5b">

### urlDrilldown

~~The `urlDrilldown` plugin requires `uiActions`. We need to determine
if this is a side-effect dependency, or a direct dependency, and remove
or refactor as necessary.~~

Static code usage-- moving to `requiredBundles`.

<img width="732" alt="Screenshot 2023-12-13 at 10 13 13 AM"
src="https://github.com/elastic/kibana/assets/297604/af32f939-f866-483d-8dd0-aab962397dbb">

---------

Co-authored-by: kibanamachine <[email protected]>
  • Loading branch information
clintandrewhall and kibanamachine authored Dec 18, 2023
1 parent b501281 commit 1c99f40
Show file tree
Hide file tree
Showing 59 changed files with 435 additions and 198 deletions.
11 changes: 9 additions & 2 deletions src/plugins/content_management/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,20 @@ import type { Context as RpcContext } from './rpc';
import {
ContentManagementServerSetup,
ContentManagementServerStart,
SetupDependencies,
ContentManagementServerSetupDependencies,
ContentManagementServerStartDependencies,
} from './types';
import { EventStreamService } from './event_stream';
import { procedureNames } from '../common/rpc';

export class ContentManagementPlugin
implements Plugin<ContentManagementServerSetup, ContentManagementServerStart, SetupDependencies>
implements
Plugin<
ContentManagementServerSetup,
ContentManagementServerStart,
ContentManagementServerSetupDependencies,
ContentManagementServerStartDependencies
>
{
private readonly logger: Logger;
private readonly core: Core;
Expand Down
5 changes: 4 additions & 1 deletion src/plugins/content_management/server/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@
import { CoreApi } from './core';

// eslint-disable-next-line @typescript-eslint/no-empty-interface
export interface SetupDependencies {}
export interface ContentManagementServerSetupDependencies {}

// eslint-disable-next-line @typescript-eslint/no-empty-interface
export interface ContentManagementServerStartDependencies {}

// eslint-disable-next-line @typescript-eslint/no-empty-interface
export interface ContentManagementServerSetup extends CoreApi {}
Expand Down
4 changes: 3 additions & 1 deletion src/plugins/files/public/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
*/

import { FilesPlugin } from './plugin';
export type { FilesSetup, FilesStart } from './plugin';

export type { FilesPublicSetup as FilesSetup, FilesPublicStart as FilesStart } from './plugin';

export type {
FilesClient,
ScopedFilesClient,
Expand Down
4 changes: 2 additions & 2 deletions src/plugins/files/public/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

import { createMockFilesClient as createBaseMocksFilesClient } from '@kbn/shared-ux-file-mocks';
import type { DeeplyMockedKeys } from '@kbn/utility-types-jest';
import { FilesSetup } from '.';
import { FilesPublicSetup } from './plugin';
import type { FilesClient, FilesClientFactory } from './types';

export const createMockFilesClient = (): DeeplyMockedKeys<FilesClient> => ({
Expand All @@ -17,7 +17,7 @@ export const createMockFilesClient = (): DeeplyMockedKeys<FilesClient> => ({
publicDownload: jest.fn(),
});

export const createMockFilesSetup = (): DeeplyMockedKeys<FilesSetup> => {
export const createMockFilesSetup = (): DeeplyMockedKeys<FilesPublicSetup> => {
return {
filesClientFactory: createMockFilesClientFactory(),
registerFileKind: jest.fn(),
Expand Down
25 changes: 19 additions & 6 deletions src/plugins/files/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,12 @@
*/

import type { CoreSetup, CoreStart, Plugin } from '@kbn/core/public';
import type { FilesClient, FilesClientFactory } from './types';
import type {
FilesClient,
FilesClientFactory,
FilesPublicSetupDependencies,
FilesPublicStartDependencies,
} from './types';
import { FileKindsRegistryImpl } from '../common/file_kinds_registry';
import { createFilesClient } from './files_client';
import { FileKindBrowser } from '../common';
Expand All @@ -17,7 +22,7 @@ import * as DefaultImageFileKind from '../common/default_image_file_kind';
/**
* Public setup-phase contract
*/
export interface FilesSetup {
export interface FilesPublicSetup {
/**
* A factory for creating an {@link FilesClient} instance. This requires a
* registered {@link FileKindBrowser}.
Expand All @@ -35,19 +40,27 @@ export interface FilesSetup {
registerFileKind(fileKind: FileKindBrowser): void;
}

export type FilesStart = Pick<FilesSetup, 'filesClientFactory'> & {
export type FilesPublicStart = Pick<FilesPublicSetup, 'filesClientFactory'> & {
getFileKindDefinition: (id: string) => FileKindBrowser;
getAllFindKindDefinitions: () => FileKindBrowser[];
};

/**
* Bringing files to Kibana
*/
export class FilesPlugin implements Plugin<FilesSetup, FilesStart> {
export class FilesPlugin
implements
Plugin<
FilesPublicSetup,
FilesPublicStart,
FilesPublicSetupDependencies,
FilesPublicStartDependencies
>
{
private registry = new FileKindsRegistryImpl<FileKindBrowser>();
private filesClientFactory?: FilesClientFactory;

setup(core: CoreSetup): FilesSetup {
setup(core: CoreSetup): FilesPublicSetup {
this.registry.register({
...DefaultImageFileKind.kind,
maxSizeBytes: DefaultImageFileKind.maxSize,
Expand Down Expand Up @@ -77,7 +90,7 @@ export class FilesPlugin implements Plugin<FilesSetup, FilesStart> {
};
}

start(core: CoreStart): FilesStart {
start(core: CoreStart): FilesPublicStart {
return {
filesClientFactory: this.filesClientFactory!,
getFileKindDefinition: (id: string): FileKindBrowser => {
Expand Down
6 changes: 6 additions & 0 deletions src/plugins/files/public/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,9 @@ export type {
FilesClientFactory,
FilesClientResponses,
} from '../common/files_client';

// eslint-disable-next-line @typescript-eslint/no-empty-interface
export interface FilesPublicSetupDependencies {}

// eslint-disable-next-line @typescript-eslint/no-empty-interface
export interface FilesPublicStartDependencies {}
2 changes: 1 addition & 1 deletion src/plugins/files/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export { createEsFileClient } from './file_client';

export { createFileHashTransform } from './file_client/stream_transforms/file_hash_transform';

export type { FilesSetup, FilesStart } from './types';
export type { FilesServerSetup as FilesSetup, FilesServerStart as FilesStart } from './types';
export type {
FileShareServiceStart,
CreateShareArgs,
Expand Down
28 changes: 18 additions & 10 deletions src/plugins/files/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,23 +25,31 @@ import {
import { BlobStorageService } from './blob_storage_service';
import { FileServiceFactory } from './file_service';
import type {
FilesPluginSetupDependencies,
FilesPluginStartDependencies,
FilesSetup,
FilesStart,
FilesServerSetupDependencies,
FilesServerStartDependencies,
FilesServerSetup,
FilesServerStart,
} from './types';

import type { FilesRequestHandlerContext, FilesRouter } from './routes/types';
import { registerRoutes, registerFileKindRoutes } from './routes';
import { Counters, registerUsageCollector } from './usage';
import * as DefaultImageKind from '../common/default_image_file_kind';

export class FilesPlugin implements Plugin<FilesSetup, FilesStart, FilesPluginSetupDependencies> {
export class FilesPlugin
implements
Plugin<
FilesServerSetup,
FilesServerStart,
FilesServerSetupDependencies,
FilesServerStartDependencies
>
{
private static analytics?: AnalyticsServiceStart;
private readonly logger: Logger;
private fileServiceFactory: undefined | FileServiceFactory;
private securitySetup: FilesPluginSetupDependencies['security'];
private securityStart: FilesPluginStartDependencies['security'];
private securitySetup: FilesServerSetupDependencies['security'];
private securityStart: FilesServerStartDependencies['security'];

constructor(initializerContext: PluginInitializerContext) {
this.logger = initializerContext.logger.get();
Expand All @@ -57,8 +65,8 @@ export class FilesPlugin implements Plugin<FilesSetup, FilesStart, FilesPluginSe

public setup(
core: CoreSetup,
{ security, usageCollection }: FilesPluginSetupDependencies
): FilesSetup {
{ security, usageCollection }: FilesServerSetupDependencies
): FilesServerSetup {
const usageCounter = usageCollection?.createUsageCounter(PLUGIN_ID);
FileServiceFactory.setup(core.savedObjects, usageCounter);
this.securitySetup = security;
Expand Down Expand Up @@ -101,7 +109,7 @@ export class FilesPlugin implements Plugin<FilesSetup, FilesStart, FilesPluginSe
};
}

public start(coreStart: CoreStart, { security }: FilesPluginStartDependencies): FilesStart {
public start(coreStart: CoreStart, { security }: FilesServerStartDependencies): FilesServerStart {
const { savedObjects, analytics } = coreStart;
this.securityStart = security;
FilesPlugin.setAnalytics(analytics);
Expand Down
22 changes: 11 additions & 11 deletions src/plugins/files/server/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { FileServiceFactory } from './file_service/file_service_factory';
/**
* Files plugin setup contract
*/
export interface FilesSetup {
export interface FilesServerSetup {
/**
* Register a {@link FileKind} which allows for specifying details about the files
* that will be uploaded.
Expand All @@ -25,23 +25,23 @@ export interface FilesSetup {
registerFileKind(fileKind: FileKind): void;
}

export interface FilesPluginSetupDependencies {
security?: SecurityPluginSetup;
usageCollection?: UsageCollectionSetup;
}

export interface FilesPluginStartDependencies {
security?: SecurityPluginStart;
}

/**
* Files plugin start contract
*/
export interface FilesStart {
export interface FilesServerStart {
/**
* Create an instance of {@link FileServiceStart}.
*
* @track-adoption
*/
fileServiceFactory: FileServiceFactory;
}

export interface FilesServerSetupDependencies {
security?: SecurityPluginSetup;
usageCollection?: UsageCollectionSetup;
}

export interface FilesServerStartDependencies {
security?: SecurityPluginStart;
}
2 changes: 1 addition & 1 deletion src/plugins/kibana_utils/public/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ export {
} from './history';
export { applyDiff } from './state_management/utils/diff_object';

export type { KibanaUtilsSetup } from './plugin';
export type { KibanaUtilsPublicSetup as KibanaUtilsSetup, KibanaUtilsPublicStart } from './plugin';

export function plugin(initializerContext: PluginInitializerContext) {
return new KibanaUtilsPublicPlugin(initializerContext);
Expand Down
6 changes: 3 additions & 3 deletions src/plugins/kibana_utils/public/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@
* Side Public License, v 1.
*/

import { KibanaUtilsSetup, KibanaUtilsStart } from './plugin';
import { KibanaUtilsPublicSetup, KibanaUtilsPublicStart } from './plugin';

export type Setup = jest.Mocked<KibanaUtilsSetup>;
export type Start = jest.Mocked<KibanaUtilsStart>;
export type Setup = jest.Mocked<KibanaUtilsPublicSetup>;
export type Start = jest.Mocked<KibanaUtilsPublicStart>;

const createSetupContract = (): Setup => {
return {
Expand Down
24 changes: 19 additions & 5 deletions src/plugins/kibana_utils/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,26 +10,40 @@ import { PluginInitializerContext, CoreSetup, CoreStart, Plugin } from '@kbn/cor
import { History } from 'history';
import { setVersion } from './set_version';

export interface KibanaUtilsSetup {
export interface KibanaUtilsPublicSetup {
setVersion: (history: Pick<History, 'location' | 'replace'>) => void;
}

export type KibanaUtilsStart = undefined;
export type KibanaUtilsPublicStart = undefined;

export class KibanaUtilsPublicPlugin implements Plugin<KibanaUtilsSetup, KibanaUtilsStart> {
// eslint-disable-next-line @typescript-eslint/no-empty-interface
export interface KibanaUtilsPublicSetupDependencies {}

// eslint-disable-next-line @typescript-eslint/no-empty-interface
export interface KibanaUtilsPublicStartDependencies {}

export class KibanaUtilsPublicPlugin
implements
Plugin<
KibanaUtilsPublicSetup,
KibanaUtilsPublicStart,
KibanaUtilsPublicSetupDependencies,
KibanaUtilsPublicStartDependencies
>
{
private readonly version: string;

constructor(initializerContext: PluginInitializerContext) {
this.version = initializerContext.env.packageInfo.version;
}

public setup(core: CoreSetup): KibanaUtilsSetup {
public setup(_core: CoreSetup): KibanaUtilsPublicSetup {
return {
setVersion: this.setVersion,
};
}

public start(core: CoreStart): KibanaUtilsStart {
public start(_core: CoreStart): KibanaUtilsPublicStart {
return undefined;
}

Expand Down
5 changes: 4 additions & 1 deletion src/plugins/navigation/public/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@ export function plugin(initializerContext: PluginInitializerContext) {
export type { TopNavMenuData, TopNavMenuProps, TopNavMenuBadgeProps } from './top_nav_menu';
export { TopNavMenu, TopNavMenuItems, TopNavMenuBadges } from './top_nav_menu';

export type { NavigationPublicPluginSetup, NavigationPublicPluginStart } from './types';
export type {
NavigationPublicSetup as NavigationPublicPluginSetup,
NavigationPublicStart as NavigationPublicPluginStart,
} from './types';

// Export plugin after all other imports
import { NavigationPublicPlugin } from './plugin';
Expand Down
25 changes: 16 additions & 9 deletions src/plugins/navigation/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,22 +9,29 @@
import { PluginInitializerContext, CoreSetup, CoreStart, Plugin } from '@kbn/core/public';
import { UnifiedSearchPublicPluginStart } from '@kbn/unified-search-plugin/public';
import {
NavigationPublicPluginSetup,
NavigationPublicPluginStart,
NavigationPluginStartDependencies,
NavigationPublicSetup,
NavigationPublicStart,
NavigationPublicSetupDependencies,
NavigationPublicStartDependencies,
} from './types';
import { TopNavMenuExtensionsRegistry, createTopNav } from './top_nav_menu';
import { RegisteredTopNavMenuData } from './top_nav_menu/top_nav_menu_data';

export class NavigationPublicPlugin
implements Plugin<NavigationPublicPluginSetup, NavigationPublicPluginStart>
implements
Plugin<
NavigationPublicSetup,
NavigationPublicStart,
NavigationPublicSetupDependencies,
NavigationPublicStartDependencies
>
{
private readonly topNavMenuExtensionsRegistry: TopNavMenuExtensionsRegistry =
new TopNavMenuExtensionsRegistry();

constructor(initializerContext: PluginInitializerContext) {}
constructor(_initializerContext: PluginInitializerContext) {}

public setup(core: CoreSetup): NavigationPublicPluginSetup {
public setup(_core: CoreSetup): NavigationPublicSetup {
return {
registerMenuItem: this.topNavMenuExtensionsRegistry.register.bind(
this.topNavMenuExtensionsRegistry
Expand All @@ -33,9 +40,9 @@ export class NavigationPublicPlugin
}

public start(
core: CoreStart,
{ unifiedSearch }: NavigationPluginStartDependencies
): NavigationPublicPluginStart {
_core: CoreStart,
{ unifiedSearch }: NavigationPublicStartDependencies
): NavigationPublicStart {
const extensions = this.topNavMenuExtensionsRegistry.getAll();

/*
Expand Down
Loading

0 comments on commit 1c99f40

Please sign in to comment.