Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Visualizations] Remove charts - editor plugins cyclic dependencies #84887

Merged
merged 14 commits into from
Dec 22, 2020

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented Dec 3, 2020

Summary

Part of #84750
Removes circular dependencies between charts and vis_default_editor plugins.
To be more specific, we decided to move the editor shared components that are used on the Options tab and currently live on the charts plugin to the vis_default_editor plugin. We are doing this not only to remove the circular dependency between these two plugins but also the editor is the plugin that should provde these components to the other plugins.

I have also moved the translations from charts to the vis_default_editor plugin.

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

078) src/plugins/vis_default_editor/public/index.ts -> src/plugins/vis_default_editor/public/default_editor_controller.tsx -> src/plugins/visualize/public/index.ts -> src/plugins/visualize/public/plugin.ts -> src/plugins/visualize/public/application/index.tsx -> src/plugins/visualize/public/application/app.tsx -> src/plugins/visualize/public/application/components/index.ts -> src/plugins/visualize/public/application/components/visualize_listing.tsx -> src/plugins/visualize/public/application/utils/index.ts -> src/plugins/visualize/public/application/utils/use/index.ts -> src/plugins/visualize/public/application/utils/use/use_saved_vis_instance.ts -> src/plugins/visualize/public/application/utils/get_visualization_instance.ts -> src/plugins/discover/public/index.ts -> src/plugins/discover/public/plugin.ts -> src/plugins/charts/public/index.ts -> src/plugins/charts/public/static/index.ts -> src/plugins/charts/public/static/components/index.ts -> src/plugins/charts/public/static/components/basic_options.tsx
079) src/plugins/vis_default_editor/public/index.ts -> src/plugins/vis_default_editor/public/default_editor_controller.tsx -> src/plugins/visualize/public/index.ts -> src/plugins/visualize/public/plugin.ts -> src/plugins/visualize/public/application/index.tsx -> src/plugins/visualize/public/application/app.tsx -> src/plugins/visualize/public/application/components/index.ts -> src/plugins/visualize/public/application/components/visualize_listing.tsx -> src/plugins/visualize/public/application/utils/index.ts -> src/plugins/visualize/public/application/utils/use/index.ts -> src/plugins/visualize/public/application/utils/use/use_saved_vis_instance.ts -> src/plugins/visualize/public/application/utils/get_visualization_instance.ts -> src/plugins/discover/public/index.ts -> src/plugins/discover/public/plugin.ts -> src/plugins/charts/public/index.ts -> src/plugins/charts/public/static/index.ts -> src/plugins/charts/public/static/components/index.ts -> src/plugins/charts/public/static/components/color_ranges.tsx
080) src/plugins/vis_default_editor/public/index.ts -> src/plugins/vis_default_editor/public/default_editor_controller.tsx -> src/plugins/visualize/public/index.ts -> src/plugins/visualize/public/plugin.ts -> src/plugins/visualize/public/application/index.tsx -> src/plugins/visualize/public/application/app.tsx -> src/plugins/visualize/public/application/components/index.ts -> src/plugins/visualize/public/application/components/visualize_listing.tsx -> src/plugins/visualize/public/application/utils/index.ts -> src/plugins/visualize/public/application/utils/use/index.ts -> src/plugins/visualize/public/application/utils/use/use_saved_vis_instance.ts -> src/plugins/visualize/public/application/utils/get_visualization_instance.ts -> src/plugins/discover/public/index.ts -> src/plugins/discover/public/plugin.ts -> src/plugins/charts/public/index.ts -> src/plugins/charts/public/static/index.ts -> src/plugins/charts/public/static/components/index.ts -> src/plugins/charts/public/static/components/color_schema.tsx

@stratoula stratoula added the Feature:Visualizations Generic visualization features (in case no more specific feature label is available) label Dec 3, 2020
@stratoula stratoula changed the title Remove charts - editor cyclic dependencies [Visualizations] Remove charts - editor plugins cyclic dependencies Dec 3, 2020
@stratoula stratoula added release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v8.0.0 labels Dec 3, 2020
@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@alexwizp alexwizp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Tested locally

@stratoula stratoula added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Dec 7, 2020
@stratoula stratoula marked this pull request as ready for review December 7, 2020 09:55
@stratoula stratoula requested a review from a team December 7, 2020 09:55
@stratoula stratoula requested review from a team as code owners December 7, 2020 09:55
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@tylersmalley tylersmalley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️ removal from circular dependency allow list

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@kindsun kindsun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Legacy, Region & Tile Maps changes lgtm!

  • code review
  • tested locally in chrome

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@sulemanof sulemanof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, didn't test!
Thanks for the movement! 🙏

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula stratoula added v7.12.0 and removed v7.11.0 labels Dec 14, 2020
@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code only review - Presentation changes LGTM

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested in Chrome and LGTM - I clicked around in various legacy chart types and they seem to work fine. Code looks good as well

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
charts 76 67 -9
visDefaultEditor 115 125 +10
total +1

Async chunks

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

id before after diff
visTypeMarkdown 12.7KB 12.8KB +58.0B
visTypeMetric 20.7KB 20.7KB +1.0B
visTypeTable 153.8KB 153.9KB +80.0B
visTypeTagcloud 284.5KB 284.5KB -1.0B
visTypeVislib 643.6KB 643.7KB +145.0B
visTypeXy 137.4KB 137.4KB +1.0B
total +284.0B

Distributable file count

id before after diff
default 47144 47904 +760

Page load bundle

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

id before after diff
charts 178.5KB 163.5KB -15.0KB
mapsLegacy 94.2KB 94.2KB +10.0B
regionMap 44.3KB 44.4KB +60.0B
tileMap 44.5KB 44.5KB +50.0B
visDefaultEditor 30.9KB 46.0KB +15.1KB
visTypeMarkdown 14.6KB 14.4KB -233.0B
visTypeMetric 26.3KB 26.4KB +85.0B
visTypeTable 11.7KB 11.5KB -233.0B
visTypeTagcloud 19.2KB 19.0KB -297.0B
total -389.0B

History

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

@stratoula stratoula merged commit a79e8a3 into elastic:master Dec 22, 2020
stratoula added a commit to stratoula/kibana that referenced this pull request Dec 22, 2020
…lastic#84887)

* Remove charts - editor cyclic dependencies

* Move translations from the charts plugin to the editor plugin

* Remove the dependency from the script as it is gone

* Fix types

Co-authored-by: Kibana Machine <[email protected]>
stratoula added a commit that referenced this pull request Dec 22, 2020
…84887) (#86730)

* Remove charts - editor cyclic dependencies

* Move translations from the charts plugin to the editor plugin

* Remove the dependency from the script as it is gone

* Fix types

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Dec 22, 2020
* master: (36 commits)
  update apm index pattern (elastic#86739)
  [Visualizations] Remove vis_default_editor - visualize plugins cyclic dependencies (elastic#85422)
  [ML] Fix alignment of values in data frame analytics results view badges (elastic#86621)
  [Visualizations] Remove charts - editor plugins cyclic dependencies (elastic#84887)
  fixing blank page (elastic#86640)
  Update dependency vega to ^5.17.1 (elastic#86715)
  [Monitoring] Convert Kibana-related server files that read from _source to typescript (elastic#86364)
  Uses @elastic/elasticsearch-canary (elastic#86398)
  [CI] Removes script previously used for Karma (elastic#86412)
  [build] Remove grunt checkPlugins task (elastic#85852)
  [build] Remove grunt docker:docs task (elastic#85848)
  [ML] Add doc link for classification AUC ROC evaluation (elastic#86660)
  [ML] Edits saved object synchronization message (elastic#86664)
  Uses the new es client in canvas usage collector's fetch methods (elastic#86668)
  [ML] Support legacy watcher URL (elastic#86661)
  [ML] Fix Single Metric Viewer y domain extending beyond the visible focus area (elastic#86655)
  Migrates search telemetry usage collector es client from legacy to new (elastic#86597)
  [Alerting] Encourage type safe usage of Alerting (elastic#86623)
  Migrates kql_telemetry usage collector es client (elastic#86585)
  [ML] Fix time range adjustment for the swim lane causing the infinite loop update (elastic#86461)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants