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

Async visualization registry #78400

Closed
timroes opened this issue Sep 24, 2020 · 3 comments
Closed

Async visualization registry #78400

timroes opened this issue Sep 24, 2020 · 3 comments
Labels
enhancement New value added to drive a business result Feature:Visualizations Generic visualization features (in case no more specific feature label is available) Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@timroes
Copy link
Contributor

timroes commented Sep 24, 2020

We should make the visualization registry allow for async registrations, so we can reduce the total page load bundle sizes across Kibana, and don't need to have every plugin bundle anything they need for the visualization registration inside the page load bundle.

Currently the API looks as follows:

visualizations.createBaseVisualization(VisConfig);

We should rather allow this to be the following:

visualization.createBaseVisualization(async () => Promise<VisConfig>);

So that only a function is registered which can async return a visualization configuration. This will allow a consumer to do codesplitting now:

visualization.createBaseVisualization(async () => import('./visualization_config'));

This means we need to change the API of the registration, but also need to change all consuming parts to be async, and only execute those registration functions once the first consumer is trying to access the registration.

We currently have some application, like maps actually reading the registry not for consuming but for disabling visualizations. We need to most likely create a separate modifyRegistry API for those, that will also only be executed once the first consumer tries to access the registry (so they won't trigger loading all of them on the maps plugin start up).

There might also be a couple of more issues around that, that we should outline in this issue.

@timroes timroes added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) enhancement New value added to drive a business result Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Sep 24, 2020
@elasticmachine
Copy link
Contributor

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

@flash1293
Copy link
Contributor

flash1293 commented Sep 24, 2020

Related issue: #58280

The way @timroes describes is one option to do this (and really close to how it's handled in this PR for Lens: #78142)

One downside of this approach is that a lot of bundles (probably one per visualization type) will be downloaded at once when accessing Visualize or Dashboards, even if not all of them are needed. Instead, we could also do the following:

Currently a visualization looks like this:

export interface BaseVisTypeOptions {
  name: string;
  title: string;
  description?: string;
  getSupportedTriggers?: () => Array<keyof TriggerContextMapping>;
  icon?: string;
  image?: string;
  stage?: 'experimental' | 'beta' | 'production';
  options?: Record<string, any>;
  visualization: VisualizationControllerConstructor | undefined;
  visConfig?: Record<string, any>;
  editor?: any;
  editorConfig?: Record<string, any>;
  hidden?: boolean;
  requestHandler?: string | unknown;
  responseHandler?: string | unknown;
  hierarchicalData?: boolean | unknown;
  setup?: unknown;
  useCustomNoDataScreen?: boolean;
  inspectorAdapters?: Adapters | (() => Adapters);
  toExpressionAst?: VisToExpressionAst;
}

Most of these options are very lightweight and can be registered directly, with lazy async functions in the places where lots of logic has to be loaded:

export interface BaseVisTypeOptions {
  name: string;
  title: string;
  description?: string;
  loadRenderDeps: () => Promise<{
    getSupportedTriggers?: () => Array<keyof TriggerContextMapping>; 
    visualization: VisualizationControllerConstructor | undefined;
    visConfig?: Record<string, any>;
    inspectorAdapters?: Adapters | (() => Adapters);
    toExpressionAst?: VisToExpressionAst;
  }>;
  loadEditDeps: () => Promise<{
    editor?: any;
    editorConfig?: Record<string, any>;
  }>;
  icon?: string;
  image?: string;
  stage?: 'experimental' | 'beta' | 'production';
  options?: Record<string, any>;
  hidden?: boolean;
  requestHandler?: string | unknown;
  responseHandler?: string | unknown;
  hierarchicalData?: boolean | unknown;
  setup?: unknown;
  useCustomNoDataScreen?: boolean;
}

Doing that allows the following:

  • Go to Visualize: No async bundle loading
    • Show vis modal: No async bundle loading
    • Select visualization: Load editor bundle and visualization bundle
  • Go to Dashboard: No async bundle loading
    • Add visualization: Load visualization bundle just for this visualization

For example this would allow us to never load any timelion logic (except for a super slim plugin registering the icon and the name) if a user is not using timelion. Of course for most stuff this can be handled in the plugin itself (e.g. by doing the async loading in the editor component or in the visualization constructor) as well, but this makes it much easier for the visualization author to implement it correctly.

The visualize embeddable and editor will make sure to load the required parts when they are actually needed (these should be the only two places where render deps / edit deps are accessed).

@timroes
Copy link
Contributor Author

timroes commented Sep 30, 2020

@flash1293 and I discussed this earlier and we agree that it would make sense keeping editor and rendering seperate async imports/bundles, so we don't need to load the full editor when just watching a specific visualization on a dashboard. So basically the way Joe suggested above.

Given that as part of #46801 we'll make all renderers using async imports and that we already have a lot of async editor components (e.g. the default editor already is loaded using React.lazy), we think it's not worth for now continuing making the full registry async. Instead we should just continue with that road, and potentially check if we can make some more editor components async. Once we're through with those refactorings we should analyze where we are in terms of size. In case we see larger optimization potential by making the full registry async we should get back to this.

Closing this for now.

cc @stratoula @sulemanof @alexwizp

@timroes timroes closed this as completed Sep 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Feature:Visualizations Generic visualization features (in case no more specific feature label is available) Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

No branches or pull requests

3 participants