[Graph] Shim plugin #47469

flash1293 · 2019-10-07T14:29:11Z

This PR shims the Graph app plugin while still using angular under the hood by pulling in angular dependencies from the global angular and initializing some services itself again in the app-local angular.

kertal

No review so far, just a note, that I currently can't log in with this PR. In Chrome there's an endless redirect. In Safari if've got the following in my logs:
server log [15:41:02.010] [warning][server][Kibana][cookie-session-storage][http] Found 2 auth sessions when we were only expecting 1.

flash1293 · 2019-10-16T08:43:56Z

@kertal My bad, the Graph app was loaded twice due to an experiment I failed to clean up.

kertal · 2019-10-16T09:59:54Z

@flash1293 works now 👍

flash1293 · 2019-10-16T13:31:45Z

Jenkins, test this.

elasticmachine · 2019-10-16T14:57:37Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 21d173c

elasticmachine · 2019-10-17T13:39:03Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 9276ba5

elasticmachine · 2019-10-18T11:57:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 4776eb1

flash1293 · 2019-10-18T12:08:48Z

src/dev/i18n/extractors/code.js

@@ -67,7 +67,7 @@ export function* extractCodeMessages(buffer, reporter) {
  try {
    ast = parse(buffer.toString(), {
      sourceType: 'module',
-      plugins: ['jsx', 'typescript', 'objectRestSpread', 'classProperties', 'asyncGenerators'],
+      plugins: ['jsx', 'typescript', 'objectRestSpread', 'classProperties', 'asyncGenerators', 'dynamicImport'],


This is necessary to support dynamic imports via await import(...

flash1293 · 2019-10-18T12:15:03Z

src/legacy/ui/public/kbn_top_nav/kbn_top_nav.js

@@ -24,7 +24,7 @@ import { TopNavMenu } from '../../../core_plugins/kibana_react/public';

 const module = uiModules.get('kibana');

-module.directive('kbnTopNav', () => {
+export function createTopNavDirective() {


exporting the directive definition as a module export to be able to put it in a separate angular module with a different name. This is done to use existing code like this one but make sure it's not using any dependencies from the global angular instance

flash1293 · 2019-10-18T12:15:48Z

src/legacy/ui/public/legacy_compat/angular_config.tsx

 ) => {
  const urlOverflow = new UrlOverflowService();
  const check = () => {
    // disable long url checks when storing state in session storage
-    if (config.get('state:storeInSessionStorage')) {
+    if (newPlatform.uiSettings.get('state:storeInSessionStorage')) {


don't use config in here to avoid having to initialize it.

flash1293 · 2019-10-18T12:16:38Z

src/legacy/ui/public/private/private.js

-    // one cache/swaps per Provider
-    const cache = {};
-    const swaps = {};
+export function PrivateProvider() {


Same as for the top nav directive. Private isn't used in this PR but it will become necessary for shimming discover and others in the same style

elasticmachine · 2019-10-18T12:18:45Z

Pinging @elastic/kibana-app (Team:KibanaApp)

elasticmachine · 2019-10-21T13:32:38Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c816050

joshdover · 2019-10-21T22:17:09Z

ack: will review tomorrow

mistic

The changes within respect to the operations team LGTM

kertal

Code LGTM, tested locally in Chrome! I salute 🥁 your angular mastership!

kertal · 2019-10-22T09:27:55Z

x-pack/legacy/plugins/graph/public/plugin.ts

+    core: CoreStart,
+    { data, npData, __LEGACY: { angularDependencies } }: GraphPluginStartDependencies
+  ) {
+    // TODO is this really the right way? I though the app context would give us those


I don't know if this is the right way, but I'm sure @joshdover knows

Unfortunately, the app mount context in the Legacy compatibility layer can't support plugins, only core services at this time. We could solve this by implementing the bindServices proposal I've added in #46483 (comment) is still being debated, but we're making a decision on this by next week).

As discussion is still ongoing, I would like to go forward with this solution - it will definitely work and when we decide on another convention we can easily adjust it here.

kertal · 2019-10-22T09:30:43Z

x-pack/legacy/plugins/graph/public/render_app.ts

+  return $injector;
+}
+
+function createLocalAngularModule(core: AppMountContext['core']) {


Nit: since core is not used, it can be removed, also you could directly return the modules without graphAngularModule

joshdover

This is looking fantastic 👍

joshdover · 2019-10-23T15:12:45Z

x-pack/legacy/plugins/graph/public/plugin.ts

+    core: CoreStart,
+    { data, npData, __LEGACY: { angularDependencies } }: GraphPluginStartDependencies
+  ) {
+    // TODO is this really the right way? I though the app context would give us those


Unfortunately, the app mount context in the Legacy compatibility layer can't support plugins, only core services at this time. We could solve this by implementing the bindServices proposal I've added in #46483 (comment) is still being debated, but we're making a decision on this by next week).

joshdover · 2019-10-23T15:15:31Z

x-pack/legacy/plugins/graph/public/render_app.ts

+  indexPatterns: DataStart['indexPatterns']['indexPatterns'];
+  npData: ReturnType<DataPlugin['start']>;
+  savedObjectsClient: SavedObjectsClientContract;
+  xpackInfo: { get(path: string): unknown };


I believe this can be replaced by #44922 once it is complete (just an FYI)

joshdover · 2019-10-23T15:16:26Z

x-pack/legacy/plugins/graph/public/plugin.ts

+    this.dataStart = data;
+    this.npDataStart = npData;
+    this.angularDependencies = angularDependencies;
+    this.savedObjectsClient = core.savedObjects.client;


Should be able to get this from contextCore in the mount function above?

At least not in the types - the AppMountContext['core'] seems to be a subset of CoreStart: src/core/public/application/types.ts line 100, and savedObjects is not there.

joshdover · 2019-10-23T15:20:15Z

x-pack/legacy/plugins/graph/public/render_app.ts

+
+export const renderApp = ({ appBasePath, element, ...deps }: GraphDependencies) => {
+  const graphAngularModule = createLocalAngularModule(deps.coreStart);
+  configureAppAngularModule(graphAngularModule);


Really happy this turned out useful...spent a lot of time breaking this out only to wonder if we would use it.

That said, we need to spend some time removing some of the legacy dependencies that this module uses. Doesn't look like there's too much:

import { fatalError } from 'ui/notify'; import { capabilities } from 'ui/capabilities'; import { modifyUrl } from 'ui/url'; import { UrlOverflowService } from '../error_url_overflow'; import { toastNotifications } from '../notify'; import { isSystemApiRequest } from '../system_api';

We should also make this function take CoreStart as an argument rather than importing it via ui/new_platform.

I have to check but it might be sufficient to just pull in the necessary parts - it looks like Graph only really needs the badges handling and even that could probably be moved to the controller. It's probably not so simple for the Kibana app apps though, so still worth thinking about this.

I worked on this in the dashboard shim PR - are you fine with leaving this as it is for now and we'll fix it there?

elasticmachine · 2019-10-24T14:46:30Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: d3c5a3e

flash1293 · 2019-10-24T14:52:53Z

Jenkins, test this. Failure looks unrelated.

elasticmachine · 2019-10-24T16:10:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: d3c5a3e

flash1293 added 6 commits October 2, 2019 14:23

wip

7ba504f

Merge remote-tracking branch 'upstream/master' into graph/shim-plugin

eea0131

wip

9f1c141

wip

cf06de8

wip

d06721a

hook up everything

0b2c0c0

flash1293 added Feature:Graph Graph application feature v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Oct 7, 2019

flash1293 added 3 commits October 14, 2019 15:08

change indentation to reduce merge conflicts

adc67ce

Merge remote-tracking branch 'upstream/master' into graph/shim-plugin

ec9fbf1

finalize shim

b058df6

kertal reviewed Oct 15, 2019

View reviewed changes

flash1293 added 2 commits October 16, 2019 09:14

Merge remote-tracking branch 'upstream/master' into graph/shim-plugin

1946b6b

fix shim

21d173c

flash1293 changed the title ~~[Graph] [skip ci] Shim plugin~~ [Graph] Shim plugin Oct 16, 2019

flash1293 added 2 commits October 17, 2019 13:59

Merge remote-tracking branch 'upstream/master' into graph/shim-plugin

b130290

get rid of ui modules and fix build

9276ba5

flash1293 added 3 commits October 18, 2019 11:02

Merge remote-tracking branch 'upstream/master' into graph/shim-plugin

c6f5e7b

formalize shim

4eee3cd

simplify graph shim

4776eb1

flash1293 added the v7.6.0 label Oct 18, 2019

flash1293 commented Oct 18, 2019

View reviewed changes

flash1293 marked this pull request as ready for review October 18, 2019 12:17

flash1293 requested a review from a team October 18, 2019 12:17

flash1293 requested a review from a team as a code owner October 18, 2019 12:17

flash1293 requested review from joshdover and Bamieh October 18, 2019 12:18

flash1293 added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Oct 18, 2019

flash1293 added 2 commits October 21, 2019 12:42

Merge remote-tracking branch 'upstream/master' into graph/shim-plugin

976d51f

simplify shim and pull confirmModal in

c816050

mistic approved these changes Oct 22, 2019

View reviewed changes

kertal self-requested a review October 22, 2019 08:49

kertal approved these changes Oct 22, 2019

View reviewed changes

joshdover added the Feature:NP Migration label Oct 22, 2019

joshdover reviewed Oct 23, 2019

View reviewed changes

flash1293 mentioned this pull request Oct 24, 2019

[discuss] Provide a single context service for different handlers #46483

Closed

Merge remote-tracking branch 'upstream/master' into graph/shim-plugin

d3c5a3e

flash1293 merged commit 82d5447 into elastic:master Oct 25, 2019

flash1293 added a commit to flash1293/kibana that referenced this pull request Oct 25, 2019

[Graph] Shim plugin (elastic#47469)

a720d6c

flash1293 mentioned this pull request Oct 25, 2019

[7.x] [Graph] Shim plugin (#47469) #49295

Merged

flash1293 added a commit that referenced this pull request Oct 25, 2019

[Graph] Shim plugin (#47469) (#49295)

5d2858e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Graph] Shim plugin #47469

[Graph] Shim plugin #47469

flash1293 commented Oct 7, 2019

kertal left a comment •

edited

Loading

flash1293 commented Oct 16, 2019

kertal commented Oct 16, 2019

flash1293 commented Oct 16, 2019

elasticmachine commented Oct 16, 2019

elasticmachine commented Oct 17, 2019

elasticmachine commented Oct 18, 2019

flash1293 Oct 18, 2019

flash1293 Oct 18, 2019

flash1293 Oct 18, 2019

flash1293 Oct 18, 2019

elasticmachine commented Oct 18, 2019

elasticmachine commented Oct 21, 2019

joshdover commented Oct 21, 2019

mistic left a comment

kertal left a comment

kertal Oct 22, 2019

joshdover Oct 23, 2019

flash1293 Oct 24, 2019

kertal Oct 22, 2019

joshdover left a comment

joshdover Oct 23, 2019

joshdover Oct 23, 2019

joshdover Oct 23, 2019

flash1293 Oct 24, 2019

joshdover Oct 23, 2019

flash1293 Oct 23, 2019

flash1293 Oct 24, 2019

elasticmachine commented Oct 24, 2019

flash1293 commented Oct 24, 2019

elasticmachine commented Oct 24, 2019

[Graph] Shim plugin #47469

[Graph] Shim plugin #47469

Conversation

flash1293 commented Oct 7, 2019

kertal left a comment • edited Loading

Choose a reason for hiding this comment

flash1293 commented Oct 16, 2019

kertal commented Oct 16, 2019

flash1293 commented Oct 16, 2019

elasticmachine commented Oct 16, 2019

💔 Build Failed

elasticmachine commented Oct 17, 2019

💔 Build Failed

elasticmachine commented Oct 18, 2019

💚 Build Succeeded

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

elasticmachine commented Oct 18, 2019

elasticmachine commented Oct 21, 2019

💚 Build Succeeded

joshdover commented Oct 21, 2019

mistic left a comment

Choose a reason for hiding this comment

kertal 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

joshdover 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

elasticmachine commented Oct 24, 2019

💔 Build Failed

flash1293 commented Oct 24, 2019

elasticmachine commented Oct 24, 2019

💚 Build Succeeded

kertal left a comment •

edited

Loading