[NP][Discover] Move discover into new platform by sulemanof · Pull Request #63473 · elastic/kibana

sulemanof · 2020-04-14T15:30:05Z

Summary

Part of #60097

Most of the changes are file transfer into new platform and related paths changes.

The main changes are:

both plugin initializers src/plugins/discover/public/plugin.ts and src/legacy/core_plugins/kibana/public/discover/plugin.ts were merged into one;
Field constructor in src/plugins/data/public/index_patterns/fields/field.ts was changed to accept dependencies { fieldFormats, toastNotifications } instead of using getter/setter, as well as FieldList in src/plugins/data/public/index_patterns/fields/field_list.ts , since it's statically used in discover and management (which is still in legacy, but would have the same problem while migrated);
translations was changed to use discover prefix;

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

sulemanof · 2020-04-14T15:31:03Z

This is currently blocked by #62624

# Conflicts: # src/legacy/core_plugins/kibana/index.js # src/legacy/core_plugins/kibana/public/discover/plugin.ts # src/legacy/core_plugins/kibana/public/index.scss # src/legacy/core_plugins/kibana/public/kibana.js # src/plugins/discover/public/build_services.ts # src/plugins/discover/public/kibana_services.ts

# Conflicts: # src/plugins/discover/public/application/angular/context/api/_stubs.js # src/plugins/discover/public/application/angular/context/api/context.ts # src/plugins/discover/public/build_services.ts

# Conflicts: # src/legacy/core_plugins/kibana/public/management/sections/index_patterns/edit_index_pattern/edit_index_pattern.js # src/plugins/discover/public/kibana_services.ts

sulemanof · 2020-04-24T18:00:38Z

retest

elasticmachine · 2020-04-27T08:06:42Z

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

kertal · 2020-04-27T08:08:25Z

@elasticmachine merge upstream

.github/CODEOWNERS

src/legacy/core_plugins/kibana/public/__tests__/discover/legacy.ts

src/plugins/data/public/index_patterns/index_patterns/index_patterns.test.ts

pgayvallet · 2020-05-01T08:38:45Z

src/plugins/discover/public/kibana_services.ts

  SortDirection,
-} from '../../../../../plugins/data/public';
+} from '../../data/public';


I think re-exporting from other KP plugins/bundles was causing some problems a few month back. I remember an issue with kibana_react re-exporting things from core. But maybe that was only with concrete types, or maybe only when these re-exports were exposed in the entry point. @spalger is there any issue with that?

Since this code exists for a long, I didn't notice any problems with that.
If the issue you mentioned exists, I think we could create a separate fix for it and let this PR go further. 🙂

I don't think there's anything wrong with this, I recommend this approach in many cases.

pgayvallet · 2020-05-01T08:48:22Z

src/plugins/discover/public/plugin.ts

+        await this.initializeInnerAngular();
+
+        // make sure the index pattern list is up to date
+        await dataStart.indexPatterns.clearCache();
+        const { renderApp } = await import('./application/application');
+        const unmount = await renderApp(innerAngularName, params.element);


I know you are already doing some ping-pong between setup and start for this initializeInnerAngular call, but ideally, initializeInnerAngular would be included in the async import instead just the app (await import('./application/application'))

That would avoid

import { getInnerAngularModuleEmbeddable, getInnerAngularModule } from './get_inner_angular';

to be present in the root plugin file. Seing all the things this get_inner_angular module imports, that would probably drastically reduce the discover plugin initial bundle's size.

I see that getInnerAngularModuleEmbeddable is used outside of the mount function, so this change seems really non-trivial, but that would really be a nice performance improvement.

That could be done in a follow-up PR though.

@pgayvallet This is a really good point!
But if you don't mind, I would do it in a follow up PR in scope of #63596

# Conflicts: # docs/development/plugins/data/public/kibana-plugin-plugins-data-public.search.md # src/plugins/data/public/public.api.md

flash1293

Tested in Chrome and didn't notice any issues. LGTM once green.

kertal

Code LGTM, tested locally in Chrome an found a problem with the single doc view, showing an empty screen:

sulemanof · 2020-05-04T10:23:17Z

Code LGTM, tested locally in Chrome an found a problem with the single doc view, showing an empty screen:

seems to be a problem with usage of an arrow function as a constructor, faced the same while migrating Dashboard to NP.

sulemanof · 2020-05-04T10:50:53Z

found a problem with the single doc view, showing an empty screen:

@kertal thanks for the good catch, fixed in the last commit

ppisljar

code LGTM

lizozom

Looks good overall
Added couple of questions

lizozom · 2020-05-04T10:43:49Z

src/legacy/ui/public/styles/_legacy/components/_table.scss

@@ -1,4 +1,4 @@
-@import '../../../../../core_plugins/kibana/public/discover/np_ready/mixins';
+@import '../../../../../../plugins/discover/public/application/mixins';


You can load this directly form NP IMO

I would wait for a comment from @kibana-design here, because I'm not sure whether this still needed. Maybe could be simply removed.

@elastic/kibana-design

I agree,
not a blocker to merge though

There are some very generic selectors in this file, I'd be very worried about removing it without doing a full search for all the selectors. I'd leave this as-is and we'll do an audit before 8.0

lizozom · 2020-05-04T10:45:59Z

...anagement/sections/index_patterns/edit_index_pattern/create_edit_field/create_edit_field.tsx

 import { EuiFlexGroup, EuiFlexItem } from '@elastic/eui';
 import { i18n } from '@kbn/i18n';
 import { HttpStart, DocLinksStart } from 'src/core/public';
+import { IndexPattern, DataPublicPluginStart } from 'src/plugins/data/public';


Could you use IIndexPattern here?

Of course I could 🙂
But before changing this, I'd like to know details why it is necessary?
AFAIK the usage of I prefixes is no longer necessary since we update typescript to 3.7 (this thing was discussed on kibana app migration meeting, @flash1293 please correct me if I'm wrong).
If it's still relevant, then we should get rid of exporting the IndexPattern from the data start and export the only interface, because IndexPattern continues to be used as interface in many places.

In addition, changing this to IIndexPattern will cause a lot of changes in data plugin itself, since the Field class constructor expecs the IndexPattern type: https://github.com/elastic/kibana/blob/master/src/plugins/data/public/index_patterns/fields/field.ts#L53
the same is about FieldList:
https://github.com/elastic/kibana/blob/master/src/plugins/data/public/index_patterns/fields/field_list.ts#L49
and this chain is quite big :)

It's not urgent.
IIndexPattern is actually a narrower interface than IndexPattern
We will take care of that in another iteration. 👍

lizozom · 2020-05-04T11:01:57Z

src/plugins/discover/public/application/angular/context/api/context.ts

 * under the License.
 */

+import { Filter, IndexPatternsContract, IndexPattern } from 'src/plugins/data/public';


// brain pick
Should we stick with the relative paths for consistency?
The absolute paths still don't work for non type imports.

I'm not aware of any restrictions to use absolute imports for types.
This wouldn't work for static js of course,
but I prefer using absolute one for types only, this is extremely helpful in context of our endless files migration, where you have to change paths in every PR as this one.

lizozom · 2020-05-04T11:05:06Z

src/plugins/discover/public/application/angular/doc_table/lib/get_sort.ts


 import _ from 'lodash';
-import { IndexPattern } from '../../../../../../../../../plugins/data/public';
+import { IndexPattern } from '../../../../../../data/public';


Could you use IINdexPattern?

src/plugins/discover/public/application/components/doc/doc.tsx

lizozom · 2020-05-04T12:37:59Z

src/plugins/discover/public/application/components/sidebar/discover_sidebar.tsx

  const [fields, setFields] = useState<IndexPatternFieldList | null>(null);
  const [fieldFilterState, setFieldFilterState] = useState(getDefaultFieldFilter());
-  const services = getServices();
+  const services = useMemo(() => getServices(), []);


This looks rather weird, because this means the services could change, but we won't get a fresh version?
@kertal

kertal

Code LGTM, doc view now works correctly 👍 . Tested locally with Chrome 81, Mac OS 10.14.6. No regressions. Thx for taking care of the last Kibana App 🦕 !

cchaos

I did a quick scan of the different Discover views and the table vis and I didn't see any errors from the styles side.

cchaos · 2020-05-04T15:41:45Z

src/legacy/ui/public/styles/_legacy/components/_table.scss

@@ -1,4 +1,4 @@
-@import '../../../../../core_plugins/kibana/public/discover/np_ready/mixins';
+@import '../../../../../../plugins/discover/public/application/mixins';


There are some very generic selectors in this file, I'd be very worried about removing it without doing a full search for all the selectors. I'd leave this as-is and we'll do an audit before 8.0

# Conflicts: # src/plugins/data/public/public.api.md

kibanamachine · 2020-05-04T19:21:43Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 0b7ad66

History

💚 Build #44915 succeeded bbd9aca
💚 Build #44872 succeeded da40cf0
💚 Build #44557 succeeded 2ef124c
💔 Build #44464 failed 30253a3
💔 Build #44161 failed f00d980

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* Move discover into NP * Convert doc_table tests to jest * Move rows_headers to use jest * Move fixed_scroll.test * Clean up * Revert jest changes * Pass down deps into IndexPatternFieldList * Fix conflicts * Pass env vars * Remove LegacyCoreStart * Update generated doc * Fix canvas type * Fix i18n * Improve stub_index_pattern code * Add fieldFormats to mocked services * Skip failing tests - while still working on them, to find out if other tests fail in jenkins * Unskip sidebar test * Move mocha tests to legacy - delete jest tests, can be converted later on * Fix Scss imports - Seems functional tests didn't build because of them? * Remove another invalid SCSS import * Pass deps as last argument * Move field list into data start contract * Move create field into data start contract, fix tests * Update docs * Fix duplicating fields * Update snapshots in management * Fix review comments * Update docs * Fix angular compilation * Update docs Co-authored-by: Matthias Wilhelm <matthias.wilhelm@elastic.co> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> # Conflicts: # .github/CODEOWNERS # x-pack/plugins/translations/translations/ja-JP.json # x-pack/plugins/translations/translations/zh-CN.json

