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

[Visualize] Lazy load default editor, fix duplicated styles #66732

Merged
merged 7 commits into from
May 20, 2020

Conversation

sulemanof
Copy link
Contributor

@sulemanof sulemanof commented May 15, 2020

Summary

Part of #64517

This is intended to split loading the default editor code and prevent duplicating of css rules across all places where stuff is imported from vis_default_editor/public (the most of vis types)

image

The EuiLoadingChart mono is added as spinner (more visible if internet is slow):

image

And the EuiErrorBoundary if something went wrong:

image

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@sulemanof sulemanof changed the title [Visualize] Lazy load bundle, fix duplicated styles [Visualize] Lazy load default editor, fix duplicated styles May 15, 2020
@sulemanof sulemanof marked this pull request as ready for review May 18, 2020 12:43
@sulemanof sulemanof requested a review from a team May 18, 2020 12:43
@sulemanof sulemanof requested a review from a team as a code owner May 18, 2020 12:43
@flash1293
Copy link
Contributor

I checked this and it only brings the initial bundle from 1.3M to 1.2M. One quick win would be to not import the default editor within the initial bundle (20% contributor):

src/plugins/visualize/public/kibana_services.ts
37:import { DefaultEditorController } from '../../vis_default_editor/public';

src/plugins/visualize/public/plugin.ts
43:import { DefaultEditorController } from '../../vis_default_editor/public';

For some reason, currently the stateless "DefaultEditorController" is put into the services object directly in plugin.ts. It looks like it would be totally fine to just import the default controller in src/plugins/visualize/public/application/editor/visualization_editor.js where it's used. It looks like this is also the only way lodash is introduced into the initial bundle (another 20% contributor), but maybe it's also imported from somewhere else, not sure.

Also, 30% of the initial bundle is caused by core, probably because of the DEFAULT_APP_CATEGORIES import in plugin.ts. @restrry are there plans to avoid this or something non-hacky we can do from our side?
Screenshot 2020-05-18 at 14 40 23

@mshustov
Copy link
Contributor

Also, 30% of the initial bundle is caused by core, probably because of the DEFAULT_APP_CATEGORIES import in plugin.ts. @restrry are there plans to avoid this or something non-hacky we can do from our side?

@flash1293 we are still planning to land tree-shaking in 7.9 (and hopefully we can port it to 7.8)

@flash1293
Copy link
Contributor

@restrry Thanks, makes sense. I'm really excited for this :)

@sulemanof I think we can reap the vis_default_editor benefits now, but anything else can probably be postponed till after tree shaking lands.

@sulemanof sulemanof added release_note:skip Skip the PR/issue when compiling release notes v7.8.0 v7.9.0 v8.0.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure labels May 19, 2020
@elasticmachine
Copy link
Contributor

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

@flash1293
Copy link
Contributor

Down to 837K 💯 Almost all of the rest of the bundle is core being pulled in which should be fixed by tree shaking.

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 Firefox, Visualize still works, LGTM.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

I didn't test locally, but the SASS imports look ok.

@sulemanof sulemanof merged commit 66c24c5 into elastic:master May 20, 2020
@sulemanof sulemanof deleted the fix/editor_loading branch May 20, 2020 14:10
sulemanof added a commit to sulemanof/kibana that referenced this pull request May 20, 2020
…66732)

* Lazy load bundle, fix duplicated styles

* Add EuiLoadingChart and EuiErrorBoundary

* Return localStorage service

* Use mono spinner

* Get rid of loading the editor outside of visualize mounting
# Conflicts:
#	src/plugins/visualize/public/kibana_services.ts
#	src/plugins/visualize/public/plugin.ts
sulemanof added a commit that referenced this pull request May 20, 2020
…67116)

* Lazy load bundle, fix duplicated styles

* Add EuiLoadingChart and EuiErrorBoundary

* Return localStorage service

* Use mono spinner

* Get rid of loading the editor outside of visualize mounting
# Conflicts:
#	src/plugins/visualize/public/kibana_services.ts
#	src/plugins/visualize/public/plugin.ts
sulemanof added a commit that referenced this pull request May 20, 2020
…67115)

* Lazy load bundle, fix duplicated styles

* Add EuiLoadingChart and EuiErrorBoundary

* Return localStorage service

* Use mono spinner

* Get rid of loading the editor outside of visualize mounting
jloleysens added a commit to jloleysens/kibana that referenced this pull request May 20, 2020
…ent/add-support-in-url-for-hidden-toggle

