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

spalger · 2018-07-11T23:03:39Z

Fixes #20694

Implements super basic new platform core system, which includes two services: core.injectedMetadata and core.legacyPlatform. The core currently has two responsibilities:

read the metadata from the DOM and initialize the ui/metadata module with legacy metadata, proving out how we plan to expose data from the new platform through the existing APIs/modules to the legacy platform.
bootstrap the legacy platform by loading either ui/chrome or ui/test_harness

Because core mutates the ui/metadata module before bootstrapping the legacy platform all existing consumers of ui/metadata won't be impacted by the fact that metadata loading was moved into the new platform. We plan to do this for many other services that will need to exist in both the legacy and new platforms, like ui/chrome (see #20696).

epixa · 2018-07-11T23:56:32Z

src/core_plugins/tests_bundle/tests_entry_template.js

+ injectedMetadataForTesting: {
+ legacyMetadata
+ }
+}).initLegacyPlatform(() => {


The convention we've talked about is having a core start function and then invoking legacy startup/bootstrap through a legacy service within the core system. This way we can identify precisely when the legacy code kicks off in the context of all the new platform services we'd be rolling out.

Is there a reason we need to abandon that approach for this initLegacyPlatform approach?

Nope, happy to move it there.

I just didn't want to worry about start()/stop() right now, but it's not a big deal.

spalger · 2018-07-12T00:17:25Z

src/core/public/legacy_platform/legacy_platform_service.ts

+import { InjectedMetadataService } from '../injected_metadata';
+
+interface Deps {
+ injectedMetadata: ReturnType<InjectedMetadataService['start']>;


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...

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.

…ing strings

…-platform-bootstrap

spalger · 2018-07-12T22:03:57Z

@epixa this is ready for another look

…-platform-bootstrap

elasticmachine · 2018-07-13T01:55:17Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

epixa

The changes here look good, and I spun up Kibana and clicked around, everything seemed unaffected by this change as we'd expect.

This all needs unit tests though.

…-platform-bootstrap

epixa · 2018-07-16T22:23:05Z

src/core/public/core_system.test.ts

@@ -100,16 +100,22 @@ describe('constructor', () => {
 });

 describe('#start()', () => {
- it('calls lifecycleSystem#start() and injectedMetadata#start()', () => {
- const core = new CoreSystem({
+ let core;


I thought you were the one in particular that hated this pattern and wanted everyone to use setup functions instead!

Haha, I do, but setup functions are best because they take arguments, when there aren't any arguments I'm not super picky. But point taken!

There is also the issue that the mocks are cleared between each test, so starting core once and reusing the value for each test wouldn't work anyway.

epixa · 2018-07-16T22:23:51Z

Still LGTM

…an be cleared

elasticmachine · 2018-07-17T00:08:52Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

epixa · 2018-07-17T00:16:13Z

And still LGTM

azasypkin

Code looks good to me, just few nits and questions. Running PR locally and approve it after a smoke test :)

azasypkin · 2018-07-17T09:17:21Z

src/core_plugins/tests_bundle/index.js

@@ -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())) {


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.

azasypkin · 2018-07-17T09:23:59Z

src/dev/tslint/pick_files_to_lint.ts

@@ -22,5 +22,5 @@ import { ToolingLog } from '@kbn/dev-utils';
 import { File } from '../file';

 export function pickFilesToLint(log: ToolingLog, files: File[]) {
- return files.filter(file => file.isTypescript());
+ return files.filter(file => file.isTypescript() && !file.isFixture());


question: what issues do we have without this? Can't we have nice linted fixtures? :)

The issue this is that the fixture files are excluded from our main TS project because they can (and in my case need to) include invalid typescript for the purpose of making sure our types impose certain limitations. Since the files are not a part of a TS project they fail the pre-commit hook since TSLint needs the project file for a specific file to run the type-aware checks.

It's possible that we could run fixtures through a linting phase that isn't type enabled, but I'd rather do that in a separate PR. See #20910

Ah, got it, thanks for explanation.

azasypkin · 2018-07-17T09:31:28Z

src/core/public/core_system.ts

+ useLegacyTestHarness?: LegacyPlatformParams['useLegacyTestHarness'];
+}
+
+export class CoreSystem {


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.

azasypkin · 2018-07-17T09:53:42Z

src/core/public/injected_metadata/deep_freeze.ts

+ * under the License.
+ */
+
+type Freezable = { [k: string]: any } | any[];


question: hmm, | any[] - is it really supposed to work for arrays as expected? I don't see any test that would check that case.

Object.freeze works just fine on arrays, but you're right that there are not tests for this. I'll add them.

azasypkin · 2018-07-17T09:58:15Z

src/core/public/injected_metadata/injected_metadata_service.ts

+ };
+}
+
+export class InjectedMetadataService {


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

azasypkin · 2018-07-17T10:31:07Z

src/core/public/core_system.test.ts

+
+ it('calls injectedMetadata#start()', () => {
+ startCore();
+ expect(MockInjectedMetadataService.mock.instances[0].start).toHaveBeenCalledTimes(1);


optional nit: maybe destructuring can make it a little bit more readable: const [mockInstance] = MockInjectedMetadataService.mock.instances;

azasypkin · 2018-07-17T10:35:33Z

src/ui/ui_render/views/chrome.jade

- body(kbn-chrome, id='#{appName}-body')
- kbn-initial-state(data=JSON.stringify(kibanaPayload))
+ body
+ kbn-injected-metadata(data=JSON.stringify(injectedMetadata))


nit: can you please also remove the last kbn-initial-state occurrence from

kibana/src/core_plugins/console/public/tests/src/integration.test.js

Line 36 in 831b9f8

'<div><div id="editor" /><div id="editor_actions" /><div id="copy_as_curl" /><kbn-initial-state data="{}"/></div>';

and update comment here

kibana/src/core_plugins/state_session_storage_redirect/package.json

Line 4 in 6d81baf

"description": "When using the state:storeInSessionStorage setting with the short-urls, we need some way to get the full URL's hashed states into sessionStorage, this app will grab the URL from the kbn-initial-state and and put the URL hashed states into sessionStorage before redirecting the user."

?

azasypkin · 2018-07-17T10:41:02Z

src/ui/ui_apps/ui_app.js

- getModules() {
- return this._main ? [this._main] : [];
+ getMainModuleId() {
+ return this._main;


azasypkin · 2018-07-17T10:47:51Z

src/ui/ui_bundles/ui_bundle.js

- await fcb(cb => unlink(this.getOutputPath(), cb));
- } catch (e) {
- return null;
+ async isCacheValid() {


question: while you're here, would you mind adding comment for isCacheValid explaining how it works exactly?

azasypkin · 2018-07-17T10:54:57Z

src/ui/ui_exports/ui_export_defaults.js

@@ -28,7 +28,7 @@ export const UI_EXPORT_DEFAULTS = {

 webpackAliases: {
 ui: resolve(ROOT, 'src/ui/public'),
- ui_framework: resolve(ROOT, 'ui_framework'),


question: btw, not related to this PR, do we still need ui_framework here

kibana/packages/kbn-eslint-import-resolver-kibana/lib/get_webpack_config.js

Line 31 in cb5ee01

ui_framework: fromKibana('ui_framework'),

?

No, I don't think so.

azasypkin

Tested locally in dev and non-dev modes, haven't noticed anything suspicious.

…-platform-bootstrap

elasticmachine · 2018-07-18T02:42:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…tic#20699) Fixes elastic#20694 Implements super basic new platform `core` system, which includes two services: `core.injectedMetadata` and `core.legacyPlatform`. The `core` currently has two responsibilities: 1. read the metadata from the DOM and initialize the `ui/metadata` module with legacy metadata, proving out how we plan to expose data from the new platform through the existing APIs/modules to the legacy platform. 2. bootstrap the legacy platform by loading either `ui/chrome` or `ui/test_harness` Because `core` mutates the `ui/metadata` module before bootstrapping the legacy platform all existing consumers of `ui/metadata` won't be impacted by the fact that metadata loading was moved into the new platform. We plan to do this for many other services that will need to exist in both the legacy and new platforms, like `ui/chrome` (see elastic#20696).

…) (#20913) Fixes #20694 Implements super basic new platform `core` system, which includes two services: `core.injectedMetadata` and `core.legacyPlatform`. The `core` currently has two responsibilities: 1. read the metadata from the DOM and initialize the `ui/metadata` module with legacy metadata, proving out how we plan to expose data from the new platform through the existing APIs/modules to the legacy platform. 2. bootstrap the legacy platform by loading either `ui/chrome` or `ui/test_harness` Because `core` mutates the `ui/metadata` module before bootstrapping the legacy platform all existing consumers of `ui/metadata` won't be impacted by the fact that metadata loading was moved into the new platform. We plan to do this for many other services that will need to exist in both the legacy and new platforms, like `ui/chrome` (see #20696).

spalger · 2018-07-18T06:56:23Z

6.4/6.x: 360c50b

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

97c86a3

spalger added WIP Work in progress Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.0.0 v6.4.0 labels Jul 11, 2018

fix test that parsed uiApp response

a7fd8fb

epixa reviewed Jul 11, 2018

View reviewed changes

fix missed rename

bb73252

This comment has been minimized.

Sign in to view

use a legacy platform service to bootstrap within the start() lifecycle

0a8dcec

spalger commented Jul 12, 2018

View reviewed changes

[tests/ui_exports_replace_injected_vars] read vars from legacyMetadata

f1a698e

This comment has been minimized.

Sign in to view

spalger added 2 commits July 12, 2018 11:38

[core/injectedMetadata] use less permissive array type to avoid freez…

9984fa5

…ing strings

[ui/metadata/tests] remove unnecessary test

6624b58

This comment has been minimized.

Sign in to view

spalger added 2 commits July 12, 2018 14:28

Merge branch 'master' of github.com:elastic/kibana into implement/new…

e514979

…-platform-bootstrap

implement feedback from Court

4f8fe21

spalger force-pushed the implement/new-platform-bootstrap branch from d1bcd06 to 4f8fe21 Compare July 12, 2018 21:58

restore app_entry_template tests

5357110

spalger force-pushed the implement/new-platform-bootstrap branch from 4bfb104 to 5357110 Compare July 12, 2018 22:03

This comment has been minimized.

Sign in to view

spalger added 2 commits July 12, 2018 17:31

Merge branch 'master' of github.com:elastic/kibana into implement/new…

3c71d11

…-platform-bootstrap

[ui/notify/tests] remove reference to window.__KBN__

b662c4d

spalger mentioned this pull request Jul 13, 2018

Migrate ui/notify/fatal_error to new platform #20752

Merged

epixa suggested changes Jul 13, 2018

View reviewed changes

spalger added 2 commits July 13, 2018 07:25

Merge branch 'master' of github.com:elastic/kibana into implement/new…

277f273

…-platform-bootstrap

[core] use *StartContract naming convention

e404fa4

[core/public] tweak tests to address feedback

33a74af

epixa reviewed Jul 16, 2018

View reviewed changes

[core/public/core/tests] start the core once in each tests so mocks c…

3e5a2ab

…an be cleared

spalger force-pushed the implement/new-platform-bootstrap branch from 127ef04 to 3e5a2ab Compare July 16, 2018 22:54

azasypkin reviewed Jul 17, 2018

View reviewed changes

azasypkin approved these changes Jul 17, 2018

View reviewed changes

spalger added 12 commits July 17, 2018 17:08

Merge branch 'master' of github.com:elastic/kibana into implement/new…

e6c5c0b

…-platform-bootstrap

[testBundle] use a Set() to avoid duplication

0ec8793

[core/public] add comments describing the core services/systems

794ae2c

[core/public] use readonly modifier for params property

fad92a0

[uiBundles] add comment about isCacheValid()

7442290

[eslintResolver] remove ui_framework alias from webpack config

4567aed

fix references to remove kbn-initial-state element

4f9f64d

[core/public/tests] use destructuring for readability

874029c

[core/public/tests] cleanup in after hook for clarity

c210fef

[core/public/deepFreeze] test that freezing arrays is supported

7a4987f

[testsBundle] fix typo

5b0edf1

[core/public/tests] fix typo

9710387

spalger merged commit 8662475 into elastic:master Jul 18, 2018

spalger mentioned this pull request Jul 18, 2018

[6.x] [core/ui] bootstrap the legacy platform within the new platform (#20699) #20913

Merged

spalger deleted the implement/new-platform-bootstrap branch July 18, 2018 06:56

This was referenced Jul 18, 2018

Move del to dependencies #20921

Merged

CI never tests the pure runtime build #20922

Closed

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

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

Conversation

spalger commented Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

spalger Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

spalger commented Jul 12, 2018

This comment has been minimized.

elasticmachine commented Jul 13, 2018

💚 Build Succeeded

epixa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

epixa commented Jul 16, 2018

elasticmachine commented Jul 17, 2018

💚 Build Succeeded

epixa commented Jul 17, 2018

azasypkin 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

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

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

elasticmachine commented Jul 18, 2018

💚 Build Succeeded

spalger commented Jul 18, 2018

spalger commented Jul 11, 2018 •

edited

Loading

spalger Jul 12, 2018 •

edited

Loading

spalger Jul 16, 2018 •

edited

Loading