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] Expression handling #37876

Merged
merged 20 commits into from
Jun 5, 2019
Merged
Show file tree
Hide file tree
Changes from 16 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 @@ -27,6 +27,12 @@ export interface ExpressionRunnerOptions {
context?: object;
getInitialContext?: () => object;
element?: Element;
/**
* If an element is specified, but the response of the expression run can't be rendered
* because it isn't a valid response or the specified renderer isn't available,
* this callback is called with the given result.
*/
onRenderFailure?: (result: Result) => void;
}

export type ExpressionRunner = (
Expand All @@ -37,7 +43,10 @@ export type ExpressionRunner = (
export const createRunFn = (
renderersRegistry: RenderFunctionsRegistry,
interpreterPromise: Promise<Interpreter>
): ExpressionRunner => async (expressionOrAst, { element, context, getInitialContext }) => {
): ExpressionRunner => async (
expressionOrAst,
{ element, context, getInitialContext, onRenderFailure }
) => {
// TODO: make interpreter initialization synchronous to avoid this
const interpreter = await interpreterPromise;
const ast =
Expand All @@ -53,7 +62,7 @@ export const createRunFn = (
});

if (element) {
if (response.type === 'render' && response.as) {
if (response.type === 'render' && response.as && renderersRegistry.get(response.as) !== null) {
renderersRegistry.get(response.as).render(element, response.value, {
onDestroy: fn => {
// TODO implement
Expand All @@ -63,8 +72,9 @@ export const createRunFn = (
},
});
} else {
// eslint-disable-next-line no-console
console.log('Unexpected result of expression', response);
if (onRenderFailure) {
onRenderFailure(response);
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,26 +39,30 @@ const waitForInterpreterRun = async () => {
await new Promise(resolve => setTimeout(resolve));
};

const RENDERER_ID = 'mockId';

describe('expressions_service', () => {
let interpretAstMock: jest.Mocked<Interpreter>['interpretAst'];
let interpreterMock: jest.Mocked<Interpreter>;
let renderFunctionMock: jest.Mocked<RenderFunction>;
let setupPluginsMock: ExpressionsServiceDependencies;
const expressionResult: Result = { type: 'render', as: 'abc', value: {} };
const expressionResult: Result = { type: 'render', as: RENDERER_ID, value: {} };

let api: ExpressionsSetup;
let testExpression: string;
let testAst: Ast;

beforeEach(() => {
interpreterMock = { interpretAst: jest.fn(_ => Promise.resolve(expressionResult)) };
interpretAstMock = jest.fn(_ => Promise.resolve(expressionResult));
interpreterMock = { interpretAst: interpretAstMock };
renderFunctionMock = ({
render: jest.fn(),
} as unknown) as jest.Mocked<RenderFunction>;
setupPluginsMock = {
interpreter: {
getInterpreter: () => Promise.resolve({ interpreter: interpreterMock }),
renderersRegistry: ({
get: () => renderFunctionMock,
get: (id: string) => (id === RENDERER_ID ? renderFunctionMock : null),
} as unknown) as RenderFunctionsRegistry,
},
};
Expand Down Expand Up @@ -101,6 +105,44 @@ describe('expressions_service', () => {
);
});

it('should return the result of the interpreter run', async () => {
const response = await api.run(testAst, {});
expect(response).toBe(expressionResult);
});

it('should call on render failure if the response is not valid', async () => {
const errorResult = { type: 'error', error: {} };
interpretAstMock.mockReturnValue(Promise.resolve(errorResult));
const renderFailureSpy = jest.fn();
const response = await api.run(testAst, {
element: document.createElement('div'),
onRenderFailure: renderFailureSpy,
});
expect(renderFailureSpy).toHaveBeenCalledWith(errorResult);
expect(response).toBe(response);
});

it('should call on render failure if the renderer is not known', async () => {
const errorResult = { type: 'render', as: 'unknown_id' };
interpretAstMock.mockReturnValue(Promise.resolve(errorResult));
const renderFailureSpy = jest.fn();
const response = await api.run(testAst, {
element: document.createElement('div'),
onRenderFailure: renderFailureSpy,
});
expect(renderFailureSpy).toHaveBeenCalledWith(errorResult);
expect(response).toBe(response);
});

it('should not call on render failure if the runner does not render', async () => {
const errorResult = { type: 'error', error: {} };
interpretAstMock.mockReturnValue(Promise.resolve(errorResult));
const renderFailureSpy = jest.fn();
const response = await api.run(testAst, { onRenderFailure: renderFailureSpy });
expect(renderFailureSpy).not.toHaveBeenCalled();
expect(response).toBe(response);
});

it('should call the render function with the result and element', async () => {
const element = document.createElement('div');

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ export interface Result {
type: string;
as?: string;
value?: unknown;
error?: unknown;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it unknown? It seems it would be either a string or an Error object, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use unknown for stuff I'm not sure about the shape. It is an object in practice but as these types are only temporary anyway I didn't bother defining it because I don't rely on it anywhere.

}

interface RenderHandlers {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,47 @@ describe('editor_frame', () => {
expect.objectContaining({ state: initialState })
);
});

it('should render the resulting expression using the expression renderer', async () => {
const instance = mount(
<EditorFrame
visualizationMap={{
testVis: { ...mockVisualization, toExpression: () => 'vis' },
}}
datasourceMap={{
testDatasource: {
...mockDatasource,
toExpression: () => 'datasource',
},
}}
initialDatasourceId="testDatasource"
initialVisualizationId="testVis"
ExpressionRenderer={expressionRendererMock}
/>
);

await waitForPromises();

instance.update();

expect(instance.find(expressionRendererMock).prop('expression')).toMatchInlineSnapshot(`
Object {
"chain": Array [
Object {
"arguments": Object {},
"function": "datasource",
"type": "function",
},
Object {
"arguments": Object {},
"function": "vis",
"type": "function",
},
],
"type": "expression",
}
`);
});
});

describe('state update', () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* 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 { Ast, fromExpression } from '@kbn/interpreter/common';
import { Visualization, Datasource, DatasourcePublicAPI } from '../../types';

export function buildExpression(
visualization: Visualization,
visualizationState: unknown,
datasource: Datasource,
datasourceState: unknown,
datasourcePublicAPI: DatasourcePublicAPI
): Ast | undefined {
const datasourceExpression = datasource.toExpression(datasourceState);
const visualizationExpression = visualization.toExpression(
visualizationState,
datasourcePublicAPI
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the assumption that both datasource and visualization will always return a useful expression is the cause of the problem. I think we've seen that in Canvas, using sample data to replace a real data set is confusing to users. Visualizations without data might be able to render some kind of frame, but are otherwise meaningless. So here's a proposal:

  1. Allow datasources and visualizations to not provide an expression. Could use either null or "" (empty string) to represent this.
  2. If the datasource expression is not provided, I would also not render the visualization. This is mostly because of the suggestion system, where we might switch the visualization type based on the selected data.

Copy link
Contributor Author

@flash1293 flash1293 Jun 4, 2019

Choose a reason for hiding this comment

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

Makes sense, I implemented it like this. If one of the two doesn't provide an expression, nothing is rendered.

I went with null as "I don't want to render right now" because it's more explicit than an empty string.


try {
const parsedDatasourceExpression =
typeof datasourceExpression === 'string'
? fromExpression(datasourceExpression)
: datasourceExpression;
const parsedVisualizationExpression =
typeof visualizationExpression === 'string'
? fromExpression(visualizationExpression)
: visualizationExpression;
return {
type: 'expression',
chain: [...parsedDatasourceExpression.chain, ...parsedVisualizationExpression.chain],
};
} catch (_) {
return undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should do something with the error. If this fails, swallowing the error will make it difficult to diagnose. If we don't know what to do, I'd remove the try / catch and add a TODO comment. At least then, we get an explicit error which we can analyze in the console.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I moved the catch to the workspace panel and show the underlying error there.

Screenshot 2019-06-04 at 10 05 45

I don't like letting the exception bubble up and crash Kibana because errors in the expression are a nice error boundary. In most cases it is probably possible to recover from the error without losing state.

We probably wont keep the very prominent error message in the workspace panel for the final release, but for beta it's nice to debug easily.

}
}
Loading