* 'master' of github.com:elastic/kibana: (49 commits)
  [Uptime] Improve responsiveness details page (elastic#67034)
  skip flaky suite (elastic#66669)
  Revert "Integration of a static filesystem for the node_modules (elastic#47998)" (elastic#67124)
  Support api_integration/kibana/stats against remote hosts (elastic#53000)
  chore(NA): add module name mapper for src plugins on x-pack (elastic#67103)
  Change the error message on TSVB in order to be more user friendly (elastic#67090)
  [kbn/optimizer] poll parent process to avoid zombie processes (elastic#67059)
  [Visualize] Lazy load default editor, fix duplicated styles (elastic#66732)
  Bump styled-component dependencies (elastic#66611)
  Bump react-markdown dependencies (elastic#66615)
  Fix Core docs links (elastic#66977)
  Timelion graph is not refreshing content after searching or filtering (elastic#67023)
  Remove `--xpack.endpoint.enabled=true` from README.md file (elastic#67053)
  Move apm tutorial from apm plugin into apm_oss plugin (elastic#66432)
  [Logs UI] Restore call to `UsageCollector.countLogs` (elastic#67051)
  Remove unused license check result from LP Security plugin (elastic#66966)
  [Saved Objects] adds support for including hidden types in saved objects client (elastic#66879)
  [Discover] Deangularize timechart header (elastic#66532)
  [Discover] Improve and unskip a11y context view test (elastic#66959)
  [SIEM] Refactor Timeline.timelineType draft to Timeline.status draft (elastic#66864)
  ...

# Conflicts:
#	x-pack/plugins/index_management/__jest__/client_integration/helpers/home.helpers.ts
gmmorris added a commit to gmmorris/kibana that referenced this pull request May 21, 2020
* master:
  [apm] Annotation API documentation (elastic#65963)
  [Uptime] Improve responsiveness details page (elastic#67034)
  skip flaky suite (elastic#66669)
  Revert "Integration of a static filesystem for the node_modules (elastic#47998)" (elastic#67124)
  Support api_integration/kibana/stats against remote hosts (elastic#53000)
  chore(NA): add module name mapper for src plugins on x-pack (elastic#67103)
  Change the error message on TSVB in order to be more user friendly (elastic#67090)
  [kbn/optimizer] poll parent process to avoid zombie processes (elastic#67059)
  [Visualize] Lazy load default editor, fix duplicated styles (elastic#66732)
  Bump styled-component dependencies (elastic#66611)
  Bump react-markdown dependencies (elastic#66615)
  Fix Core docs links (elastic#66977)
  Timelion graph is not refreshing content after searching or filtering (elastic#67023)
  Remove `--xpack.endpoint.enabled=true` from README.md file (elastic#67053)
  Move apm tutorial from apm plugin into apm_oss plugin (elastic#66432)
  [Logs UI] Restore call to `UsageCollector.countLogs` (elastic#67051)
  Remove unused license check result from LP Security plugin (elastic#66966)
gmmorris added a commit to gmmorris/kibana that referenced this pull request May 21, 2020
* master: (21 commits)
  [Alerting] Hides the `alert` SavedObjects type (elastic#66719)
  skip flaky suite (elastic#66869)
  fix visual baseline tests
  [kbn/optimizer] require fsevents on macos (elastic#67147)
  [APM] Fix obscured service map connections (elastic#67129)
  [apm] Annotation API documentation (elastic#65963)
  [Uptime] Improve responsiveness details page (elastic#67034)
  skip flaky suite (elastic#66669)
  Revert "Integration of a static filesystem for the node_modules (elastic#47998)" (elastic#67124)
  Support api_integration/kibana/stats against remote hosts (elastic#53000)
  chore(NA): add module name mapper for src plugins on x-pack (elastic#67103)
  Change the error message on TSVB in order to be more user friendly (elastic#67090)
  [kbn/optimizer] poll parent process to avoid zombie processes (elastic#67059)
  [Visualize] Lazy load default editor, fix duplicated styles (elastic#66732)
  Bump styled-component dependencies (elastic#66611)
  Bump react-markdown dependencies (elastic#66615)
  Fix Core docs links (elastic#66977)
  Timelion graph is not refreshing content after searching or filtering (elastic#67023)
  Remove `--xpack.endpoint.enabled=true` from README.md file (elastic#67053)
  Move apm tutorial from apm plugin into apm_oss plugin (elastic#66432)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.8.0 v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants