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

[lens] Editor frame initializes datasources and visualizations #36060

Merged
merged 6 commits into from
May 9, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
4 changes: 4 additions & 0 deletions x-pack/plugins/lens/public/app_plugin/plugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import React from 'react';
import { editorFrameSetup, editorFrameStop } from '../editor_frame_plugin';
import { indexPatternDatasourceSetup, indexPatternDatasourceStop } from '../indexpattern_plugin';
import { xyVisualizationSetup, xyVisualizationStop } from '../xy_visualization_plugin';
import { App } from './app';

export class AppPlugin {
Expand All @@ -16,15 +17,18 @@ export class AppPlugin {
// TODO: These plugins should not be called from the top level, but since this is the
// entry point to the app we have no choice until the new platform is ready
const indexPattern = indexPatternDatasourceSetup();
const xyVisualization = xyVisualizationSetup();
const editorFrame = editorFrameSetup();

editorFrame.registerDatasource('indexpattern', indexPattern);
editorFrame.registerVisualization('xy', xyVisualization);

return <App editorFrame={editorFrame} />;
}

stop() {
indexPatternDatasourceStop();
xyVisualizationStop();
editorFrameStop();
}
}
Expand Down
137 changes: 121 additions & 16 deletions x-pack/plugins/lens/public/editor_frame_plugin/editor_frame.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,34 +4,139 @@
* you may not use this file except in compliance with the Elastic License.
*/

import React from 'react';
import { Datasource, Visualization } from '../types';
import React, { useState, useEffect } from 'react';
import { Datasource, Visualization, DatasourcePublicAPI } from '../types';

interface EditorFrameProps {
datasources: { [key: string]: Datasource };
visualizations: { [key: string]: Visualization };

activeDatasource: string | null;
}

interface DatasourceState {
[key: string]: {
wylieconlon marked this conversation as resolved.
Show resolved Hide resolved
state: any;
setState: (state: any) => void;
wylieconlon marked this conversation as resolved.
Show resolved Hide resolved
api: DatasourcePublicAPI;
wylieconlon marked this conversation as resolved.
Show resolved Hide resolved
datasource: Datasource;
};
}

interface VisualizationState {
[key: string]: {
wylieconlon marked this conversation as resolved.
Show resolved Hide resolved
state: any;
setState: (state: any) => void;
wylieconlon marked this conversation as resolved.
Show resolved Hide resolved
visualization: Visualization;
wylieconlon marked this conversation as resolved.
Show resolved Hide resolved
};
}

export function EditorFrame(props: EditorFrameProps) {
const keys = Object.keys(props.datasources);
const dsKeys = Object.keys(props.datasources);
const vKeys = Object.keys(props.visualizations);

const [datasourceState, setDatasourceState] = useState({} as DatasourceState);
wylieconlon marked this conversation as resolved.
Show resolved Hide resolved
const [visualizationState, setVisualizationState] = useState({} as VisualizationState);

// Initialize all datasources and load their public APIs
useEffect(
Copy link
Contributor

@flash1293 flash1293 May 6, 2019

Choose a reason for hiding this comment

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

Why initializing all datasources and visualizations? Is this for debugging purposes? If yes I would vote for adding dropdowns for switching between them because it's much closer to what this plugin will do in a finished product. IMO we should only initialize the initial datasource and show a spinner during that period (which means there has to be some indicator in the state that datasource initialization is currently in progress) and initialize the new datasource on switch

() => {
Promise.all(
dsKeys.map(key => {
return props.datasources[key].initialize();
})
).then(allStates => {
const newState: { [key: string]: any } = {};
allStates.forEach((state, index) => {
const key = dsKeys[index];
const datasource = props.datasources[key];

const setState = (s: any) => {
setDatasourceState({
...datasourceState,
[key]: s,
});
};
newState[key] = {
datasource,
api: datasource.getPublicAPI(state, setState),
state,
setState,
};
});
setDatasourceState(newState);
});
},
[dsKeys.length]
);

// Initialize all visualizations
useEffect(
() => {
Promise.all(
vKeys.map(key => {
return props.visualizations[key].initialize();
})
).then(allStates => {
const newState: { [key: string]: any } = {};
allStates.forEach((state, index) => {
const key = vKeys[index];
const visualization = props.visualizations[key];

const setState = (s: any) => {
setVisualizationState({
...visualizationState,
[key]: s,
});
};
newState[key] = {
visualization,
state,
setState,
};
});
setVisualizationState(newState);
});
},
[vKeys.length]
);

return (
<div>
<h2>Editor Frame</h2>

{keys.map(key => (
<div
key={key}
ref={domElement => {
if (domElement) {
props.datasources[key].renderDataPanel(domElement, {
state: {},
setState: () => {},
});
}
}}
/>
))}
{Object.keys(datasourceState).length &&
dsKeys.map(key => (
<div
Copy link
Contributor

@flash1293 flash1293 May 6, 2019

Choose a reason for hiding this comment

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

This way of rendering agnostic components won't update them on state changes. Since we will have the same problem for rendering dimension editors, we should create a general purpose component which abstracts that away. Don't bother in this PR though, I'm going to implement this in a separate one.

key={key}
ref={domElement => {
if (domElement) {
datasourceState[key].datasource.renderDataPanel(domElement, {
state: datasourceState[key].state,
setState: datasourceState[key].setState,
});
}
}}
/>
))}

{Object.keys(datasourceState).length &&
Object.keys(visualizationState).length &&
props.activeDatasource &&
vKeys.map(key => (
<div
key={key}
ref={domElement => {
if (domElement) {
props.visualizations[key].renderConfigPanel(domElement, {
datasource: datasourceState[props.activeDatasource!].api,
state: visualizationState[key].state,
setState: visualizationState[key].setState,
});
}
}}
/>
))}
</div>
);
}
27 changes: 22 additions & 5 deletions x-pack/plugins/lens/public/editor_frame_plugin/plugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,21 @@

import React from 'react';
import { render, unmountComponentAtNode } from 'react-dom';
import { Datasource, Visualization, EditorFrameSetup } from '../types';
import { Datasource, Visualization, EditorFrameSetup, DatasourcePublicAPI } from '../types';

import { EditorFrame } from './editor_frame';

class EditorFramePlugin {
constructor() {}

private datasources: { [key: string]: Datasource } = {};
private visualizations: { [key: string]: Visualization } = {};
private datasources: {
[key: string]: Datasource;
} = {};
private visualizations: {
[key: string]: Visualization;
} = {};

private activeDatasource: string | null = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to hold information about the active data source and the mounted element in the plugin? This restricts us in only showing / managing a single editor frame at a time. We might want to have multiple "editor frame instances" later, especially for the embedding use case.

My proposed change is something like this:

      createInstance: (domElement: Element) => {
        return {
            render: () => render(
              <EditorFrame
                datasources={this.datasources}
                visualizations={this.visualizations}
                initialDatasource={Object.keys(this.datasources)[0]}
              />,
              domElement
            ),
          unmount() {
            unmountComponentAtNode(domElement); 
          }
        };
      },

Copy link
Contributor

Choose a reason for hiding this comment

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

Even if we don't do this, the state about the active datasource should be managed inside the editor frame component, not the plugin - the editor frame will be able to change the datasource in the future which would require a callback like onChangeDatasource. The plugin doesn't need to know which datasource is currently used.
The concrete proposal would be to pass in an initialDatasource


private element: Element | null = null;

Expand All @@ -23,19 +29,30 @@ class EditorFramePlugin {
render: domElement => {
this.element = domElement;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't follow the createInstance proposal form above (and I think we should), we have to check whether render was already called on another element and clean up there:

if (this.element !== null && this.element !== domElement) {
  unmountComponentAtNode(this.element);
}

render(
<EditorFrame datasources={this.datasources} visualizations={this.visualizations} />,
<EditorFrame
datasources={this.datasources}
visualizations={this.visualizations}
activeDatasource={this.activeDatasource}
/>,
domElement
);
},
registerDatasource: (name, datasource) => {
registerDatasource: async (name, datasource) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary async

// casting it to an unknown datasource. This doesn't introduce runtime errors
// because each type T is always also an unknown, but typescript won't do it
// on it's own because we are loosing type information here.
// So it's basically explicitly saying "I'm dropping the information about type T here
// because this information isn't useful to me." but without using any which can leak
// const state = await datasource.initialize();
Copy link
Contributor

Choose a reason for hiding this comment

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

Commented out code


this.datasources[name] = datasource as Datasource<unknown>;

if (!this.activeDatasource) {
this.activeDatasource = name;
}
},
registerVisualization: (name, visualization) => {
// this.visualizations[name] = visualization as Visualization<unknown>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Commented out code

this.visualizations[name] = visualization as Visualization<unknown>;
},
};
Expand Down
5 changes: 4 additions & 1 deletion x-pack/plugins/lens/public/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,10 @@ export interface VisualizationSuggestion<T = unknown> {
}

export interface Visualization<T = unknown> {
renderConfigPanel: (props: VisualizationProps<T>) => void;
// For initializing from saved object
initialize: (state?: T) => Promise<T>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this async? Is the visualization allowed to query stuff during init? I was under the impression the visualization doesn't have to be initialized because it doesn't need to trigger any side effects

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Visualizations can have internal state just like datasources, and this is mostly for rehydrating from persisted state


renderConfigPanel: (domElement: Element, props: VisualizationProps<T>) => void;

toExpression: (state: T, datasource: DatasourcePublicAPI) => string;

Expand Down
7 changes: 7 additions & 0 deletions x-pack/plugins/lens/public/xy_visualization_plugin/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

export * from './plugin';
22 changes: 22 additions & 0 deletions x-pack/plugins/lens/public/xy_visualization_plugin/plugin.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { xyVisualization } from './xy_visualization';

class XyVisualizationPlugin {
constructor() {}

setup() {
return xyVisualization;
}

stop() {}
}

const plugin = new XyVisualizationPlugin();

export const xyVisualizationSetup = () => plugin.setup();
export const xyVisualizationStop = () => plugin.stop();
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import React from 'react';
import { render } from 'react-dom';
import { Visualization, DimensionRole } from '../types';

export interface XyVisualizationState {
roles: DimensionRole[];
}

export const xyVisualization: Visualization<XyVisualizationState> = {
async initialize() {
return {
roles: [],
};
},

renderConfigPanel: (domElement, props) => {
render(<div>XY Visualization</div>, domElement);
},

getSuggestions: options => [],

getMappingOfTableToRoles: (state, datasource) => [],

toExpression: state => '',
};