Remove src/plugins/vis_default_editor -> src/plugins/visualizations cyclic dependencies #86988

alexwizp · 2020-12-29T13:27:10Z

Part of #84750

Summary

Removes circular dependencies between vis_default_editor and visualizations plugins.

The dependencies that were removed based on the node scripts/find_plugins_with_circular_deps --debug script.

Dev docs

What was changed in that PR:

ISchemas and Schema interfaces were moved from vis_default_editor to visualizations plugin.
Schemas class was moved from vis_default_editor to visualizations plugin (now it's a private class, we should not use it out of visualizations plugin)
Type definition object for visualizations was changed:

Before:

  {
       editorConfig: {
         schemas: new Schemas([{schemaObject1, schemasObject2}])
      }
    }

Now:

{
     editorConfig: {
       schemas: [{schemaObject1, schemasObject2}]
    }
  }

…yclic dependencies Part of elastic#86758

elasticmachine · 2020-12-29T16:10:38Z

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

jbudz

operations - Circular dependency list LGTM

src/plugins/vis_default_editor/public/components/agg_common_props.ts

sulemanof · 2020-12-30T11:37:17Z

src/plugins/vis_default_editor/public/schemas.ts

-  tooltip?: ReactNode;
-}
+import { AggGroupNames, AggParam } from '../../data/public';
+import type { ISchemas, Schema } from '../../visualizations/public';

 export class Schemas implements ISchemas {


discuss: I don't have a strong opinion here, but I'm not sure the Schemas class should stay in vis_default_editor since now..
Is there any reason to leave it here ?
Assuming interfaces were moved into visualizations, the class definition should be also moved there.

good point, I've checked that and looks like you are right. visualizations is more suitable place for that. Initially I thought that we use it only in vis_default_editor

I don't have a strong opinion either but I think that Schemas class is highly connected with the editor so I am not sure that it should be part of the visualizations plugin.

@stratoula I don't agree with you that it's more connected with vis_default_editor. From my point of view It's related to visualizations cause other plugins read this value though public schemas field in VisType object (see src/plugins/visualizations/public/vis_types/base_vis_type.ts)

What I did it my last commit:

Schemas marked as private for vizualizations plugin and now we use it only from one place. In future we can probably remove it cause I don't see any reason to keep that class. We use it only for restructure initial type.editorConfig.schemas array. Perhaps we can create simple static functions to make this more obvious.

@timroes please correct me if I'm wrong but I see our current design like on screen below (sorry for ugly picture). The main idea here that our visualizations should not know about about the visDefaultEditor and use API-types from visualizations plugin. It allow us to simply change change the editor just by modifying schema in future

Ok Aliaksei, I see your point and the last change has made things a little bit better. I will approve it as it removes the dependencies between the plugins.

I think Schemas were already a bit on the fence and still are unfortunately. While I think the sole purpose they are used today are the default editor and to build its UI (and thus I would say semantically they belong most into the default editor plugin), we returned them as a high level concept from the VisType as Alexej mentioned and thus they became a core concept of the vis registration itself (and should belong in visualizations).

So I would see two options:

Change the API that Schemas will not be returned as a core concept of visualization registry, but be purely part of the editor, e.g. by passing it into a property of the default editor explciitly and always specifying the default editor explicitly (like registerVisType({ ..., editor: () => <DefaultEditor schemas={schemas} />, ... })) and then move then also in vis default editor and keep them there, or

(the easier solution for now): don't change the APIs and keep them as they are, in which case I think they need to live in Visualizations for now, since they are a high level concern of VisType (and the registry).

@timroes It's looks like a right things, but unfortunately not only the vis_default_editor uses schemas and it was a blocker why I didn't remove it at all in that PR. At least one plugin vis_type_xy reads that value (see src/plugins/vis_type_xy/public/editor/components/options/metrics_axes/index.tsx) and to avoid any regression I decided to keep it in visualizations plugin. Semantically currently it's a best place for that.

From my point of view, we should:

visualizations plugin should have only Schema interface; It probably should be renamed to EditorSchema

Schemas class and ISchema interface should be removed at all;

For vis_default_editor we should create simple functions to extract metrics/buckets from the editorConfig.schemas in more obvious way.

Think why vis_type_xy uses schemas. Looks strange

But it's out of that PR. If you agree with me or have your own plan to refactor that let's create a new item for that.

src/plugins/vis_default_editor/public/components/sidebar/state/actions.ts

src/plugins/vis_default_editor/public/components/sidebar/sidebar.tsx

sulemanof

LGTM, didn't test
The only my concern is about leaving the Schema class in vis_default_editor, but it's OK to be merged overall

kindsun

Region and tile map changes lgtm w/ green CI!

code review
tested locally in chrome

alexwizp · 2021-01-03T12:04:44Z

@elasticmachine merge upstream

alexwizp · 2021-01-05T07:52:01Z

@elasticmachine merge upstream

# Conflicts: # src/plugins/visualizations/public/vis_types/types.ts

alexwizp · 2021-01-06T10:29:52Z

@elasticmachine merge upstream

alexwizp · 2021-01-10T11:16:27Z

@elasticmachine merge upstream

kibanamachine · 2021-01-10T11:16:39Z

expected head sha didn’t match current head ref.

alexwizp · 2021-01-11T07:03:51Z

@elasticmachine merge upstream

stratoula

LGTM, tested locally with Safari. No regressions found and the cyclic dependencies are gone.

kibanamachine · 2021-01-11T09:20:28Z

💚 Build Succeeded

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

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`visualizations`	153	154	+1

Async chunks

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

id	before	after	diff
`visDefaultEditor`	300.9KB	301.1KB	+232.0B
`visTypeTable`	153.9KB	153.7KB	-188.0B
`visTypeVislib`	643.7KB	643.7KB	+1.0B
`visualizations`	61.4KB	61.4KB	+2.0B
total			+47.0B

Page load bundle

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

id	before	after	diff
`regionMap`	44.4KB	44.4KB	-41.0B
`tileMap`	44.5KB	44.5KB	-41.0B
`visDefaultEditor`	46.5KB	44.3KB	-2.2KB
`visTypeMetric`	26.4KB	26.3KB	-41.0B
`visTypeTagcloud`	19.0KB	18.9KB	-16.0B
`visTypeVislib`	46.7KB	46.4KB	-217.0B
`visTypeXy`	68.3KB	68.0KB	-247.0B
`visualizations`	169.3KB	171.2KB	+1.9KB
total			-895.0B

History

💛 Build #97778 was flaky 0c4bee4
💚 Build #97224 succeeded 79fe1ab
💔 Build #97204 failed abd1d6c
💚 Build #96922 succeeded a717114
💔 Build #96904 failed 3b4c3da

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

…yclic dependencies (elastic#86988) * Remove src/plugins/vis_default_editor -> src/plugins/visualizations cyclic dependencies Part of elastic#86758 * Update agg_common_props.ts * Schemas -> visualizations * fix JEST * make Schemas class private for visualizations plugin * fix type error * fix markdown app in visualize app * remove visualizations from kibana.json's Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* master: [APM] Define placement “Right” to offset tooltip (elastic#87729) Fix UI glitch on SOM delete confirmation modal (elastic#87623) Remove src/plugins/vis_default_editor -> src/plugins/visualizations cyclic dependencies (elastic#86988) [Timelion] Fix tests flakiness on suggestion click (elastic#87273) [Uptime] Fix/details page tabs (elastic#86296) [ML] Fix earliest and latest texts for date fields (elastic#87482) chore(NA): move grokdebugger plugin test fixtures out of __tests__ folder (elastic#87765) [Security Solution] Refactor Cypress scenarios to use internal contex… (elastic#86609) [Security Solution] Unskip cypress tests (elastic#86653)

…yclic dependencies (#86988) (#87804) * Remove src/plugins/vis_default_editor -> src/plugins/visualizations cyclic dependencies Part of #86758 * Update agg_common_props.ts * Schemas -> visualizations * fix JEST * make Schemas class private for visualizations plugin * fix type error * fix markdown app in visualize app * remove visualizations from kibana.json's Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

alexwizp mentioned this pull request Dec 29, 2020

Remove circular deps between KibanaApp team plugins #84750

Closed

5 tasks

alexwizp requested review from stratoula and sulemanof December 29, 2020 13:34

alexwizp self-assigned this Dec 29, 2020

alexwizp added v7.12.0 v8.0.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:skip Skip the PR/issue when compiling release notes Feature:Visualizations Generic visualization features (in case no more specific feature label is available) labels Dec 29, 2020

Remove src/plugins/vis_default_editor -> src/plugins/visualizations c…

e001422

…yclic dependencies Part of elastic#86758

alexwizp force-pushed the 86758-3 branch from 5d05383 to e001422 Compare December 29, 2020 13:40

alexwizp requested review from DianaDerevyankina and VladLasitsa December 29, 2020 13:42

elastic deleted a comment from kibanamachine Dec 29, 2020

alexwizp marked this pull request as ready for review December 29, 2020 16:10

alexwizp requested a review from a team December 29, 2020 16:10

alexwizp requested a review from a team as a code owner December 29, 2020 16:10

jbudz approved these changes Dec 29, 2020

View reviewed changes

sulemanof reviewed Dec 30, 2020

View reviewed changes

src/plugins/vis_default_editor/public/components/agg_common_props.ts Outdated Show resolved Hide resolved

Update agg_common_props.ts

0b030bc

sulemanof reviewed Dec 30, 2020

View reviewed changes

src/plugins/vis_default_editor/public/components/sidebar/state/actions.ts Outdated Show resolved Hide resolved

sulemanof reviewed Dec 30, 2020

View reviewed changes

src/plugins/vis_default_editor/public/components/sidebar/sidebar.tsx Show resolved Hide resolved

sulemanof approved these changes Dec 30, 2020

View reviewed changes

Schemas -> visualizations

aadfd8c

alexwizp force-pushed the 86758-3 branch from 616531e to aadfd8c Compare December 30, 2020 15:51

alexwizp requested a review from a team as a code owner December 30, 2020 15:51

kindsun approved these changes Dec 30, 2020

View reviewed changes

kibanamachine and others added 2 commits January 3, 2021 07:04

Merge branch 'master' into 86758-3

026d3ed

fix JEST

4abf76e

kibanamachine and others added 5 commits January 5, 2021 02:52

Merge branch 'master' into 86758-3

af6efc9

make Schemas class private for visualizations plugin

84ae45f

Merge remote-tracking branch 'upstream/master' into 86758-3

6d614de

# Conflicts: # src/plugins/visualizations/public/vis_types/types.ts

fix type error

3b4c3da

fix markdown app in visualize app

a717114

alexwizp added release_note:breaking and removed release_note:skip Skip the PR/issue when compiling release notes labels Jan 5, 2021

alexwizp added 2 commits January 6, 2021 11:25

Merge remote-tracking branch 'upstream/master' into 86758-3

2fad5b6

remove visualizations from kibana.json's

abd1d6c

Merge branch 'master' into 86758-3

79fe1ab

Merge branch 'master' into 86758-3

0c4bee4

Merge branch 'master' into 86758-3

f67403c

stratoula approved these changes Jan 11, 2021

View reviewed changes

alexwizp merged commit 66c8b2a into elastic:master Jan 11, 2021

alexwizp mentioned this pull request Jan 11, 2021

[7.x] Remove src/plugins/vis_default_editor -> src/plugins/visualizations cyclic dependencies (#86988) #87804

Merged

timroes added release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. and removed release_note:breaking labels Mar 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove src/plugins/vis_default_editor -> src/plugins/visualizations cyclic dependencies #86988

Remove src/plugins/vis_default_editor -> src/plugins/visualizations cyclic dependencies #86988

alexwizp commented Dec 29, 2020 •

edited

Loading

elasticmachine commented Dec 29, 2020

jbudz left a comment

sulemanof Dec 30, 2020

alexwizp Dec 30, 2020

stratoula Jan 5, 2021 •

edited

Loading

alexwizp Jan 5, 2021

stratoula Jan 11, 2021

timroes Jan 11, 2021

alexwizp Jan 11, 2021

sulemanof left a comment

kindsun left a comment

alexwizp commented Jan 3, 2021

alexwizp commented Jan 5, 2021

alexwizp commented Jan 6, 2021

alexwizp commented Jan 10, 2021

kibanamachine commented Jan 10, 2021

alexwizp commented Jan 11, 2021

stratoula left a comment

kibanamachine commented Jan 11, 2021

Remove src/plugins/vis_default_editor -> src/plugins/visualizations cyclic dependencies #86988

Remove src/plugins/vis_default_editor -> src/plugins/visualizations cyclic dependencies #86988

Conversation

alexwizp commented Dec 29, 2020 • edited Loading

Summary

Dev docs

Before:

Now:

elasticmachine commented Dec 29, 2020

jbudz left a comment

Choose a reason for hiding this comment

sulemanof Dec 30, 2020

Choose a reason for hiding this comment

alexwizp Dec 30, 2020

Choose a reason for hiding this comment

stratoula Jan 5, 2021 • edited Loading

Choose a reason for hiding this comment

alexwizp Jan 5, 2021

Choose a reason for hiding this comment

stratoula Jan 11, 2021

Choose a reason for hiding this comment

timroes Jan 11, 2021

Choose a reason for hiding this comment

alexwizp Jan 11, 2021

Choose a reason for hiding this comment

sulemanof left a comment

Choose a reason for hiding this comment

kindsun left a comment

Choose a reason for hiding this comment

alexwizp commented Jan 3, 2021

alexwizp commented Jan 5, 2021

alexwizp commented Jan 6, 2021

alexwizp commented Jan 10, 2021

kibanamachine commented Jan 10, 2021

alexwizp commented Jan 11, 2021

stratoula left a comment

Choose a reason for hiding this comment

kibanamachine commented Jan 11, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

alexwizp commented Dec 29, 2020 •

edited

Loading

stratoula Jan 5, 2021 •

edited

Loading