Remove SavedObjectRegistryProvider from codebase #53455

kertal · 2019-12-18T06:57:49Z

Summary

Follow up of #51562, cleanup SavedObjectRegistryProvider from the codebase. Part of this PR is also moving ui/directives/saved_object_finder to timelion, since it's only used there. `
Note: should be reviewed after merging : #53293

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~- [ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~- [ ] 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~~

For maintainers

~~- [ ] This was checked for breaking API changes and was labeled appropriately~~
~~- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

…-13-np-saved-object-visualize

…-13-np-saved-object-visualize # Conflicts: # src/legacy/ui/public/saved_objects/helpers/apply_es_resp.ts

…-np-remove-SavedObjectRegistryProvider

…-18-np-remove-SavedObjectRegistryProvider

- since it's used at no other place

flash1293 · 2019-12-20T09:15:13Z

@elasticmachine merge upstream

…RegistryProvider

kertal · 2019-12-20T11:04:34Z

jenkins, test this (unrelated error before)

flash1293 · 2019-12-20T11:10:16Z

src/legacy/core_plugins/kibana/public/dashboard/index.ts

  return {
    dashboardConfig: injector.get('dashboardConfig'),
-    savedObjectRegistry,
    savedDashboards: injector.get('savedDashboards'),


It looks like we also don't need to inject saved dashboards anymore because we can create the class inside the app using the helper (might be done in a separate PR)

thx, I've taken care of this, planned to do it in a 2020 PR, but the time is now!
did it the same way with visualize, so the injected variables are just used for savedObjectManagementRegistry. now the first 2020 PR will be cleanup of this 🔨

flash1293

Overall fine, but we should resolve the import issues with visualize/dashboard explained in the comments

flash1293 · 2019-12-20T11:17:08Z

src/legacy/core_plugins/kibana/public/dashboard/legacy_app.js

@@ -33,6 +33,7 @@ import { DashboardListing, EMPTY_FILTER } from './listing/dashboard_listing';
 import { addHelpMenuToAppChrome } from './help_menu/help_menu_util';
 import { registerTimefilterWithGlobalStateFactory } from '../../../../ui/public/timefilter/setup_router';
 import { syncOnMount } from './global_state_sync';
+import { savedObjectLoaderDashboard } from './saved_dashboard/saved_dashboards';


This import isn't allowed because it uses services directly without going through plugin.ts (the linting PR will start flagging this).

Suggestion to do this cleanly:

Create the saved_dashboard class and service in the start phase of the dashboard plugin and

expose it through the contract.

Then have a little wrapper in legacy world registering the class from the shimmed contract into angular for the management view.

This would put the class creation where it belongs, doesn't cross legacy/shimmed boundaries, wouldn't block moving the dashboard app into NP and is still compatible with the angular setup.

Basically it would mean moving this part into the plugin.ts:

const savedObjectsClient = npStart.core.savedObjects.client; const services = { savedObjectsClient, indexPatterns: npStart.plugins.data.indexPatterns, chrome: npStart.core.chrome, overlays: npStart.core.overlays, }; const SavedDashboard = createSavedDashboardClass(services); return { savedObjectLoaderDashboard: new SavedObjectLoader( SavedDashboard, savedObjectsClient, npStart.core.chrome ) };

And keeping just this part in legacy saved_dasboard:

// This is the only thing that gets injected into controllers module.service('savedDashboards', () => dashboardStart.savedObjectLoaderDashboard);

Thx, yes, the upcoming linting improvement will complain about this changes. So I've solved it the "Discover way"™" for now, services created twice, and will clean this by removing module.service('savedDashboards'... in a follow up PR

flash1293 · 2019-12-20T11:21:29Z

src/legacy/core_plugins/kibana/public/discover/saved_searches/saved_searches_register.ts

-  };
-  return createSavedSearchesService(services);
-});
+module.service('savedSearches', () => savedSearches);