* [NP][Discover] Move discover into new platform (#63473) * Move discover into NP * Convert doc_table tests to jest * Move rows_headers to use jest * Move fixed_scroll.test * Clean up * Revert jest changes * Pass down deps into IndexPatternFieldList * Fix conflicts * Pass env vars * Remove LegacyCoreStart * Update generated doc * Fix canvas type * Fix i18n * Improve stub_index_pattern code * Add fieldFormats to mocked services * Skip failing tests - while still working on them, to find out if other tests fail in jenkins * Unskip sidebar test * Move mocha tests to legacy - delete jest tests, can be converted later on * Fix Scss imports - Seems functional tests didn't build because of them? * Remove another invalid SCSS import * Pass deps as last argument * Move field list into data start contract * Move create field into data start contract, fix tests * Update docs * Fix duplicating fields * Update snapshots in management * Fix review comments * Update docs * Fix angular compilation * Update docs Co-authored-by: Matthias Wilhelm <matthias.wilhelm@elastic.co> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> # Conflicts: # .github/CODEOWNERS # x-pack/plugins/translations/translations/ja-JP.json # x-pack/plugins/translations/translations/zh-CN.json * Fix intl conflicts

Move discover into NP

dde7972

flash1293 mentioned this pull request Apr 14, 2020

Meta: Kibana app NP cutover #60097

Closed

69 tasks

sulemanof added 7 commits April 15, 2020 17:09

Convert doc_table tests to jest

d2c1ee6

Move rows_headers to use jest

420b22b

Move fixed_scroll.test

7e837cf

Merge remote-tracking branch 'upstream/master' into np/discover

cdd25d2

# Conflicts: # src/plugins/discover/public/application/angular/context/api/_stubs.js # src/plugins/discover/public/application/angular/context/api/context.ts # src/plugins/discover/public/build_services.ts

Clean up

d37f000

Revert jest changes

62ced26

sulemanof mentioned this pull request Apr 23, 2020

[Data] Statically exported IndexPatternFieldList uses runtime deps #64322

Closed

sulemanof added 5 commits April 24, 2020 14:54

Merge remote-tracking branch 'upstream/master' into np/discover

cf58cc1

Pass down deps into IndexPatternFieldList

7b54319

Merge remote-tracking branch 'upstream/master' into np/discover

afacf40

# Conflicts: # src/legacy/core_plugins/kibana/public/management/sections/index_patterns/edit_index_pattern/edit_index_pattern.js # src/plugins/discover/public/kibana_services.ts

Fix conflicts

ca5f1b3

Pass env vars

096eed9

Remove LegacyCoreStart

ab99f53

kertal assigned kertal and sulemanof Apr 27, 2020

kertal mentioned this pull request Apr 27, 2020

Discover NP #64491

Closed

7 tasks

kertal added Feature:NP Migration Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// labels Apr 27, 2020

kertal added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Apr 27, 2020

Merge branch 'master' into np/discover

337c9b5

flash1293 added the v7.8.0 label Apr 27, 2020

Update generated doc

a3080f2

flash1293 mentioned this pull request May 1, 2020

Switch to core application service #63443

Merged

pgayvallet approved these changes May 1, 2020

View reviewed changes

alexwizp approved these changes May 4, 2020

View reviewed changes

sulemanof added 3 commits May 4, 2020 11:15

Fix review comments

2d85cbe

Merge remote-tracking branch 'upstream/master' into np/discover

733ea56

# Conflicts: # docs/development/plugins/data/public/kibana-plugin-plugins-data-public.search.md # src/plugins/data/public/public.api.md

Update docs

da40cf0

flash1293 approved these changes May 4, 2020

View reviewed changes

kertal requested review from kertal and ppisljar May 4, 2020 10:10

kertal requested changes May 4, 2020

View reviewed changes

Fix angular compilation

bbd9aca

ppisljar approved these changes May 4, 2020

View reviewed changes

lizozom reviewed May 4, 2020

View reviewed changes

kertal self-requested a review May 4, 2020 13:44

kertal approved these changes May 4, 2020

View reviewed changes

cchaos approved these changes May 4, 2020

View reviewed changes

sulemanof added 2 commits May 4, 2020 20:13

Merge remote-tracking branch 'upstream/master' into np/discover

39fc4a7

# Conflicts: # src/plugins/data/public/public.api.md

Update docs

0b7ad66

sulemanof merged commit f126e61 into elastic:master May 4, 2020

sulemanof deleted the np/discover branch May 4, 2020 20:34

sulemanof mentioned this pull request May 4, 2020

[7.x] [NP][Discover] Move discover into new platform (#63473) #65177

Merged

cchaos mentioned this pull request May 5, 2020

[Doc-Table] Import styles into JS where possible #65329

Merged

1 task

sulemanof added v7.9.0 and removed v7.8.0 labels May 6, 2020

sulemanof added the backported label May 6, 2020

		@@ -1,4 +1,4 @@
		@import '../../../../../core_plugins/kibana/public/discover/np_ready/mixins';
		@import '../../../../../../plugins/discover/public/application/mixins';

Conversation

sulemanof commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

sulemanof commented Apr 14, 2020

Uh oh!

sulemanof commented Apr 24, 2020

Uh oh!

elasticmachine commented Apr 27, 2020

Uh oh!

kertal commented Apr 27, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pgayvallet May 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet May 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

kertal left a comment

Choose a reason for hiding this comment

Uh oh!

sulemanof commented May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sulemanof commented May 4, 2020

Uh oh!

ppisljar left a comment

Choose a reason for hiding this comment

Uh oh!

lizozom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cchaos May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kertal left a comment

Choose a reason for hiding this comment

Uh oh!

cchaos left a comment

sulemanof commented Apr 14, 2020 •

edited

Loading

pgayvallet May 1, 2020 •

edited

Loading

pgayvallet May 1, 2020 •

edited

Loading

sulemanof commented May 4, 2020 •

edited

Loading

cchaos May 4, 2020 •

edited

Loading

cchaos May 4, 2020 •

edited

Loading