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

TypeScript cleanup in visualizations plugin #78428

Merged
merged 2 commits into from
Sep 28, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,15 @@

import { i18n } from '@kbn/i18n';

import { BaseVisTypeOptions } from 'src/plugins/visualizations/public';
import { createInputControlVisController } from './vis_controller';
import { getControlsTab } from './components/editor/controls_tab';
import { OptionsTab } from './components/editor/options_tab';
import { InputControlVisDependencies } from './plugin';

export function createInputControlVisTypeDefinition(deps: InputControlVisDependencies) {
export function createInputControlVisTypeDefinition(
deps: InputControlVisDependencies
): BaseVisTypeOptions {
const InputControlVisController = createInputControlVisController(deps);
const ControlsTab = getControlsTab(deps);

Expand Down
12 changes: 6 additions & 6 deletions src/plugins/input_control_vis/public/vis_controller.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,12 @@ import { RangeControl } from './control/range_control_factory';
import { ListControl } from './control/list_control_factory';
import { InputControlVisDependencies } from './plugin';
import { FilterManager, Filter } from '../../data/public';
import { VisParams, Vis } from '../../visualizations/public';
import { VisParams, ExprVis } from '../../visualizations/public';

export const createInputControlVisController = (deps: InputControlVisDependencies) => {
return class InputControlVisController {
private I18nContext?: I18nStart['Context'];
private isLoaded = false;
private _isLoaded = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ Since the type is propertly passed through now, this is colling with Visualizatins isLoaded() function, so I rename this for now.


controls: Array<RangeControl | ListControl>;
queryBarUpdateHandler: () => void;
Expand All @@ -45,7 +45,7 @@ export const createInputControlVisController = (deps: InputControlVisDependencie
timeFilterSubscription: Subscription;
visParams?: VisParams;

constructor(public el: Element, public vis: Vis) {
constructor(public el: Element, public vis: ExprVis) {
this.controls = [];

this.queryBarUpdateHandler = this.updateControlsFromKbn.bind(this);
Expand All @@ -58,7 +58,7 @@ export const createInputControlVisController = (deps: InputControlVisDependencie
.getTimeUpdate$()
.subscribe(() => {
if (this.visParams?.useTimeFilter) {
this.isLoaded = false;
this._isLoaded = false;
}
});
}
Expand All @@ -68,11 +68,11 @@ export const createInputControlVisController = (deps: InputControlVisDependencie
const [{ i18n }] = await deps.core.getStartServices();
this.I18nContext = i18n.Context;
}
if (!this.isLoaded || !isEqual(visParams, this.visParams)) {
if (!this._isLoaded || !isEqual(visParams, this.visParams)) {
this.visParams = visParams;
this.controls = [];
this.controls = await this.initControls();
this.isLoaded = true;
this._isLoaded = true;
}
this.drawVis();
}
Expand Down
3 changes: 2 additions & 1 deletion src/plugins/vis_type_metric/public/metric_vis_type.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,14 @@
*/

import { i18n } from '@kbn/i18n';
import { BaseVisTypeOptions } from 'src/plugins/visualizations/public';
import { MetricVisOptions } from './components/metric_vis_options';
import { ColorSchemas, colorSchemas, ColorModes } from '../../charts/public';
import { AggGroupNames } from '../../data/public';
import { Schemas } from '../../vis_default_editor/public';
import { toExpressionAst } from './to_ast';

export const createMetricVisTypeDefinition = () => ({
export const createMetricVisTypeDefinition = (): BaseVisTypeOptions => ({
name: 'metric',
title: i18n.translate('visTypeMetric.metricTitle', { defaultMessage: 'Metric' }),
icon: 'visMetric',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ describe('Table Vis - Controller', () => {
function getRangeVis(params?: object) {
return ({
type: tableVisTypeDefinition,
params: Object.assign({}, tableVisTypeDefinition.visConfig.defaults, params),
params: Object.assign({}, tableVisTypeDefinition.visConfig?.defaults, params),
data: {
aggs: createAggConfigs(stubIndexPattern, [
{ type: 'count', schema: 'metric' },
Expand Down
8 changes: 5 additions & 3 deletions src/plugins/vis_type_table/public/table_vis_type.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,19 @@ import { CoreSetup, PluginInitializerContext } from 'kibana/public';
import { i18n } from '@kbn/i18n';
import { AggGroupNames } from '../../data/public';
import { Schemas } from '../../vis_default_editor/public';
import { Vis } from '../../visualizations/public';
import { BaseVisTypeOptions, Vis } from '../../visualizations/public';
import { tableVisResponseHandler } from './table_vis_response_handler';
// @ts-ignore
import tableVisTemplate from './table_vis.html';
import { TableOptions } from './components/table_vis_options_lazy';
import { getTableVisualizationControllerClass } from './vis_controller';
import { VIS_EVENT_TO_TRIGGER } from '../../../plugins/visualizations/public';

export function getTableVisTypeDefinition(core: CoreSetup, context: PluginInitializerContext) {
export function getTableVisTypeDefinition(
core: CoreSetup,
context: PluginInitializerContext
): BaseVisTypeOptions {
return {
type: 'table',
name: 'table',
title: i18n.translate('visTypeTable.tableVisTitle', {
defaultMessage: 'Data Table',
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/vis_type_table/public/vis_controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export function getTableVisualizationControllerClass(
}
}

async render(esResponse: object, visParams: VisParams) {
async render(esResponse: object, visParams: VisParams): Promise<void> {
getKibanaLegacy().loadFontAwesome();
await this.initLocalAngular();

Expand Down
2 changes: 1 addition & 1 deletion src/plugins/vis_type_vislib/public/vis_controller.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ export const createVislibVisController = (deps: VisTypeVislibDependencies) => {
this.container.appendChild(this.legendEl);
}

render(esResponse: any, visParams: VisParams) {
render(esResponse: any, visParams: VisParams): Promise<void> {
if (this.vislibVis) {
this.destroy();
}
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/visualizations/public/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export { getSchemas as getVisSchemas } from './legacy/build_pipeline';

/** @public types */
export { VisualizationsSetup, VisualizationsStart };
export { VisTypeAlias, VisType } from './vis_types';
export { VisTypeAlias, VisType, BaseVisTypeOptions, ReactVisTypeOptions } from './vis_types';
export { VisParams, SerializedVis, SerializedVisData, VisData } from './vis';
export type VisualizeEmbeddableFactoryContract = PublicContract<VisualizeEmbeddableFactory>;
export type VisualizeEmbeddableContract = PublicContract<VisualizeEmbeddable>;
Expand Down
9 changes: 7 additions & 2 deletions src/plugins/visualizations/public/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,15 @@
* under the License.
*/

import { ExpressionAstExpression } from 'src/plugins/expressions';
import { SavedObject } from '../../../plugins/saved_objects/public';
import {
AggConfigOptions,
SearchSourceFields,
TimefilterContract,
} from '../../../plugins/data/public';
import { SerializedVis, Vis, VisParams } from './vis';
import { ExprVis } from './expressions/vis';

export { Vis, SerializedVis, VisParams };

Expand All @@ -35,7 +37,7 @@ export interface VisualizationController {

export type VisualizationControllerConstructor = new (
el: HTMLElement,
vis: Vis
vis: ExprVis
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ It seems there is currently two very similar Vis classes around ExprVis and Vis and they've got mixed up in a couple of places. ExprVis is the instance used in the renderer when intatiating the visualizations, while Vis is the one used by the editor instantiated at a very high level.

) => VisualizationController;

export interface SavedVisState {
Expand Down Expand Up @@ -71,4 +73,7 @@ export interface VisToExpressionAstParams {
abortSignal?: AbortSignal;
}

export type VisToExpressionAst = (vis: Vis, params: VisToExpressionAstParams) => string;
export type VisToExpressionAst = (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ This was completely wrongly typed :-) This function was never supposed to return a string. It's used in build_pipeline and the result is passed in a function expecting an ExpressionAst (also all implementations currently return that).

vis: Vis,
params: VisToExpressionAstParams
) => ExpressionAstExpression;
16 changes: 13 additions & 3 deletions src/plugins/visualizations/public/vis_types/base_vis_type.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { VisToExpressionAst, VisualizationControllerConstructor } from '../types
import { TriggerContextMapping } from '../../../ui_actions/public';
import { Adapters } from '../../../inspector/public';

export interface BaseVisTypeOptions {
interface CommonBaseVisTypeOptions {
name: string;
title: string;
description?: string;
Expand All @@ -31,7 +31,6 @@ export interface BaseVisTypeOptions {
image?: string;
stage?: 'experimental' | 'beta' | 'production';
options?: Record<string, any>;
visualization: VisualizationControllerConstructor | undefined;
visConfig?: Record<string, any>;
editor?: any;
editorConfig?: Record<string, any>;
Expand All @@ -42,9 +41,20 @@ export interface BaseVisTypeOptions {
setup?: unknown;
useCustomNoDataScreen?: boolean;
inspectorAdapters?: Adapters | (() => Adapters);
toExpressionAst?: VisToExpressionAst;
}

interface ExpressionBaseVisTypeOptions extends CommonBaseVisTypeOptions {
toExpressionAst: VisToExpressionAst;
visualization?: undefined;
}

interface VisualizationBaseVisTypeOptions extends CommonBaseVisTypeOptions {
toExpressionAst?: undefined;
visualization: VisualizationControllerConstructor | undefined;
}

export type BaseVisTypeOptions = ExpressionBaseVisTypeOptions | VisualizationBaseVisTypeOptions;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ Properly splitted this up into two types. It either needs to have a visualization or a toExpressionAst, but not both. Technically this COULD still be used that way (since the toExpressionAst could return an expression containing the visualization function, that would then go and look for visualization), but we're not using it this way, since toExpressionAst is used for the functions using their own renderer, so I rather wanted this state to reflect the refactoring we want to end up with, not what technically theoretically would be possible.


export class BaseVisType {
name: string;
title: string;
Expand Down
2 changes: 2 additions & 0 deletions src/plugins/visualizations/public/vis_types/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,5 @@
*/

export * from './types_service';
export type { BaseVisTypeOptions } from './base_vis_type';
export type { ReactVisTypeOptions } from './react_vis_type';
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,12 @@

import React from 'react';
import { render, unmountComponentAtNode } from 'react-dom';
import { Vis, VisualizationController } from '../types';
import { VisualizationController } from '../types';
import { getI18n, getUISettings } from '../services';
import { ExprVis } from '../expressions/vis';

export class ReactVisController implements VisualizationController {
private el: HTMLElement;
private vis: Vis;

constructor(element: HTMLElement, vis: Vis) {
this.el = element;
this.vis = vis;
}
constructor(private element: HTMLElement, private vis: ExprVis) {}

public render(visData: any, visParams: any): Promise<void> {
const I18nContext = getI18n().Context;
Expand All @@ -51,12 +46,12 @@ export class ReactVisController implements VisualizationController {
renderComplete={resolve}
/>
</I18nContext>,
this.el
this.element
);
});
}

public destroy() {
unmountComponentAtNode(this.el);
unmountComponentAtNode(this.element);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,14 @@
import { BaseVisType, BaseVisTypeOptions } from './base_vis_type';
import { ReactVisController } from './react_vis_controller';

export type ReactVisTypeOptions = Omit<BaseVisTypeOptions, 'visualization' | 'toExpressionAst'>;

/**
* This class should only be used for visualizations not using the `toExpressionAst` with a custom renderer.
* If you implement a custom renderer you should just mount a react component inside this.
*/
export class ReactVisType extends BaseVisType {
constructor(opts: Omit<BaseVisTypeOptions, 'visualization'>) {
constructor(opts: ReactVisTypeOptions) {
super({
...opts,
visualization: ReactVisController,
Expand Down
14 changes: 6 additions & 8 deletions src/plugins/visualizations/public/vis_types/types_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,8 @@

import { IconType } from '@elastic/eui';
import { visTypeAliasRegistry, VisTypeAlias } from './vis_type_alias_registry';
// @ts-ignore
import { BaseVisType } from './base_vis_type';
// @ts-ignore
import { ReactVisType } from './react_vis_type';
import { BaseVisType, BaseVisTypeOptions } from './base_vis_type';
import { ReactVisType, ReactVisTypeOptions } from './react_vis_type';
import { TriggerContextMapping } from '../../../ui_actions/public';

export interface VisType {
Expand Down Expand Up @@ -71,17 +69,17 @@ export class TypesService {
return {
/**
* registers a visualization type
* @param {VisType} config - visualization type definition
* @param config - visualization type definition
*/
createBaseVisualization: (config: any) => {
createBaseVisualization: (config: BaseVisTypeOptions): void => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ This is the most important part of this PR, since it finally puts the proper typings for the visualization registry into the public API we expose.

const vis = new BaseVisType(config);
registerVisualization(() => vis);
},
/**
* registers a visualization which uses react for rendering
* @param {VisType} config - visualization type definition
* @param config - visualization type definition
*/
createReactVisualization: (config: any) => {
createReactVisualization: (config: ReactVisTypeOptions): void => {
const vis = new ReactVisType(config);
registerVisualization(() => vis);
},
Expand Down