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

Migrate vis_type_table plugin to synchronous lifecycle #90582

Closed
pgayvallet opened this issue Feb 8, 2021 · 4 comments · Fixed by #111339
Closed

Migrate vis_type_table plugin to synchronous lifecycle #90582

pgayvallet opened this issue Feb 8, 2021 · 4 comments · Fixed by #111339
Assignees
Labels
discuss Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@pgayvallet
Copy link
Contributor

Required for #53268

In #89562, we migrated almost all plugins to synchronous lifecycle.

However, the vis_type_table plugin could not be migrated, because of the conditional lazy import the it is performing

public async setup(
core: CoreSetup<TablePluginStartDependencies>,
deps: TablePluginSetupDependencies
) {
const { legacyVisEnabled } = this.initializerContext.config.get();
if (legacyVisEnabled) {
const { registerLegacyVis } = await import('./legacy');
registerLegacyVis(core, deps, this.initializerContext);
} else {
const { registerTableVis } = await import('./register_vis');
registerTableVis(core, deps, this.initializerContext);
}
}

Note that those config-based conditional imports seems to make sense, as they reduce the initial-load bundle size. However, this is not something that is going to work with synchronous plugins, nor a use-case we really thought about.

I'm unsure what should be done here to be honest:

  1. Remove these lazy import and just load everything in the initial bundle by using 'plain' imports

Seems the most pragmatic, but OTOH it just doesn't feel right.

  1. Keep the lazy import but remove the awaits and have the registerXXXVis performing the registration asynchronously.

Seems like an even worse option

  1. ???

WDYT all?

@pgayvallet pgayvallet added discuss Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Feb 8, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@elasticmachine
Copy link
Contributor

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

@stratoula
Copy link
Contributor

stratoula commented Feb 15, 2021

@pgayvallet I just saw this! Thanx for reporting it. Right now this visualization has two implementations, the legacy one and the default one (which uses Eui datagrid). We plan to remove the legacy one on the next minors (before 7.15). When the old implementation will be removed this conditional lazy import will be removed too. Do you think we can wait for some minors? Or is it crucial from your side?

@pgayvallet
Copy link
Contributor Author

Do you think we can wait for some minors? Or is it crucial from your side?

It would be totally fine to wait

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants