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.

Sign up for GitHub

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

Jump to bottom

[Visualizations] Remove vis_default_editor - visualize plugins cyclic dependencies #85422

Merged
merged 11 commits into from
Dec 22, 2020
Merged

[Visualizations] Remove vis_default_editor - visualize plugins cyclic dependencies #85422

merged 11 commits into from
Dec 22, 2020

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Dec 9, 2020 •
edited
Loading

Part of: #84750

Summary

Removes circular dependencies between vis_default_editor and visualize plugins.
The dependencies that were removed based on the node scripts/find_plugins_with_circular_deps --debug script.

What was changed in that PR:

  1. setDefaultEditor function was added into visualize API (setup hook).
  2. vis_default_editor still depends on visualize and execute setDefaultEditor hook on setup phase

These two changes allowed the dependency to be kept in only one direction: vis_default_editor -> visualize

@alexwizp
[Visualizations] Remove vis_default_editor - visualize plugins cyclic…
075d2ca 
… dependencies

# Conflicts:
#	src/plugins/visualize/kibana.json
@alexwizp alexwizp mentioned this pull request Dec 9, 2020
Closed
5 tasks
@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp alexwizp self-assigned this Dec 11, 2020
@alexwizp alexwizp added v7.12.0 v8.0.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Vis Editor Visualization editor issues release_note:skip Skip the PR/issue when compiling release notes labels Dec 11, 2020
@sulemanof
Copy link
Contributor

Hey! I'm fond of creating an editor setter in visualize plugin contract!
But, what if you create a registry instead ?
This would be possible to register other editors (since we also have the tsvb editor) only if visualize is enabled.
Then the editor would be picked up something like:

// vis.type.editor = 'tsvb'
const Editor = visualize.editorRegistry.find(vis.type.editor);

find would return the default one if editor is not specified

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp
Copy link
Contributor Author

@sulemanof Honestly I don't see any benefits of adding one more registry for that. Currently we can set a specific editor in visTypeDefinition object. It's a pretty flexible solution which works without any extra classes.

Probably we can implement your idea in future, but it should be definitely resolved in a separate PR, cause for me it looks like before using custom Editor it should be registered. It can be a blocker for some of ThirdParty plugins.

@alexwizp alexwizp marked this pull request as ready for review December 14, 2020 13:41
@alexwizp alexwizp requested a review from a team December 14, 2020 13:41
@alexwizp alexwizp requested a review from a team as a code owner December 14, 2020 13:41
@elasticmachine
Copy link
Contributor

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

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

jbudz
jbudz approved these changes Dec 17, 2020
View reviewed changes
Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

operations - circular dependencies list LGTM

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visDefaultEditor 125 126 +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 300.9KB +1.0B
visualize 130.9KB 130.8KB -52.0B
total -51.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
visDefaultEditor 46.0KB 46.5KB +491.0B
visualize 40.4KB 42.3KB +1.9KB
total +2.4KB

History

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

stratoula
stratoula approved these changes Dec 22, 2020
View reviewed changes
Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Dependency has been removed, checked it locally. Code LGTM. I tested it and it seems that works fine 😉

@alexwizp alexwizp merged commit e2c4356 into elastic:master Dec 22, 2020
@alexwizp alexwizp mentioned this pull request Dec 22, 2020
Merged
alexwizp added a commit to alexwizp/kibana that referenced this pull request Dec 22, 2020
@alexwizp @kibanamachine
[Visualizations] Remove vis_default_editor - visualize plugins cyclic…
08d25c5 
… dependencies (elastic#85422)

* [Visualizations] Remove vis_default_editor - visualize plugins cyclic dependencies

# Conflicts:
#	src/plugins/visualize/kibana.json

* fix CI

* fix CI

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Dec 22, 2020
@gmmorris
Merge branch 'master' into alerts/typed-action-groups
d1e98eb 
* 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)
  ...
alexwizp added a commit that referenced this pull request Dec 22, 2020
@alexwizp @kibanamachine
[Visualizations] Remove vis_default_editor - visualize plugins cyclic…
a74c4cc 
… dependencies (#85422) (#86753)

* [Visualizations] Remove vis_default_editor - visualize plugins cyclic dependencies

# Conflicts:
#	src/plugins/visualize/kibana.json

* fix CI

* fix CI

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@alexwizp alexwizp deleted the deps_1 branch January 16, 2021 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbudz jbudz jbudz approved these changes

@stratoula stratoula stratoula approved these changes

@sulemanof sulemanof Awaiting requested review from sulemanof

@timroes timroes Awaiting requested review from timroes

Assignees

@alexwizp alexwizp

Labels
Feature:Vis Editor Visualization editor issues 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@alexwizp @sulemanof @elasticmachine @kibanamachine @jbudz @stratoula