We could use the same pattern as suggested above for dashboard here (only creating the service once and exposing through the contract, but that way is also fine because it's just created twice, once for "inside" the app and once for the global registry. That approach would also be OK for me for dashboard.

♩ ♪ ♫ ♬ ♭ ♮ ♯ 🎼 🎵 🎶 I did it discover way ♩ ♪ ♫ ♬ ♭ ♮ ♯ 🎼 🎵 🎶

flash1293 · 2019-12-20T11:22:16Z

src/legacy/core_plugins/kibana/public/visualize/listing/visualize_listing.js

@@ -25,6 +25,7 @@ import { i18n } from '@kbn/i18n';

 import { getServices } from '../kibana_services';
 import { wrapInI18nContext } from '../legacy_imports';
+import { savedObjectLoaderVisualize } from '../saved_visualizations/saved_visualization_register';


Same problem as with dashboard here

…-18-np-remove-SavedObjectRegistryProvider

…der' of github.com:kertal/kibana into kertal-pr-2019-12-18-np-remove-SavedObjectRegistryProvider

flash1293 · 2019-12-20T14:44:49Z

src/legacy/core_plugins/kibana/public/dashboard/index.ts

-  const Private = injector.get<IPrivate>('Private');
-
-  const savedObjectRegistry = Private(SavedObjectRegistryProvider);
-
  return {
    dashboardConfig: injector.get('dashboardConfig'),


🎉Down to one, nice! We should probably just move that flag into kibana_legacy, then dashboard is almost ready to go (not in this PR though)

flash1293

Didn't test again but code changes LGTM once green CI. We should add a dec docs section here explaining the change as 3rd party plugin might use the registry to integrate with visualizations/dashboards.

kibanamachine · 2019-12-21T09:36:16Z

💚 Build Succeeded

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

History

💚 Build #16911 succeeded 6f9e2d5
💚 Build #16881 succeeded e8764c3
💔 Build #16863 failed e8764c3
💚 Build #16634 succeeded 013e5f4
💔 Build #16620 failed 4cf7a3b

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

…le-saved-objects * 'master' of github.com:elastic/kibana: (250 commits) Allow chromeless applications to render via non-/app routes (elastic#51527) Add server rendering service to enable standalone route rendering (elastic#52161) Possibility to filter when testing scripted fields (elastic#35379) (elastic#44220) Update maps telemetry mappings to account for recent updates (elastic#53803) [Maps] Only show legend when layer is visible (elastic#53781) remove use of experimental fs.promises api (elastic#53346) [APM] Add log statements for flaky test (elastic#53775) [APM] Transaction page throws unhandled exception if transactions doesn't have `http.request` (elastic#53760) Licensing plugin functional tests (elastic#53580) [Lens] Disable saving visualization until there are no changes to the document (elastic#52982) [Monitoring] Added safeguard for some EUI components (elastic#53318) [Vega] Shim new platform - cleanup vega_visualization dependencies (elastic#53605) Display changed field formats without requiring hard page refresh. (elastic#53746) Upgrade EUI to v17.3.1 (elastic#53655) [APM] Fix missing apm indicies (elastic#53541) Disable inspector for timelion (elastic#53747) Clean up search servie (elastic#53701) [Dashboard] Grid: removing double handler (elastic#53707) Remove SavedObjectRegistryProvider from codebase (elastic#53455) Move ui/courier into data shim plugin (elastic#52359) ...

kertal and others added 26 commits December 13, 2019 13:13

Initial version

a1c86ec

Further migrations

70d7cf8

merge upstream/master

9b337a8

Remove ES2015 public types

9374470

Code improvements & cleanups

f94e5d3

Fix test

ca96123

Merge remote-tracking branch 'upstream/master' into kertal-pr-2019-12…

ae596fe

…-13-np-saved-object-visualize

Improve types

03b8efb

Merge remote-tracking branch 'upstream/master' into kertal-pr-2019-12…

a568b28

…-13-np-saved-object-visualize # Conflicts: # src/legacy/ui/public/saved_objects/helpers/apply_es_resp.ts

remove global angular references in graph

e2515bf

fix test

59004ff

Remove accidentally commited files

e3b108c

Add missing doc files

c09e7ca

Cleanup Discover

e392307

Cleanup visualize

3372685

Cleanup visualize - remove registry

78d4822

Cleanup visualize - plugin.ts

1ecab13

Cleanup dashboard

9cc80c2

Cleanup dashboard - register

17b3737

merge

e7cd3b8

Merge branch 'flash1293-graph/deangularize' into kertal-pr-2019-12-18…

105748f

…-np-remove-SavedObjectRegistryProvider

Merge remote-tracking branch 'upstream/master' into kertal-pr-2019-12…

c3a8cab

…-18-np-remove-SavedObjectRegistryProvider

Cleanup timelion

2dcbe20

Migrate saved_object_finder to timelion

032900d

- since it's used at no other place

Remove unused SavedObjectRegistryProvider

d8b384e

Fix i18n issues

7987d95

kertal self-assigned this Dec 19, 2019

kertal added 2 commits December 19, 2019 07:02

merge master / fix conflicts

4cf7a3b

Cleanup SavedObjectRegistryProvider from discover (merge leftover)

013e5f4

kertal added the Feature:NP Migration label Dec 19, 2019

kertal changed the title ~~remove saved object registry provider~~ Remove SavedObjectRegistryProvider from codebase Dec 19, 2019

kertal marked this pull request as ready for review December 19, 2019 10:58

kertal requested a review from a team December 19, 2019 10:58

kertal added the release_note:skip Skip the PR/issue when compiling release notes label Dec 19, 2019

kertal requested review from flash1293, lizozom and maryia-lapata December 19, 2019 11:21

Merge branch 'master' into kertal-pr-2019-12-18-np-remove-SavedObject…

e8764c3

…RegistryProvider

flash1293 reviewed Dec 20, 2019

View reviewed changes

flash1293 requested changes Dec 20, 2019

View reviewed changes

kertal added 5 commits December 20, 2019 13:39

Dashboard review changes

a36d340

Visualize review changes

0f12cfe

Merge remote-tracking branch 'upstream/master' into kertal-pr-2019-12…

c539e7a

…-18-np-remove-SavedObjectRegistryProvider

Fix potential linting conflict in application.ts

f21b8d9

Merge branch 'kertal-pr-2019-12-18-np-remove-SavedObjectRegistryProvi…

6f9e2d5

…der' of github.com:kertal/kibana into kertal-pr-2019-12-18-np-remove-SavedObjectRegistryProvider

flash1293 reviewed Dec 20, 2019

View reviewed changes

flash1293 approved these changes Dec 20, 2019

View reviewed changes

Merge master/fix conflicts

a6eade5

elastic deleted a comment from elasticmachine Dec 21, 2019

kertal merged commit b921987 into elastic:master Dec 21, 2019

kertal added a commit to kertal/kibana that referenced this pull request Dec 21, 2019

Remove SavedObjectRegistryProvider from codebase (elastic#53455)

3e77cf9

kertal mentioned this pull request Dec 21, 2019

[7.x] Remove SavedObjectRegistryProvider from codebase (#53455) #53745

Merged

kertal added a commit that referenced this pull request Dec 21, 2019

Remove SavedObjectRegistryProvider from codebase (#53455) (#53745)

4b2a419

jkelastic pushed a commit to jkelastic/kibana that referenced this pull request Jan 8, 2020

Remove SavedObjectRegistryProvider from codebase (elastic#53455)

42f4942

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove SavedObjectRegistryProvider from codebase #53455

Remove SavedObjectRegistryProvider from codebase #53455

kertal commented Dec 18, 2019 •

edited

Loading

flash1293 commented Dec 20, 2019

kertal commented Dec 20, 2019

flash1293 Dec 20, 2019

kertal Dec 20, 2019

flash1293 left a comment

flash1293 Dec 20, 2019 •

edited

Loading

kertal Dec 20, 2019 •

edited

Loading

flash1293 Dec 20, 2019

kertal Dec 20, 2019

flash1293 Dec 20, 2019

flash1293 Dec 20, 2019 •

edited

Loading

flash1293 left a comment

kibanamachine commented Dec 21, 2019

Remove SavedObjectRegistryProvider from codebase #53455

Remove SavedObjectRegistryProvider from codebase #53455

Conversation

kertal commented Dec 18, 2019 • edited Loading

Summary

Checklist

For maintainers

flash1293 commented Dec 20, 2019

kertal commented Dec 20, 2019

flash1293 Dec 20, 2019

Choose a reason for hiding this comment

kertal Dec 20, 2019

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

flash1293 Dec 20, 2019 • edited Loading

Choose a reason for hiding this comment

kertal Dec 20, 2019 • edited Loading

Choose a reason for hiding this comment

flash1293 Dec 20, 2019

Choose a reason for hiding this comment

kertal Dec 20, 2019

Choose a reason for hiding this comment

flash1293 Dec 20, 2019

Choose a reason for hiding this comment

flash1293 Dec 20, 2019 • edited Loading

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

kibanamachine commented Dec 21, 2019

💚 Build Succeeded

History

kertal commented Dec 18, 2019 •

edited

Loading

flash1293 Dec 20, 2019 •

edited

Loading

kertal Dec 20, 2019 •

edited

Loading

flash1293 Dec 20, 2019 •

edited

Loading