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

[core/ui] bootstrap the legacy platform within the new platform #20699

Merged
merged 37 commits into from
Jul 18, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
97c86a3
[core/ui] bootstrap the legacy platform within the new platform
Jul 11, 2018
a7fd8fb
fix test that parsed uiApp response
Jul 11, 2018
bb73252
fix missed rename
Jul 11, 2018
0a8dcec
use a legacy platform service to bootstrap within the start() lifecycle
Jul 12, 2018
f1a698e
[tests/ui_exports_replace_injected_vars] read vars from legacyMetadata
Jul 12, 2018
9984fa5
[core/injectedMetadata] use less permissive array type to avoid freez…
Jul 12, 2018
6624b58
[ui/metadata/tests] remove unnecessary test
Jul 12, 2018
e514979
Merge branch 'master' of github.com:elastic/kibana into implement/new…
Jul 12, 2018
4f8fe21
implement feedback from Court
Jul 12, 2018
5357110
restore app_entry_template tests
Jul 12, 2018
3c71d11
Merge branch 'master' of github.com:elastic/kibana into implement/new…
Jul 13, 2018
b662c4d
[ui/notify/tests] remove reference to window.__KBN__
Jul 13, 2018
277f273
Merge branch 'master' of github.com:elastic/kibana into implement/new…
Jul 13, 2018
e404fa4
[core] use *StartContract naming convention
Jul 13, 2018
cc1d8f0
Merge branch 'master' of github.com:elastic/kibana into implement/new…
Jul 13, 2018
2151adf
[tslint] ignore fixtures in precommit hook
Jul 13, 2018
04af61a
[core/public] add tests, use *Params in constructors
Jul 13, 2018
9c76bfb
[core/public/tests] fix type definitions
Jul 13, 2018
3073159
[core/public/legacyPlatform] test arguments to legacy bootstrap module
Jul 13, 2018
450e6b1
[core/public/core/tests] use correct services for mock generics
Jul 13, 2018
5a5210b
Merge branch 'master' of github.com:elastic/kibana into implement/new…
Jul 13, 2018
675f032
[core/public] list explicit params for core service
Jul 13, 2018
7ec434b
Merge branch 'master' of github.com:elastic/kibana into implement/new…
Jul 16, 2018
33a74af
[core/public] tweak tests to address feedback
Jul 16, 2018
3e5a2ab
[core/public/core/tests] start the core once in each tests so mocks c…
Jul 16, 2018
e6c5c0b
Merge branch 'master' of github.com:elastic/kibana into implement/new…
Jul 18, 2018
0ec8793
[testBundle] use a Set() to avoid duplication
Jul 18, 2018
794ae2c
[core/public] add comments describing the core services/systems
Jul 18, 2018
fad92a0
[core/public] use `readonly` modifier for params property
Jul 18, 2018
7442290
[uiBundles] add comment about isCacheValid()
Jul 18, 2018
4567aed
[eslintResolver] remove ui_framework alias from webpack config
Jul 18, 2018
4f9f64d
fix references to remove kbn-initial-state element
Jul 18, 2018
874029c
[core/public/tests] use destructuring for readability
Jul 18, 2018
c210fef
[core/public/tests] cleanup in after hook for clarity
Jul 18, 2018
7a4987f
[core/public/deepFreeze] test that freezing arrays is supported
Jul 18, 2018
5b0edf1
[testsBundle] fix typo
Jul 18, 2018
9710387
[core/public/tests] fix typo
Jul 18, 2018
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
49 changes: 49 additions & 0 deletions src/core/public/core_system.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

// import global polyfills before everything else
import 'babel-polyfill';
import 'custom-event-polyfill';
import 'whatwg-fetch';

import { InjectedMetadata, InjectedMetadataService } from './injected_metadata';
import { LegacyPlatformService } from './legacy_platform';

interface CoreSystemParams {
injectedMetadataForTesting?: InjectedMetadata;
bootstrapLegacyPlatform: () => void;
}

export class CoreSystem {
Copy link
Member

Choose a reason for hiding this comment

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

nit: would be really great if you can add a top level comment for CoreSystem explaining what its role and what it's supposed to do.

private injectedMetadata: InjectedMetadataService;
private legacyPlatform: LegacyPlatformService;

constructor(params: CoreSystemParams) {
const { injectedMetadataForTesting, bootstrapLegacyPlatform } = params;

this.injectedMetadata = new InjectedMetadataService(injectedMetadataForTesting);
this.legacyPlatform = new LegacyPlatformService(bootstrapLegacyPlatform);
}

public start() {
this.legacyPlatform.start({
injectedMetadata: this.injectedMetadata.start(),
});
}
}
20 changes: 20 additions & 0 deletions src/core/public/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

