Combine visualizations and visualize plugins #121550

DianaDerevyankina · 2021-12-17T16:35:34Z

Closes #113737

Summary

Merged `visualize` into `visualizations` plugin.

Removed dashboard.dashboardFeatureFlagConfig.allowByValueEmbeddables flag to avoid circular dependencies between visualizations and dashboard;
Code from visualize public plugin was added to visualizations public plugin with minor services changes:
- dashboard was removed because of circular dependency;
- visualizations was removed as all required code was used directly from visualizations;
- visEditorsRegistry was added to get rid of its getter and setter and pass it through as other services.
Following code from visualizations previously was used only in visualize services, so it was imported directly and removed from VisualizationsStart
- createVis -> imported createVisAsync from vis_async
- convertToSerializedVis -> imported from saved_visualize_utils
- convertFromSerializedVis -> imported from saved_visualize_utils
- getSavedVisualization -> imported from saved_visualize_utils
- saveVisualization -> imported from saved_visualize_utils
- findListItems -> imported from saved_visualize_utils
- __LEGACY.createVisEmbeddableFromObject -> imported from create_vis_embeddable_from_object
Code from visualize server plugin was added to visualizations server plugin without any changes;
Changes related to kibana.json:
- dependencies from visualize kibana.json were moved to visualizations kibana.json;
- In vis_default_editor kibana.json visualize optional plugin was replaced with visualizations;
- In timeseries kibana.json visualize was removed as it already has visualizations dependency;
Updated tests and translations keys.

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

flash1293 · 2021-12-17T16:57:19Z

...scover/public/application/main/components/sidebar/discover_index_pattern_management.test.tsx

