[legacy visualization] Register vis type with async callback#267598
[legacy visualization] Register vis type with async callback#267598nreese wants to merge 18 commits into
Conversation
|
/ci |
|
/ci |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| createBaseVisualization: <TVisParams extends VisParams>( | ||
| config: VisTypeDefinition<TVisParams> | ||
| ): void => { | ||
| const vis = new BaseVisType(config); | ||
| this.registerVisualization(vis); | ||
| this.registerVisualization(config.name, async () => new BaseVisType(config)); | ||
| }, |
There was a problem hiding this comment.
🔴 Critical vis_types/types_service.ts:53
In the deprecated createBaseVisualization method, async () => new BaseVisType(config) constructs a BaseVisType instance that is then passed to registerVisualization, which constructs another BaseVisType around it—creating a double-wrapped object. The registerVisualization function expects a factory returning Promise<VisTypeDefinition>, but receives a factory returning Promise<BaseVisType>. This causes the inner BaseVisType to be treated as the raw config object, duplicating defaults and breaking type expectations.
createBaseVisualization: <TVisParams extends VisParams>(
config: VisTypeDefinition<TVisParams>
): void => {
- this.registerVisualization(config.name, async () => new BaseVisType(config));
+ this.registerVisualization(config.name, async () => config);
},🤖 Copy this AI Prompt to have your agent fix this:
In file src/platform/plugins/shared/visualizations/public/vis_types/types_service.ts around lines 53-57:
In the deprecated `createBaseVisualization` method, `async () => new BaseVisType(config)` constructs a `BaseVisType` instance that is then passed to `registerVisualization`, which constructs another `BaseVisType` around it—creating a double-wrapped object. The `registerVisualization` function expects a factory returning `Promise<VisTypeDefinition>`, but receives a factory returning `Promise<BaseVisType>`. This causes the inner `BaseVisType` to be treated as the raw config object, duplicating defaults and breaking type expectations.
Evidence trail:
- PR diff for types_service.ts: git_diff base=MERGE_BASE head=REVIEWED_COMMIT path=src/platform/plugins/shared/visualizations/public/vis_types/types_service.ts
- types_service.ts lines 27-37 (registerVisualization): creates `new BaseVisType(config)` from the factory result
- types_service.ts line 53 (createBaseVisualization): passes `async () => new BaseVisType(config)` as the factory → double wrapping
- types_service.ts lines 60-66 (createBaseVisualizationAsync): correctly passes factory directly, no double wrapping
- base_vis_type.ts constructor: applies `defaultsDeep` for visConfig, editorConfig, options — these would be called twice on the same data in the double-wrap case
There was a problem hiding this comment.
@nreese this comment seems legit, in the registerVisualization function you are calling:
this.types[name] = async () => {
const config = await getVisDefinition();
return new BaseVisType(config);
};
so basically new BaseVisType(new BaseVisType(config))
|
/ci |
|
/ci |
|
/ci |
|
/ci |
|
/ci |
|
/ci |
nreese
changed the title
nreese
added
Team:Visualizations
|
Pinging @elastic/kibana-visualizations (Team:Visualizations) |
| createBaseVisualization: <TVisParams extends VisParams>( | ||
| config: VisTypeDefinition<TVisParams> | ||
| ): void => { | ||
| const vis = new BaseVisType(config); | ||
| this.registerVisualization(vis); | ||
| this.registerVisualization(config.name, async () => new BaseVisType(config)); | ||
| }, |
There was a problem hiding this comment.
@nreese this comment seems legit, in the registerVisualization function you are calling:
this.types[name] = async () => {
const config = await getVisDefinition();
return new BaseVisType(config);
};
so basically new BaseVisType(new BaseVisType(config))
| getIconForSavedObject: () => { | ||
| return 'visualizeApp'; |
There was a problem hiding this comment.
If you change this, in the Dashboard Add from library you will lose the chart icons.
Not a big deal for the agg-base visualize charts, but Vega will be affected to.
Before:
After:
There was a problem hiding this comment.
If you change this, in the Dashboard Add from library you will lose the chart icons.
Not a big deal for the agg-base visualize charts, but Vega will be affected to.
I think this is ok and even beneficial in the long run. There are plans to create a vega embeddable in the near future for "as code". This change will make it clear when a vega library item is vega embeddable or legacy vega vis.
There was a problem hiding this comment.
I see but for now I don't think this is a valid change, it will create confusion until we don't have the vega embeddable is ready. We should not release this change until Vega is not clearly distinguishable from the rest.
There was a problem hiding this comment.
You can just hardcode a check for the visState.type and return the Vega icon if vega and visualizeApp otherwise. We don't really need the registry for that. We can then clean this up with the vega embeddable
| private getAll = async () => { | ||
| return await asyncMap(Object.values(this.types), async (getVisType) => { | ||
| return await getVisType(); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
nit: The asyncMap propagates the first rejection, so if a dynamic import fails everything is rejected (this differs a bit from the previous logic if I understood it correcly)
There was a problem hiding this comment.
How would you like this resolved? Promise.all?
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsasync chunk count
History
|
3 participants
PR updates visType registry:
createBaseVisualizationAsyncmethod to allow plugins to keep visType out of page load.PR then updates vega and maps plugins to demonstrate async registry and verify page load reductions.