[new platform] render legacy platform into a container (#21248)

* [core/public/legacyPlatform] render into a child of the rootDomElement * [kbn-chrome] use #kibana-body selector for ensuring root height * after feedback in #20752, clear elements by setting textContent * [browserTests] mount CoreSystem in a child, we require stuff in <body> * [core/public] ensure the rootDomElement is full-screen * add some comments tieing together root style rules * use `readonly` for unchanged properties * avoid unnecessary recreation of default param * comment about why coreSystem is not rendered into body in the tests * use more destructuring * rename argument in callback type signature * fix typo * [core/public/styles] switch from less to css
opensearch-project · Aug 7, 2018 · 1a99df7 · 1a99df7
1 parent 5e5b2ce
commit 1a99df7
Show file tree

Hide file tree

Showing 8 changed files with 100 additions and 51 deletions.
diff --git a/src/core/public/core.css b/src/core/public/core.css
@@ -0,0 +1,9 @@
+/**
+ * designed to emulate root-element stretching and overflow
+ * prevention previously handled in kbn_chrome.less
+ */
+.coreSystemRootDomElement {
+  overflow-x: hidden;
+  min-width: 100%;
+  min-height: 100%;
+}
diff --git a/src/core/public/core_system.test.ts b/src/core/public/core_system.test.ts
@@ -56,7 +56,7 @@ import { CoreSystem } from './core_system';
 jest.spyOn(CoreSystem.prototype, 'stop');
 
 const defaultCoreSystemParams = {
-  rootDomElement: null!,
+  rootDomElement: document.createElement('div'),
   injectedMetadata: {} as any,
   requireLegacyFiles: jest.fn(),
 };
@@ -91,8 +91,8 @@ describe('constructor', () => {
     });
   });
 