export { CoreSystem } from './core_system';
36 changes: 36 additions & 0 deletions src/core/public/injected_metadata/deep_freeze.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

type Freezable = { [k: string]: any } | ArrayLike<any>;

type RecursiveReadOnly<T> = T extends Freezable
? Readonly<{ [K in keyof T]: RecursiveReadOnly<T[K]> }>
: T;

export function deepFreeze<T extends Freezable>(object: T) {
// for any properties that reference an object, makes sure that object is
// recursively frozen as well
for (const value of Object.values(object)) {
if (value !== null && typeof value === 'object') {
deepFreeze(value);
}
}

return Object.freeze(object) as RecursiveReadOnly<T>;
}
20 changes: 20 additions & 0 deletions src/core/public/injected_metadata/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

export { InjectedMetadata, InjectedMetadataService } from './injected_metadata_service';
Original file line number Diff line number Diff line change
Expand Up @@ -17,33 +17,27 @@
* under the License.
*/

import sinon from 'sinon';
import expect from 'expect.js';
import { deepFreeze } from './deep_freeze';
import { readInjectedMetadataFromDom } from './read_injected_metadata_from_dom';

import { appEntryTemplate } from '../app_entry_template';

function createMockBundle() {
return {
getContext: sinon.stub().returns(''),
getRequires: sinon.stub().returns([])
export interface InjectedMetadata {
legacyMetadata: {
[key: string]: any;
};
}

describe('ui bundles / appEntryTemplate', () => {
it('embeds bundle.getContext() result', () => {
const bundle = createMockBundle();
bundle.getContext.returns('foo bar baz');
expect(appEntryTemplate(bundle)).to.contain('foo bar baz');
});
export class InjectedMetadataService {
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: could you please add a top level comment for this service as well?

constructor(private injectedMetadataForTesting?: InjectedMetadata) {}

public start() {
const state = deepFreeze(
this.injectedMetadataForTesting || (readInjectedMetadataFromDom() as InjectedMetadata)
);

it('joins requires into list', () => {
const bundle = createMockBundle();
const requires = [
'foo',
'bar',
'baz'
];
bundle.getRequires.returns(requires);
expect(appEntryTemplate(bundle)).to.contain(requires.join('\n'));
});
});
return {
getLegacyMetadata() {
return state.legacyMetadata;
},
};
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

export function readInjectedMetadataFromDom() {
const el = document.querySelector('kbn-injected-metadata');

if (!el) {
throw new Error('unable to find <kbn-injected-metadata> element');
}

const json = el.getAttribute('data');
if (!json) {
throw new Error('<kbn-injected-metadata> does not have a data atttibute');
}

return JSON.parse(json);
}
20 changes: 20 additions & 0 deletions src/core/public/legacy_platform/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

export { LegacyPlatformService } from './legacy_platform_service';
39 changes: 39 additions & 0 deletions src/core/public/legacy_platform/legacy_platform_service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

question: there is no need to change anything in this PR, just wondering if we should put all legacy related public stuff into public/legacy_compat like we do for server and as mentioned in #12466 (comment) (mentioned only server/legacy_compat though)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be fine renaming the LegacyPlatformService to LegacyCompatService, but I think it makes sense this way too.

* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { InjectedMetadataService } from '../injected_metadata';

interface Deps {
injectedMetadata: ReturnType<InjectedMetadataService['start']>;
Copy link
Contributor Author

@spalger spalger Jul 12, 2018

Choose a reason for hiding this comment

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

I'm really not sure what to name this type, but once we get plugins in the mix we will probably want to export a named type they can use to define the dependencies of their start functions... InjectedMetadataContract? InjectedMetadataStartContract? StartedInjectedMetadata...

Copy link
Contributor

Choose a reason for hiding this comment

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

InjectedMetadataStartContract works for me. I never really intended the word "contract" to be codified, but if that's the language we're using when we talk about this thing, we might as well just write it that way as well.

}

export class LegacyPlatformService {
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: same note about top level comment here as well. We can add these comments in the following PRs if you want though.

constructor(private bootstrapLegacyPlatform: () => void) {}

public start({ injectedMetadata }: Deps) {
/**
* Injects parts of the new platform into parts of the legacy platform
* so that legacy APIs/modules can mimic their new platform counterparts
*/
require('ui/metadata').__newPlatformInit__(injectedMetadata.getLegacyMetadata());

// call the legacy platform bootstrap function (bootstraps ui/chrome in apps and ui/test_harness in browser tests)
this.bootstrapLegacyPlatform();
}
}
12 changes: 6 additions & 6 deletions src/core_plugins/tests_bundle/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@
* under the License.
*/

import { union } from 'lodash';

import { fromRoot } from '../../utils';

import findSourceFiles from './find_source_files';
Expand All @@ -36,7 +34,7 @@ export default (kibana) => {

uiExports: {
async __bundleProvider__(kbnServer) {
let modules = [];
const modules = [];

const {
config,
Expand Down Expand Up @@ -66,8 +64,8 @@ export default (kibana) => {

// add the modules from all of this plugins apps
for (const app of uiApps) {
if (app.getPluginId() === pluginId) {
modules = union(modules, app.getModules());
if (app.getPluginId() === pluginId && !modules.includes(app.getMainModuleId())) {
Copy link
Member

Choose a reason for hiding this comment

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

optional nit: if we use const modules = new Set() we won't need this !....includes checks (that we can accidentally forget about if we decide to add new for for some reason). The only cons is that we'll have to do modules: [...modules] or Array.from(modules) later.

modules.push(app.getMainModuleId());
}
}

Expand All @@ -76,7 +74,9 @@ export default (kibana) => {
} else {
// add the modules from all of the apps
for (const app of uiApps) {
modules = union(modules, app.getModules());
if (!modules.includes(app.getMainModuleId())) {
modules.push(app.getMainModuleId());
}
}

for (const plugin of plugins) {
Expand Down
17 changes: 13 additions & 4 deletions src/core_plugins/tests_bundle/tests_entry_template.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ export const createTestEntryTemplate = (defaultUiSettings) => (bundle) => `
*
*/

window.__KBN__ = {
import { CoreSystem } from '__kibanaCore__'

const legacyMetadata = {
version: '1.2.3',
buildNum: 1234,
vars: {
Expand Down Expand Up @@ -64,7 +66,14 @@ window.__KBN__ = {
}
};

require('ui/test_harness');
${bundle.getRequires().join('\n')}
require('ui/test_harness').bootstrap(/* go! */);
new CoreSystem({
injectedMetadataForTesting: {
legacyMetadata
},
bootstrapLegacyPlatform: () => {
require('ui/test_harness');
${bundle.getRequires().join('\n ')}
require('ui/test_harness').bootstrap();
}
}).start()
`;
8 changes: 2 additions & 6 deletions src/optimize/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,6 @@ export default async (kbnServer, server, config) => {
basePublicPath: config.get('server.basePath')
}));

await uiBundles.writeEntryFiles();

// Not all entry files produce a css asset. Ensuring they exist prevents
// an error from occurring when the file is missing.
await uiBundles.ensureStyleFiles();

// in prod, only bundle when something is missing or invalid
const reuseCache = config.get('optimize.useBundleCache')
? await uiBundles.areAllBundleCachesValid()
Expand All @@ -62,6 +56,8 @@ export default async (kbnServer, server, config) => {
return;
}

await uiBundles.resetBundleDir();

// only require the FsOptimizer when we need to
const optimizer = new FsOptimizer({
uiBundles,
Expand Down
5 changes: 1 addition & 4 deletions src/optimize/watch/watch_optimizer.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,7 @@ export default class WatchOptimizer extends BaseOptimizer {

// log status changes
this.status$.subscribe(this.onStatusChangeHandler);

await this.uiBundles.writeEntryFiles();
await this.uiBundles.ensureStyleFiles();

await this.uiBundles.resetBundleDir();
await this.initCompiler();

this.compiler.plugin('watch-run', this.compilerRunStartHandler);
Expand Down
2 changes: 1 addition & 1 deletion src/ui/__tests__/ui_exports_replace_injected_vars.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import KbnServer from '../../server/kbn_server';

const getInjectedVarsFromResponse = (resp) => {
const $ = cheerio.load(resp.payload);
const data = $('kbn-initial-state').attr('data');
const data = $('kbn-injected-metadata').attr('data');
return JSON.parse(data).vars;
};

Expand Down
3 changes: 0 additions & 3 deletions src/ui/public/chrome/chrome.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,6 @@ import _ from 'lodash';
import angular from 'angular';

import { metadata } from '../metadata';
import 'babel-polyfill';
import 'whatwg-fetch';
import 'custom-event-polyfill';
import '../state_management/global_state';
import '../config';
import '../notify';
Expand Down
Loading