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

Introduce UI PluginsService #32672

Merged
merged 20 commits into from
Apr 3, 2019
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/core/public/core_system.test.mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { i18nServiceMock } from './i18n/i18n_service.mock';
import { injectedMetadataServiceMock } from './injected_metadata/injected_metadata_service.mock';
import { legacyPlatformServiceMock } from './legacy/legacy_service.mock';
import { notificationServiceMock } from './notifications/notifications_service.mock';
import { pluginsServiceMock } from './plugins/plugins_service.mock';
import { uiSettingsServiceMock } from './ui_settings/ui_settings_service.mock';

export const MockLegacyPlatformService = legacyPlatformServiceMock.create();
Expand Down Expand Up @@ -90,3 +91,9 @@ export const ChromeServiceConstructor = jest.fn().mockImplementation(() => MockC
jest.doMock('./chrome', () => ({
ChromeService: ChromeServiceConstructor,
}));

export const MockPluginsService = pluginsServiceMock.create();
export const PluginsServiceConstructor = jest.fn().mockImplementation(() => MockPluginsService);
jest.doMock('./plugins', () => ({
PluginsService: PluginsServiceConstructor,
}));
79 changes: 46 additions & 33 deletions src/core/public/core_system.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
* under the License.
*/

import { Observable } from 'rxjs';
import { take } from 'rxjs/operators';

import {
BasePathServiceConstructor,
ChromeServiceConstructor,
Expand All @@ -33,12 +36,14 @@ import {
MockInjectedMetadataService,
MockLegacyPlatformService,
MockNotificationsService,
MockPluginsService,
MockUiSettingsService,
NotificationServiceConstructor,
UiSettingsServiceConstructor,
} from './core_system.test.mocks';

import { CoreSystem } from './core_system';

jest.spyOn(CoreSystem.prototype, 'stop');

const defaultCoreSystemParams = {
Expand Down Expand Up @@ -113,7 +118,7 @@ describe('constructor', () => {

expect(NotificationServiceConstructor).toHaveBeenCalledTimes(1);
expect(NotificationServiceConstructor).toHaveBeenCalledWith({
targetDomElement: expect.any(HTMLElement),
targetDomElement$: expect.any(Observable),
Copy link
Contributor

Choose a reason for hiding this comment

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

optional: I’d remove this assertion, correctness of arguments is checked by TS compiler

});
});

Expand Down Expand Up @@ -197,79 +202,84 @@ describe('#stop', () => {
expect(MockI18nService.stop).toHaveBeenCalled();
});

it('clears the rootDomElement', () => {
it('clears the rootDomElement', async () => {
const rootDomElement = document.createElement('div');
const coreSystem = createCoreSystem({
rootDomElement,
});

coreSystem.setup();
await coreSystem.setup();
expect(rootDomElement.innerHTML).not.toBe('');
coreSystem.stop();
await coreSystem.stop();
expect(rootDomElement.innerHTML).toBe('');
});
});

describe('#setup()', () => {
function setupCore(rootDomElement = defaultCoreSystemParams.rootDomElement) {
const core = createCoreSystem({
...defaultCoreSystemParams,
rootDomElement,
});

return core.setup();
}

it('clears the children of the rootDomElement and appends container for legacyPlatform and notifications', () => {
it('clears the children of the rootDomElement and appends container for legacyPlatform and notifications', async () => {
const root = document.createElement('div');
root.innerHTML = '<p>foo bar</p>';
setupCore(root);
await setupCore(root);
expect(root.innerHTML).toBe('<div></div><div></div>');
});

it('calls injectedMetadata#setup()', () => {
setupCore();

it('calls injectedMetadata#setup()', async () => {
await setupCore();
expect(MockInjectedMetadataService.setup).toHaveBeenCalledTimes(1);
});

it('calls http#setup()', () => {
setupCore();
it('calls http#setup()', async () => {
await setupCore();
expect(MockHttpService.setup).toHaveBeenCalledTimes(1);
});

it('calls basePath#setup()', () => {
setupCore();
it('calls basePath#setup()', async () => {
await setupCore();
expect(MockBasePathService.setup).toHaveBeenCalledTimes(1);
});

it('calls uiSettings#setup()', () => {
setupCore();
it('calls uiSettings#setup()', async () => {
await setupCore();
expect(MockUiSettingsService.setup).toHaveBeenCalledTimes(1);
});

it('calls i18n#setup()', () => {
setupCore();
it('calls i18n#setup()', async () => {
await setupCore();
expect(MockI18nService.setup).toHaveBeenCalledTimes(1);
});

it('calls fatalErrors#setup()', () => {
setupCore();
it('calls fatalErrors#setup()', async () => {
await setupCore();
expect(MockFatalErrorsService.setup).toHaveBeenCalledTimes(1);
});

it('calls notifications#setup()', () => {
setupCore();
it('calls notifications#setup()', async () => {
await setupCore();
expect(MockNotificationsService.setup).toHaveBeenCalledTimes(1);
});

it('calls chrome#setup()', () => {
setupCore();
it('calls chrome#setup()', async () => {
await setupCore();
expect(MockChromeService.setup).toHaveBeenCalledTimes(1);
});

it('calls plugin#setup()', async () => {
await setupCore();
expect(MockPluginsService.setup).toHaveBeenCalledTimes(1);
});
});

describe('LegacyPlatform targetDomElement', () => {
it('only mounts the element when set up, before setting up the legacyPlatformService', () => {
it('only mounts the element when set up, before setting up the legacyPlatformService', async () => {
const rootDomElement = document.createElement('div');
const core = createCoreSystem({
rootDomElement,
Expand All @@ -285,32 +295,35 @@ describe('LegacyPlatform targetDomElement', () => {
expect(targetDomElement).toHaveProperty('parentElement', null);

// setting up the core system should mount the targetDomElement as a child of the rootDomElement
core.setup();
await core.setup();
expect(targetDomElementParentInSetup!).toBe(rootDomElement);
});
});

describe('Notifications targetDomElement', () => {
it('only mounts the element when set up, before setting up the notificationsService', () => {
it('only mounts the element when set up, before setting up the notificationsService', async () => {
const rootDomElement = document.createElement('div');
const core = createCoreSystem({
rootDomElement,
});

let targetDomElementParentInSetup: HTMLElement;
const [[{ targetDomElement$ }]] = NotificationServiceConstructor.mock.calls;

let targetDomElementParentInSetup: HTMLElement | null;
MockNotificationsService.setup.mockImplementation(
(): any => {
targetDomElementParentInSetup = targetDomElement.parentElement;
(targetDomElement$ as Observable<HTMLElement>).pipe(take(1)).subscribe({
next: targetDomElement => {
// The targetDomElement should already be a child once it's received by the NotificationsService
expect(targetDomElement.parentElement).not.toBeNull();
targetDomElementParentInSetup = targetDomElement.parentElement;
},
});
}
);

// targetDomElement should not have a parent element when the LegacyPlatformService is constructed
const [[{ targetDomElement }]] = NotificationServiceConstructor.mock.calls;
expect(targetDomElement).toHaveProperty('parentElement', null);

// setting up the core system should mount the targetDomElement as a child of the rootDomElement
core.setup();
await core.setup();
expect(targetDomElementParentInSetup!).toBe(rootDomElement);
});
});
55 changes: 39 additions & 16 deletions src/core/public/core_system.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@

import './core.css';

import { Subject } from 'rxjs';

import { CoreSetup } from '.';
import { BasePathService } from './base_path';
import { ChromeService } from './chrome';
import { FatalErrorsService } from './fatal_errors';
Expand All @@ -27,6 +30,7 @@ import { I18nService } from './i18n';
import { InjectedMetadataParams, InjectedMetadataService } from './injected_metadata';
import { LegacyPlatformParams, LegacyPlatformService } from './legacy';
import { NotificationsService } from './notifications';
import { PluginsService } from './plugins';
import { UiSettingsService } from './ui_settings';

interface Params {
Expand All @@ -37,6 +41,10 @@ interface Params {
useLegacyTestHarness?: LegacyPlatformParams['useLegacyTestHarness'];
}

/** @internal */
// tslint:disable-next-line no-empty-interface
export interface CoreContext {}

/**
* The CoreSystem is the root of the new platform, and setups all parts
* of Kibana in the UI, including the LegacyPlatform which is managed
Expand All @@ -55,9 +63,10 @@ export class CoreSystem {
private readonly basePath: BasePathService;
private readonly chrome: ChromeService;
private readonly i18n: I18nService;
private readonly plugins: PluginsService;

private readonly rootDomElement: HTMLElement;
private readonly notificationsTargetDomElement: HTMLDivElement;
private readonly notificationsTargetDomElement$: Subject<HTMLDivElement>;
private readonly legacyPlatformTargetDomElement: HTMLDivElement;

constructor(params: Params) {
Expand Down Expand Up @@ -85,15 +94,18 @@ export class CoreSystem {
},
});

this.notificationsTargetDomElement = document.createElement('div');
this.notificationsTargetDomElement$ = new Subject();
this.notifications = new NotificationsService({
targetDomElement: this.notificationsTargetDomElement,
targetDomElement$: this.notificationsTargetDomElement$.asObservable(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why make this change in this PR? It seems unrelated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that LegacyService#setup is async this was necessary to ensure that the NotificationsService does not attempt to render toasts before this DOM element is mounted on the page. Without this change, it's possible a toast that was added by a plugin's setup may get cleared (by the toast timeout) before it was even actually shown to the user.

});
this.http = new HttpService();
this.basePath = new BasePathService();
this.uiSettings = new UiSettingsService();
this.chrome = new ChromeService({ browserSupportsCsp });

const core: CoreContext = {};
this.plugins = new PluginsService(core);

this.legacyPlatformTargetDomElement = document.createElement('div');
this.legacyPlatform = new LegacyPlatformService({
targetDomElement: this.legacyPlatformTargetDomElement,
Expand All @@ -102,14 +114,8 @@ export class CoreSystem {
});
}

public setup() {
public async setup() {
try {
// ensure the rootDomElement is empty
this.rootDomElement.textContent = '';
this.rootDomElement.classList.add('coreSystemRootDomElement');
this.rootDomElement.appendChild(this.notificationsTargetDomElement);
this.rootDomElement.appendChild(this.legacyPlatformTargetDomElement);

const i18n = this.i18n.setup();
const notifications = this.notifications.setup({ i18n });
const injectedMetadata = this.injectedMetadata.setup();
Expand All @@ -127,16 +133,32 @@ export class CoreSystem {
notifications,
});

this.legacyPlatform.setup({
const core: CoreSetup = {
basePath,
chrome,
fatalErrors,
http,
i18n,
injectedMetadata,
fatalErrors,
notifications,
http,
basePath,
uiSettings,
chrome,
});
};

await this.plugins.setup(core);
joshdover marked this conversation as resolved.
Show resolved Hide resolved

// ensure the rootDomElement is empty
this.rootDomElement.textContent = '';
this.rootDomElement.classList.add('coreSystemRootDomElement');

const notificationsTargetDomElement = document.createElement('div');
this.rootDomElement.appendChild(notificationsTargetDomElement);
this.rootDomElement.appendChild(this.legacyPlatformTargetDomElement);

// Only provide the DOM element to notifications once it's attached to the page.
// This prevents notifications from timing out before being displayed.
this.notificationsTargetDomElement$.next(notificationsTargetDomElement);

this.legacyPlatform.setup(core);

return { fatalErrors };
} catch (error) {
Expand All @@ -146,6 +168,7 @@ export class CoreSystem {

public stop() {
this.legacyPlatform.stop();
this.plugins.stop();
this.notifications.stop();
this.http.stop();
this.uiSettings.stop();
Expand Down
7 changes: 6 additions & 1 deletion src/core/public/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,10 @@ import { HttpSetup } from './http';
import { I18nSetup } from './i18n';
import { InjectedMetadataParams, InjectedMetadataSetup } from './injected_metadata';
import { NotificationsSetup, Toast, ToastInput, ToastsSetup } from './notifications';
import { Plugin, PluginInitializer, PluginInitializerContext, PluginSetupContext } from './plugins';
import { UiSettingsClient, UiSettingsSetup, UiSettingsState } from './ui_settings';

export { CoreSystem } from './core_system';
export { CoreContext, CoreSystem } from './core_system';

export interface CoreSetup {
i18n: I18nSetup;
Expand All @@ -50,6 +51,10 @@ export {
ChromeHelpExtension,
InjectedMetadataSetup,
InjectedMetadataParams,
Plugin,
PluginInitializer,
PluginInitializerContext,
PluginSetupContext,
NotificationsSetup,
Toast,
ToastInput,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,9 @@ export class InjectedMetadataService {
return this.state.csp;
},

/**
* An array of frontend plugins in topological order.
*/
getPlugins: () => {
return this.state.uiPlugins;
},
Expand Down
Loading