-  it('passes rootDomElement, requireLegacyFiles, and useLegacyTestHarness to LegacyPlatformService', () => {
-    const rootDomElement = { rootDomElement: true } as any;
+  it('passes requireLegacyFiles, useLegacyTestHarness, and a dom element to LegacyPlatformService', () => {
+    const rootDomElement = document.createElement('div');
     const requireLegacyFiles = { requireLegacyFiles: true } as any;
     const useLegacyTestHarness = { useLegacyTestHarness: true } as any;
 
@@ -106,14 +106,14 @@ describe('constructor', () => {
 
     expect(MockLegacyPlatformService).toHaveBeenCalledTimes(1);
     expect(MockLegacyPlatformService).toHaveBeenCalledWith({
-      rootDomElement,
+      targetDomElement: expect.any(HTMLElement),
       requireLegacyFiles,
       useLegacyTestHarness,
     });
   });
 
   it('passes injectedMetadata, rootDomElement, and a stopCoreSystem function to FatalErrorsService', () => {
-    const rootDomElement = { rootDomElement: true } as any;
+    const rootDomElement = document.createElement('div');
     const injectedMetadata = { injectedMetadata: true } as any;
 
     const coreSystem = new CoreSystem({
@@ -152,14 +152,22 @@ describe('#stop', () => {
 });
 
 describe('#start()', () => {
-  function startCore() {
+  function startCore(rootDomElement = defaultCoreSystemParams.rootDomElement) {
     const core = new CoreSystem({
       ...defaultCoreSystemParams,
+      rootDomElement,
     });
 
     core.start();
   }
 
+  it('clears the children of the rootDomElement and appends container for legacyPlatform', () => {
+    const root = document.createElement('div');
+    root.innerHTML = '<p>foo bar</p>';
+    startCore(root);
+    expect(root.innerHTML).toBe('<div></div>');
+  });
+
   it('calls injectedMetadata#start()', () => {
     startCore();
     const [mockInstance] = MockInjectedMetadataService.mock.instances;
@@ -173,14 +181,29 @@ describe('#start()', () => {
     expect(mockInstance.start).toHaveBeenCalledTimes(1);
     expect(mockInstance.start).toHaveBeenCalledWith();
   });
+});
 
-  it('calls legacyPlatform#start()', () => {
-    startCore();
-    const [mockInstance] = MockLegacyPlatformService.mock.instances;
-    expect(mockInstance.start).toHaveBeenCalledTimes(1);
-    expect(mockInstance.start).toHaveBeenCalledWith({
-      injectedMetadata: mockInjectedMetadataStartContract,
-      fatalErrors: mockFatalErrorsStartContract,
+describe('LegacyPlatform targetDomElement', () => {
+  it('only mounts the element when started, before starting the legacyPlatformService', () => {
+    const rootDomElement = document.createElement('div');
+    const core = new CoreSystem({
+      ...defaultCoreSystemParams,
+      rootDomElement,
+    });
+
+    const [legacyPlatform] = MockLegacyPlatformService.mock.instances;
+
+    let targetDomElementParentInStart: HTMLElement;
+    (legacyPlatform as any).start.mockImplementation(() => {
+      targetDomElementParentInStart = targetDomElement.parentElement;
     });
+
+    // targetDomElement should not have a parent element when the LegacyPlatformService is constructed
+    const [[{ targetDomElement }]] = MockLegacyPlatformService.mock.calls;
+    expect(targetDomElement).toHaveProperty('parentElement', null);
+
+    // starting the core system should mount the targetDomElement as a child of the rootDomElement
+    core.start();
+    expect(targetDomElementParentInStart!).toBe(rootDomElement);
   });
 });
diff --git a/src/core/public/core_system.ts b/src/core/public/core_system.ts
@@ -17,13 +17,14 @@
  * under the License.
  */
 
+import './core.css';
 import { FatalErrorsService } from './fatal_errors';
 import { InjectedMetadataParams, InjectedMetadataService } from './injected_metadata';
 import { LegacyPlatformParams, LegacyPlatformService } from './legacy_platform';
 
 interface Params {
+  rootDomElement: HTMLElement;
   injectedMetadata: InjectedMetadataParams['injectedMetadata'];
-  rootDomElement: LegacyPlatformParams['rootDomElement'];
   requireLegacyFiles: LegacyPlatformParams['requireLegacyFiles'];
   useLegacyTestHarness?: LegacyPlatformParams['useLegacyTestHarness'];
 }
@@ -35,13 +36,18 @@ interface Params {
  * platform the CoreSystem will get many more Services.
  */
 export class CoreSystem {
-  private fatalErrors: FatalErrorsService;
-  private injectedMetadata: InjectedMetadataService;
-  private legacyPlatform: LegacyPlatformService;
+  private readonly fatalErrors: FatalErrorsService;
+  private readonly injectedMetadata: InjectedMetadataService;
+  private readonly legacyPlatform: LegacyPlatformService;
+
+  private readonly rootDomElement: HTMLElement;
+  private readonly legacyPlatformTargetDomElement: HTMLDivElement;
 
   constructor(params: Params) {
     const { rootDomElement, injectedMetadata, requireLegacyFiles, useLegacyTestHarness } = params;
 
+    this.rootDomElement = rootDomElement;
+
     this.injectedMetadata = new InjectedMetadataService({
       injectedMetadata,
     });
@@ -54,15 +60,21 @@ export class CoreSystem {
       },
     });
 
+    this.legacyPlatformTargetDomElement = document.createElement('div');
     this.legacyPlatform = new LegacyPlatformService({
-      rootDomElement,
+      targetDomElement: this.legacyPlatformTargetDomElement,
       requireLegacyFiles,
       useLegacyTestHarness,
     });
   }
 
   public start() {
     try {
+      // ensure the rootDomElement is empty
+      this.rootDomElement.textContent = '';
+      this.rootDomElement.classList.add('coreSystemRootDomElement');
+      this.rootDomElement.appendChild(this.legacyPlatformTargetDomElement);
+
       const injectedMetadata = this.injectedMetadata.start();
       const fatalErrors = this.fatalErrors.start();
       this.legacyPlatform.start({ injectedMetadata, fatalErrors });
@@ -73,5 +85,6 @@ export class CoreSystem {
 
   public stop() {
     this.legacyPlatform.stop();
+    this.rootDomElement.textContent = '';
   }
 }
diff --git a/src/core/public/legacy_platform/__snapshots__/legacy_platform_service.test.ts.snap b/src/core/public/legacy_platform/__snapshots__/legacy_platform_service.test.ts.snap
@@ -1,24 +1,19 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
-exports[`#stop() destroys the angular scope and empties the rootDomElement if angular is bootstraped to rootDomElement 1`] = `
+exports[`#stop() destroys the angular scope and empties the targetDomElement if angular is bootstraped to targetDomElement 1`] = `
 <div
   class="ng-scope"
 />
 `;
 
-exports[`#stop() does nothing if angular was not bootstrapped to rootDomElement 1`] = `
+exports[`#stop() does nothing if angular was not bootstrapped to targetDomElement 1`] = `
 <div>
   
       
   <h1>
-    foo
+    this should not be removed
   </h1>
   
-      
-  <h2>
-    bar
-  </h2>
-  
     
 </div>
 `;
diff --git a/src/core/public/legacy_platform/legacy_platform_service.test.ts b/src/core/public/legacy_platform/legacy_platform_service.test.ts
@@ -62,7 +62,7 @@ const injectedMetadataStartContract = {
 };
 
 const defaultParams = {
-  rootDomElement: { someDomElement: true } as any,
+  targetDomElement: document.createElement('div'),
   requireLegacyFiles: jest.fn(() => {
     mockLoadOrder.push('legacy files');
   }),
@@ -109,7 +109,7 @@ describe('#start()', () => {
     });
 
     describe('useLegacyTestHarness = false', () => {
-      it('passes the rootDomElement to ui/chrome', () => {
+      it('passes the targetDomElement to ui/chrome', () => {
         const legacyPlatform = new LegacyPlatformService({
           ...defaultParams,
         });
@@ -121,11 +121,11 @@ describe('#start()', () => {
 
         expect(mockUiTestHarnessBootstrap).not.toHaveBeenCalled();
         expect(mockUiChromeBootstrap).toHaveBeenCalledTimes(1);
-        expect(mockUiChromeBootstrap).toHaveBeenCalledWith(defaultParams.rootDomElement);
+        expect(mockUiChromeBootstrap).toHaveBeenCalledWith(defaultParams.targetDomElement);
       });
     });
     describe('useLegacyTestHarness = true', () => {
-      it('passes the rootDomElement to ui/test_harness', () => {
+      it('passes the targetDomElement to ui/test_harness', () => {
         const legacyPlatform = new LegacyPlatformService({
           ...defaultParams,
           useLegacyTestHarness: true,
@@ -138,7 +138,7 @@ describe('#start()', () => {
 
         expect(mockUiChromeBootstrap).not.toHaveBeenCalled();
         expect(mockUiTestHarnessBootstrap).toHaveBeenCalledTimes(1);
-        expect(mockUiTestHarnessBootstrap).toHaveBeenCalledWith(defaultParams.rootDomElement);
+        expect(mockUiTestHarnessBootstrap).toHaveBeenCalledWith(defaultParams.targetDomElement);
       });
     });
   });
@@ -192,29 +192,28 @@ describe('#start()', () => {
 });
 
 describe('#stop()', () => {
-  it('does nothing if angular was not bootstrapped to rootDomElement', () => {
-    const rootDomElement = document.createElement('div');
-    rootDomElement.innerHTML = `
-      <h1>foo</h1>
-      <h2>bar</h2>
+  it('does nothing if angular was not bootstrapped to targetDomElement', () => {
+    const targetDomElement = document.createElement('div');
+    targetDomElement.innerHTML = `
+      <h1>this should not be removed</h1>
     `;
 
     const legacyPlatform = new LegacyPlatformService({
       ...defaultParams,
-      rootDomElement,
+      targetDomElement,
     });
 
     legacyPlatform.stop();
-    expect(rootDomElement).toMatchSnapshot();
+    expect(targetDomElement).toMatchSnapshot();
   });
 
-  it('destroys the angular scope and empties the rootDomElement if angular is bootstraped to rootDomElement', () => {
-    const rootDomElement = document.createElement('div');
+  it('destroys the angular scope and empties the targetDomElement if angular is bootstraped to targetDomElement', () => {
+    const targetDomElement = document.createElement('div');
     const scopeDestroySpy = jest.fn();
 
     const legacyPlatform = new LegacyPlatformService({
       ...defaultParams,
-      rootDomElement,
+      targetDomElement,
     });
 
     // simulate bootstraping with a module "foo"
@@ -225,15 +224,15 @@ describe('#stop()', () => {
       },
     }));
 
-    rootDomElement.innerHTML = `
+    targetDomElement.innerHTML = `
       <bar></bar>
     `;
 
-    angular.bootstrap(rootDomElement, ['foo']);
+    angular.bootstrap(targetDomElement, ['foo']);
 
     legacyPlatform.stop();
 
-    expect(rootDomElement).toMatchSnapshot();
+    expect(targetDomElement).toMatchSnapshot();
     expect(scopeDestroySpy).toHaveBeenCalledTimes(1);
   });
 });
diff --git a/src/core/public/legacy_platform/legacy_platform_service.ts b/src/core/public/legacy_platform/legacy_platform_service.ts
@@ -27,7 +27,7 @@ interface Deps {
 }
 
 export interface LegacyPlatformParams {
-  rootDomElement: HTMLElement;
+  targetDomElement: HTMLElement;
   requireLegacyFiles: () => void;
   useLegacyTestHarness?: boolean;
 }
@@ -55,11 +55,11 @@ export class LegacyPlatformService {
     // require the files that will tie into the legacy platform
     this.params.requireLegacyFiles();
 
-    bootstrapModule.bootstrap(this.params.rootDomElement);
+    bootstrapModule.bootstrap(this.params.targetDomElement);
   }
 
   public stop() {
-    const angularRoot = angular.element(this.params.rootDomElement);
+    const angularRoot = angular.element(this.params.targetDomElement);
     const injector$ = angularRoot.injector();
 
     // if we haven't gotten to the point of bootstraping
@@ -72,11 +72,11 @@ export class LegacyPlatformService {
     injector$.get('$rootScope').$destroy();
 
     // clear the inner html of the root angular element
-    this.params.rootDomElement.textContent = '';
+    this.params.targetDomElement.textContent = '';
   }
 
   private loadBootstrapModule(): {
-    bootstrap: (rootDomElement: HTMLElement) => void;
+    bootstrap: (targetDomElement: HTMLElement) => void;
   } {
     if (this.params.useLegacyTestHarness) {
       // wrapped in NODE_ENV check so the `ui/test_harness` module

diff --git a/src/core_plugins/tests_bundle/tests_entry_template.js b/src/core_plugins/tests_bundle/tests_entry_template.js
@@ -72,13 +72,18 @@ const legacyMetadata = {
   }
 };
 
+// render the core system in a child of the body as the default children of the body
+// in the browser tests are needed for mocha and other test components to work
+const rootDomElement = document.createElement('div');
+document.body.appendChild(rootDomElement)
+
 new CoreSystem({
   injectedMetadata: {
     version: legacyMetadata.version,
     buildNumber: legacyMetadata.buildNum,
     legacyMetadata
   },
-  rootDomElement: document.body,
+  rootDomElement,
   useLegacyTestHarness: true,
   requireLegacyFiles: () => {
     ${bundle.getRequires().join('\n  ')}

diff --git a/src/ui/public/chrome/directives/kbn_chrome.less b/src/ui/public/chrome/directives/kbn_chrome.less
@@ -2,7 +2,12 @@
 @import (reference) "~ui/styles/mixins";
 @import (reference) "~ui/styles/variables";
 
-body {
+/**
+ * stretch the root element of the Kibana application to set the base-size that
+ * flexed children should keep. Only works when paired with root styles applied
+ * by core service from new platform
+ */
+#kibana-body {
   overflow-x: hidden;
   min-height: 100%;
 }