@@ -20,7 +20,7 @@ const mockServices = {
    },
  }),
  capabilities: {
-    visualize: {


AFAIK capabilities are not coupled to the plugin id - I would like to keep it as visualize if possible, I think this will cause a bunch of downstream problems.

alexwizp · 2021-12-29T18:59:27Z

@elasticmachine merge upstream

alexwizp · 2021-12-29T19:01:46Z

@elasticmachine merge upstream

flash1293 · 2021-12-30T08:12:32Z

src/plugins/kibana_usage_collection/server/collectors/application_usage/schema.ts

@@ -124,7 +124,7 @@ export const applicationUsageSchema = {
  kibana: commonSchema, // It's a forward app so we'll likely never report it
  management: commonSchema,
  short_url_redirect: commonSchema, // It's a forward app so we'll likely never report it
-  visualize: commonSchema,


Let’s not change telemetry keys - the application we are tracking here is still “visualize”, everything else will cause problems

flash1293 · 2021-12-30T08:14:51Z

x-pack/plugins/apm/scripts/create_apm_users_and_roles/roles/read_only_user_role.ts

@@ -51,7 +51,7 @@ export const readOnlyUserRole: RoleType = {
        ml: ['read'],
        maps: ['read'],
        graph: ['read'],
-        visualize: ['read'],
+        visualizations: ['read'],


References the permission which is still “visualize”

flash1293 · 2021-12-30T08:15:13Z

x-pack/plugins/spaces/server/usage_collection/spaces_usage_collector.ts

@@ -140,7 +140,7 @@ export interface UsageData extends UsageStats {
    dev_tools?: number;
    advancedSettings?: number;
    infrastructure?: number;
-    visualize?: number;
+    visualizations?: number;


References the permission which is still “visualize “

flash1293 · 2021-12-30T08:15:26Z

x-pack/plugins/spaces/server/usage_collection/spaces_usage_collector.ts

@@ -285,7 +285,7 @@ export function getSpacesUsageCollector(
            description: 'The number of spaces which have this feature disabled.',
          },
        },
-        visualize: {
+        visualizations: {


flash1293 · 2021-12-30T08:15:33Z

x-pack/plugins/telemetry_collection_xpack/schema/xpack_plugins.json

@@ -7655,7 +7655,7 @@
                "description": "The number of spaces which have this feature disabled."
              }
            },
-            "visualize": {
+            "visualizations": {


alexwizp · 2022-01-03T11:09:53Z

src/plugins/visualize/server/plugin.ts

-  public setup(core: CoreSetup) {
-    this.logger.debug('visualize: Setup');
-
-    core.capabilities.registerProvider(capabilitiesProvider);


Are you sure we don't need to register visualize capabilities in visualizations plugin?

alexwizp · 2022-01-03T11:38:57Z

src/plugins/visualizations/public/services.ts

@@ -66,3 +67,6 @@ export const [getOverlays, setOverlays] = createGetterSetter<OverlayStart>('Over
 export const [getChrome, setChrome] = createGetterSetter<ChromeStart>('Chrome');

 export const [getSpaces, setSpaces] = createGetterSetter<SpacesPluginStart>('Spaces', false);
+
+export const [getVisEditorsRegistry, setVisEditorsRegistry] =


Not sure that we need to add one more getterSetter for that case. I've checked all places where we use it and looks like you can just add this service into

src/plugins/visualizations/public/plugin.ts const services: VisualizeServices = {

Could you please check that?

…y getter and setter with service

elasticmachine · 2022-01-04T13:37:59Z

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

ThomThomson

Super small Presentation team changes LGTM!

It'll be a lot simpler and less confusing to have these two plugins combined into one!

stratoula

Diana well done, it seems to work fine.
One proposal I have for easier navigation into the plugin is to rename the visualizations-->application folder to `visualizations-->visualize_app' or a name that describes what this folder contains (actually the listing code and the visualize editor wrapper)

stratoula · 2022-01-05T07:18:21Z

src/plugins/vis_default_editor/tsconfig.json

@@ -12,7 +12,8 @@
  "references": [
    { "path": "../../core/tsconfig.json" },
    { "path": "../data/tsconfig.json" },
-    { "path": "../visualize/tsconfig.json" },
+    { "path": "../visualizations/tsconfig.json" },
+    { "path": "../discover/tsconfig.json" },


Why have we added the discover dependency here?

I think in case of adding visualizations/tsconfig.json it should still work without /discover/tsconfig.json
but for that case I prefer to keep discover cause it's in requiredBundles.

stratoula · 2022-01-05T07:19:32Z

src/plugins/vis_types/timeseries/tsconfig.json

@@ -18,7 +18,7 @@
    { "path": "../../data/tsconfig.json" },
    { "path": "../../expressions/tsconfig.json" },
    { "path": "../../visualizations/tsconfig.json" },
-    { "path": "../../visualize/tsconfig.json" },
+    { "path": "../../dashboard/tsconfig.json" },


Why are we adding the dashboard dependency here?

In src/plugins/vis_types/timeseries/server/usage_collector/get_usage_collector.ts we have an import from dashboard. Dashboard plugin should be added to rootDir

stratoula · 2022-01-05T07:29:39Z

src/plugins/visualizations/common/constants.ts

+export const STATE_STORAGE_KEY = '_a';
+export const GLOBAL_STATE_STORAGE_KEY = '_g';
+
+export const APP_NAME = 'visualize';


I would rename this constant to VISUALIZE_APP_NAME as the current app is called visualizations and it can be confusing.

stratoula · 2022-01-05T07:57:57Z

src/plugins/visualizations/public/types.ts

@@ -93,3 +113,30 @@ export interface VisEditorOptionsProps<VisParamType = unknown> {
  setValidity(isValid: boolean): void;
  setTouched(isTouched: boolean): void;
 }
+
+export interface VisualizationServices extends CoreStart {


Is it used anywhere? We already have the VisualizeServices on the application/types so it seems unnecessary. I would remove this type and move the VisualizeServices to this file and export it from here as it is not used only from the application folder but it is used in many places of this plugin

it's actually unnecessary, thanks for noticing!

…applicaion folder to visualize_app and APP_NAME to VISUALIZE_APP_NAME, revert types.ts

flash1293

Looking mostly good to me, just two small things

src/plugins/visualizations/public/visualize_app/utils/get_top_nav_config.tsx

flash1293 · 2022-01-10T17:49:11Z

src/plugins/visualizations/public/visualize_app/utils/get_top_nav_config.tsx

@@ -372,18 +378,21 @@ export const getTopNavConfig = (
      ? [
          {
            id: 'save',
-            iconType: showSaveAndReturn ? undefined : 'save',
+            iconType: originatingApp ? undefined : 'save',


Same here - old showSaveAndReturn included the check of the savedvis id which is omitted here

flash1293 · 2022-01-10T17:49:16Z

src/plugins/visualizations/public/visualize_app/utils/get_top_nav_config.tsx

-            description: i18n.translate('visualize.topNavMenu.saveVisualizationButtonAriaLabel', {
-              defaultMessage: 'Save Visualization',
-            }),
+            emphasize: !originatingApp,


flash1293 · 2022-01-10T17:53:34Z

src/plugins/vis_default_editor/kibana.json

@@ -2,7 +2,7 @@
  "id": "visDefaultEditor",
  "version": "kibana",
  "ui": true,
-  "optionalPlugins": ["visualize"],
+  "optionalPlugins": ["visualizations"],
  "requiredBundles": ["kibanaUtils", "kibanaReact", "data", "fieldFormats", "discover", "esUiShared"],
  "owner": {


Can we add the description from the other kibana.json here as well?

flash1293

Ah, nvm, I get it now - just checking for originatingApp is right. No objections, lgtm, thanks!

stratoula

Thanx for applying my suggestions Diana! LGTM!

# Conflicts: # .i18nrc.json

kibana-ci · 2022-01-12T13:41:05Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`visualizations`	89	141	+52
`visualize`	62	-	-62
total			-10

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`visualizations`	288	283	-5
`visualize`	23	-	-23
total			-28

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`visualizations`	59.4KB	111.6KB	+52.2KB
`visualize`	51.5KB	-	-51.5KB
total			+692.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`visualizations`	16	15	-1
`visualize`	1	-	-1
total			-2

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`visDefaultEditor`	19.4KB	19.4KB	+5.0B
`visTypeTimeseries`	14.7KB	14.7KB	-12.0B
`visualizations`	37.9KB	48.7KB	+10.8KB
`visualize`	15.7KB	-	-15.7KB
total			-4.9KB

Unknown metric groups

API count

id	before	after	diff
`visualizations`	307	303	-4
`visualize`	24	-	-24
total			-28

async chunk count

id	before	after	diff
`visualizations`	7	8	+1
`visualize`	1	-	-1
total			-0

ESLint disabled line counts

id	before	after	diff
`visualizations`	13	16	+3
`visualize`	3	-	-3
total			-0

References to deprecated APIs

id	before	after	diff
`visualizations`	86	127	+41
`visualize`	41	-	-41
total			-0

Total ESLint disabled count

id	before	after	diff
`visualizations`	15	18	+3
`visualize`	3	-	-3
total			-0

History

💔 Build #16932 failed 37e4d71
💚 Build #15896 succeeded 88f25b8
💔 Build #15884 failed 33d3443
💚 Build #15643 succeeded 60bda6d
💚 Build #15629 succeeded c199925
💔 Build #15557 failed 0f31cdb

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

cc @dziyanadzeraviankina

[WIP] Combine visualizations and visualize plugins

0309a8c

flash1293 reviewed Dec 17, 2021

View reviewed changes

DianaDerevyankina added 8 commits December 23, 2021 11:29

Merge branch 'main' into issues/113737

8204d60

Revert some changes and do some refactoring

1c5d5ed

Refactor some code

4e4e91d

Fix some tests

9f7f2ca

Fix functional tests and some jest test cases

940d622

Update telemetry

9daa15b

Fix get_visualization_instance.test and plugin-list.asciidoc

2c602c4

Refactor some code

d69fe69

Merge branch 'main' into issues/113737

611d63e

kibanamachine and others added 3 commits December 29, 2021 14:01

Merge branch 'main' into issues/113737

3bfad19

fix CI

434989d

Add visualizations to vis_default_editor tsconfig

ec538b3

DianaDerevyankina self-assigned this Dec 30, 2021

flash1293 reviewed Dec 30, 2021

View reviewed changes

DianaDerevyankina added backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.1.0 labels Dec 30, 2021

flash1293 reviewed Dec 30, 2021

View reviewed changes

DianaDerevyankina added 4 commits December 30, 2021 11:21

Revert changes related to telemetry and permissions

cf8e8c6

Add dashboard to timeseries tsconfig.json

7a87489

Update limits file

e0d7ca0

Update translation keys

0af27e2

alexwizp reviewed Jan 3, 2022

View reviewed changes

DianaDerevyankina added 4 commits January 3, 2022 18:29

Add capabilitiesProvider back to server and replace visEditorsRegistr…

543173c

…y getter and setter with service

Update mocks.ts

0f31cdb

Revert changes related to visEditorsRegistry

c199925

Get rid of visEditorsRegistry getter and setter

60bda6d

alexwizp marked this pull request as ready for review January 4, 2022 13:37

alexwizp requested review from a team as code owners January 4, 2022 13:37

ThomThomson approved these changes Jan 4, 2022

View reviewed changes

jbudz approved these changes Jan 4, 2022

View reviewed changes

ryankeairns approved these changes Jan 4, 2022

View reviewed changes

stratoula reviewed Jan 5, 2022

View reviewed changes

DianaDerevyankina added 3 commits January 5, 2022 15:53

Merge branch 'main' into issues/113737

d4fcb65

Remove dashboard from timeseries/tsconfig.json

33d3443

Return back dashboard dependency to timeseries/tsconfig.json, rename …

88f25b8

…applicaion folder to visualize_app and APP_NAME to VISUALIZE_APP_NAME, revert types.ts

alexwizp approved these changes Jan 5, 2022

View reviewed changes

flash1293 reviewed Jan 10, 2022

View reviewed changes

flash1293 approved these changes Jan 11, 2022

View reviewed changes

stratoula approved these changes Jan 12, 2022

View reviewed changes

DianaDerevyankina added 2 commits January 12, 2022 14:51

Merge branch 'main' into issues/113737

37e4d71

# Conflicts: # .i18nrc.json

Remove comma in .i18nrc.json

ebd3fd0

DianaDerevyankina merged commit a9ec1be into elastic:main Jan 12, 2022

yenienserrano mentioned this pull request Nov 8, 2022

Compatibility with Elastic 8 wazuh/wazuh-dashboard-plugins#4567

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Combine visualizations and visualize plugins #121550

Combine visualizations and visualize plugins #121550

DianaDerevyankina commented Dec 17, 2021 •

edited

Loading

flash1293 Dec 17, 2021

alexwizp commented Dec 29, 2021

alexwizp commented Dec 29, 2021

flash1293 Dec 30, 2021

flash1293 Dec 30, 2021

flash1293 Dec 30, 2021

flash1293 Dec 30, 2021

flash1293 Dec 30, 2021

alexwizp Jan 3, 2022

alexwizp Jan 3, 2022

elasticmachine commented Jan 4, 2022

ThomThomson left a comment

stratoula left a comment

stratoula Jan 5, 2022

alexwizp Jan 5, 2022

stratoula Jan 5, 2022

DianaDerevyankina Jan 5, 2022

stratoula Jan 5, 2022

stratoula Jan 5, 2022

DianaDerevyankina Jan 5, 2022

flash1293 left a comment

flash1293 Jan 10, 2022

flash1293 Jan 10, 2022

flash1293 Jan 10, 2022

flash1293 left a comment

stratoula left a comment

kibana-ci commented Jan 12, 2022

API count

async chunk count

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

Combine visualizations and visualize plugins #121550

Combine visualizations and visualize plugins #121550

Conversation

DianaDerevyankina commented Dec 17, 2021 • edited Loading

Summary

Merged visualize into visualizations plugin.

Checklist

For maintainers

Choose a reason for hiding this comment

alexwizp commented Dec 29, 2021

alexwizp commented Dec 29, 2021

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 Jan 4, 2022

ThomThomson left a comment

Choose a reason for hiding this comment

stratoula 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

flash1293 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

flash1293 left a comment

Choose a reason for hiding this comment

stratoula left a comment

Choose a reason for hiding this comment

kibana-ci commented Jan 12, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

async chunk count

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

History

DianaDerevyankina commented Dec 17, 2021 •

edited

Loading

Merged `visualize` into `visualizations` plugin.