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

load assets lazily in es-ui plugins #62487

Merged
merged 3 commits into from
Apr 6, 2020

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Apr 3, 2020

Summary

Part of #62263
Implements lazy loading for management sections + share url.

name size before size after
share 837Kb 76 Kb
upgrade assistant 324Kb 59Kb
index management 3.2Mb 3.0 Mb
snapshot restore 3Mb 3Mb

@mshustov mshustov added chore v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Apr 3, 2020
@mshustov mshustov requested review from a team as code owners April 3, 2020 17:04
Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Looks good, but can you help me understand the changes to Index Management and Snapshot Restore? Both of those apps were already lazy-loading before this change. So what do these changes accomplish?

Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

App arch change LGTM

@mshustov
Copy link
Contributor Author

mshustov commented Apr 6, 2020

Looks good, but can you help me understand the changes to Index Management and Snapshot Restore? Both of those apps were already lazy-loading before this change. So what do these changes accomplish?

Index management uses singleton services that initialized on the first import. It means that:

  • breadcrumbService
  • documentationService
  • httpService
  • notificationService
  • uiMetricService

present in the plugin bundle, even though they required when the app/management section is rendered.
I moved breadcrumbService & documentationService to be imported on management section mount lazily. I'm not familiar with your code good enough to move other services behind async loading and I leave it up to you.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@mshustov mshustov merged commit 5940b72 into elastic:master Apr 6, 2020
@mshustov mshustov deleted the issue-62263-es-ui-lazy-load branch April 6, 2020 10:29
mshustov added a commit to mshustov/kibana that referenced this pull request Apr 6, 2020
* load assets lazily

* addres cj comments
mshustov added a commit to mshustov/kibana that referenced this pull request Apr 6, 2020
* load assets lazily

* addres cj comments
mshustov added a commit that referenced this pull request Apr 6, 2020
* load assets lazily

* addres cj comments
mshustov added a commit that referenced this pull request Apr 6, 2020
* load assets lazily

* addres cj comments
@sebelga
Copy link
Contributor

sebelga commented Apr 6, 2020

Thanks for doing this @restrry !
So you recommend to move all the logic that we currently have in the plugin setup() cycle into the lazy-loaded "mount" file you've created?

// This is for example the logic for index_management

const { http, notifications } = coreSetup;
const { usageCollection, management } = plugins;

httpService.setup(http);
notificationService.setup(notifications);
this.uiMetricService.setup(usageCollection);

// And here we register the app with the `mount` handler
management.sections.getSection('elasticsearch')!.registerApp({ ...

Note: the gain on "snapshot_restore" is quite insignificant 😊

@mshustov
Copy link
Contributor Author

mshustov commented Apr 6, 2020

So you recommend to move all the logic that we currently have in the plugin setup() cycle into the lazy-loaded "mount" file you've created?

To move everything that possible behind mount. It's usually related to UI-specific API. Note that you cannot use any setup API from in mount since mount is called after start.

@sebelga
Copy link
Contributor

sebelga commented Apr 7, 2020

Are you saying that the above

const { http, notifications } = coreSetup;

is it not available in the closure?

setup(coreSetup: CoreSetup) {
  const { http, notifications } = coreSetup;

  management.sections.getSection('elasticsearch')!.registerApp({
    id: PLUGIN.id,
    title: 'myPlugin',
    order: 1,
    mount: async params => {
      // Initiate a service with core HTTP
	  // Is this possible?
      const apiService = new MyApiService(http);
      const { mountManagementSection } = await import('./mount_app');
      const services = {
        apiService,
      };
      return mountManagementSection(services);
    },
  });

@mshustov
Copy link
Contributor Author

mshustov commented Apr 7, 2020

@sebelga Even if it works atm, you shouldn't rely on it in the future. setup is preparational phase. you shouldn't use any setup API (besides getStartServices) after your plugin started.

@sebelga
Copy link
Contributor

sebelga commented Apr 13, 2020

Ok, thanks for clarifying @restrry !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants