-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Expression executor service #36885
Expression executor service #36885
Changes from 6 commits
3c74514
af85958
935b008
0d99f0a
37a6d55
c211b74
802b6f1
ff1d950
0eeceb2
f1a59e0
a2ac4f6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,3 +18,5 @@ | |
*/ | ||
|
||
export { Registry } from './lib/registry'; | ||
|
||
export { fromExpression, Ast } from './lib/ast'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
/* | ||
* Licensed to Elasticsearch B.V. under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch B.V. licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
export type Ast = unknown; | ||
|
||
export declare function fromExpression(expression: string): Ast; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,180 @@ | ||
/* | ||
* Licensed to Elasticsearch B.V. under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch B.V. licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
import { fromExpression, Ast } from '@kbn/interpreter/common'; | ||
|
||
import { | ||
ExpressionExecutorService, | ||
RenderFunctionsRegistry, | ||
RenderFunction, | ||
Interpreter, | ||
ExpressionExecutorSetupPlugins, | ||
Result, | ||
ExpressionExecutorSetup, | ||
} from './expression_executor_service'; | ||
import { mount } from 'enzyme'; | ||
import React from 'react'; | ||
|
||
const waitForInterpreterRun = async () => { | ||
// Wait for two ticks with empty callback queues | ||
// This makes sure the runFn promise and actual interpretAst | ||
// promise have been resolved and processed | ||
await new Promise(resolve => setTimeout(resolve)); | ||
await new Promise(resolve => setTimeout(resolve)); | ||
}; | ||
|
||
describe('expression_executor_service', () => { | ||
let interpreterMock: jest.Mocked<Interpreter>; | ||
let renderFunctionMock: jest.Mocked<RenderFunction>; | ||
let setupPluginsMock: ExpressionExecutorSetupPlugins; | ||
const expressionResult: Result = { type: 'render', as: 'abc', value: {} }; | ||
|
||
let api: ExpressionExecutorSetup; | ||
let testExpression: string; | ||
let testAst: Ast; | ||
|
||
beforeEach(() => { | ||
interpreterMock = { interpretAst: jest.fn(_ => Promise.resolve(expressionResult)) }; | ||
renderFunctionMock = ({ | ||
render: jest.fn(), | ||
} as unknown) as jest.Mocked<RenderFunction>; | ||
setupPluginsMock = { | ||
interpreter: { | ||
getInterpreter: () => Promise.resolve({ interpreter: interpreterMock }), | ||
renderersRegistry: ({ | ||
get: () => renderFunctionMock, | ||
} as unknown) as RenderFunctionsRegistry, | ||
}, | ||
}; | ||
api = new ExpressionExecutorService().setup(null, setupPluginsMock); | ||
testExpression = 'test | expression'; | ||
testAst = fromExpression(testExpression); | ||
}); | ||
|
||
it('should return run function', () => { | ||
expect(typeof api.run).toBe('function'); | ||
}); | ||
|
||
it('should call the interpreter with parsed expression', async () => { | ||
await api.run(testExpression, document.createElement('div')); | ||
expect(interpreterMock.interpretAst).toHaveBeenCalledWith( | ||
testAst, | ||
expect.anything(), | ||
expect.anything() | ||
); | ||
}); | ||
|
||
it('should call the interpreter with passed in ast', async () => { | ||
await api.run(testAst, document.createElement('div')); | ||
expect(interpreterMock.interpretAst).toHaveBeenCalledWith( | ||
testAst, | ||
expect.anything(), | ||
expect.anything() | ||
); | ||
}); | ||
|
||
it('should call the render function with the result and element', async () => { | ||
const element = document.createElement('div'); | ||
|
||
await api.run(testAst, element); | ||
expect(renderFunctionMock.render).toHaveBeenCalledWith( | ||
element, | ||
expressionResult.value, | ||
expect.anything() | ||
); | ||
expect(interpreterMock.interpretAst).toHaveBeenCalledWith( | ||
testAst, | ||
expect.anything(), | ||
expect.anything() | ||
); | ||
}); | ||
|
||
it('should call interpreter and render function when called through react component', async () => { | ||
const ExpressionRenderer = api.ExpressionRenderer; | ||
|
||
mount(<ExpressionRenderer expression={testExpression} />); | ||
|
||
await waitForInterpreterRun(); | ||
|
||
expect(renderFunctionMock.render).toHaveBeenCalledWith( | ||
expect.any(Element), | ||
expressionResult.value, | ||
expect.anything() | ||
); | ||
expect(interpreterMock.interpretAst).toHaveBeenCalledWith( | ||
testAst, | ||
expect.anything(), | ||
expect.anything() | ||
); | ||
}); | ||
|
||
it('should call interpreter and render function again if expression changes', async () => { | ||
const ExpressionRenderer = api.ExpressionRenderer; | ||
|
||
const instance = mount(<ExpressionRenderer expression={testExpression} />); | ||
|
||
await waitForInterpreterRun(); | ||
|
||
expect(renderFunctionMock.render).toHaveBeenCalledWith( | ||
expect.any(Element), | ||
expressionResult.value, | ||
expect.anything() | ||
); | ||
expect(interpreterMock.interpretAst).toHaveBeenCalledWith( | ||
testAst, | ||
expect.anything(), | ||
expect.anything() | ||
); | ||
|
||
instance.setProps({ expression: 'supertest | expression ' }); | ||
|
||
await waitForInterpreterRun(); | ||
|
||
expect(renderFunctionMock.render).toHaveBeenCalledTimes(2); | ||
expect(interpreterMock.interpretAst).toHaveBeenCalledTimes(2); | ||
}); | ||
|
||
it('should not call interpreter and render function again if expression does not change', async () => { | ||
const ast = fromExpression(testExpression); | ||
|
||
const ExpressionRenderer = api.ExpressionRenderer; | ||
|
||
const instance = mount(<ExpressionRenderer expression={testExpression} />); | ||
|
||
await waitForInterpreterRun(); | ||
|
||
expect(renderFunctionMock.render).toHaveBeenCalledWith( | ||
expect.any(Element), | ||
expressionResult.value, | ||
expect.anything() | ||
); | ||
expect(interpreterMock.interpretAst).toHaveBeenCalledWith( | ||
ast, | ||
expect.anything(), | ||
expect.anything() | ||
); | ||
|
||
instance.update(); | ||
|
||
await waitForInterpreterRun(); | ||
|
||
expect(renderFunctionMock.render).toHaveBeenCalledTimes(1); | ||
expect(interpreterMock.interpretAst).toHaveBeenCalledTimes(1); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,177 @@ | ||
/* | ||
* Licensed to Elasticsearch B.V. under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch B.V. licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
import { CoreSetup } from 'kibana/public'; | ||
import { useRef, useEffect } from 'react'; | ||
import React from 'react'; | ||
|
||
import { fromExpression, Ast } from '@kbn/interpreter/common'; | ||
|
||
// this type import and the types below them should be switched to the types of | ||
// the interpreter plugin itself once they are ready | ||
import { RunPipelineHandlers } from 'ui/visualize/loader/pipeline_helpers/run_pipeline'; | ||
import { Registry } from '@kbn/interpreter/common'; | ||
import { RequestAdapter, DataAdapter } from 'ui/inspector/adapters'; | ||
|
||
type Context = object; | ||
type Handlers = RunPipelineHandlers; | ||
export interface Result { | ||
type: string; | ||
as?: string; | ||
value?: unknown; | ||
} | ||
|
||
interface RenderHandlers { | ||
done: () => void; | ||
onDestroy: (fn: () => void) => void; | ||
} | ||
|
||
export interface RenderFunction { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these just exported for the purposes of testing, or is the intent to allow them to be imported by downstream consumers? If we want them to be imported downstream, the convention we & core have been using is to re-export the types from the top-level plugin definition, so that people don't need to know the path to the directory to import from. e.g. import { data } from 'plugins/data';
// we want to avoid this as it makes your directory structure part of your public contract
import { RenderFunction } from 'plugins/data/expression_executor';
// so this is how we've been solving the issue so far
import { data, RenderFunction } from 'plugins/data';
// (the platform team has also discussed eventually namespacing any types by service if it becomes confusing. e.g. Expressions.RenderFunction) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense, thanks. These types are only exported for the tests, I changed my code to re-export public stuff in index |
||
name: string; | ||
displayName: string; | ||
help: string; | ||
validate: () => void; | ||
reuseDomNode: boolean; | ||
render: (domNode: Element, data: unknown, handlers: RenderHandlers) => void; | ||
} | ||
|
||
export type RenderFunctionsRegistry = Registry<unknown, RenderFunction>; | ||
|
||
export interface Interpreter { | ||
interpretAst(ast: Ast, context: Context, handlers: Handlers): Promise<Result>; | ||
} | ||
|
||
export interface ExpressionExecutorSetupPlugins { | ||
interpreter: { | ||
renderersRegistry: RenderFunctionsRegistry; | ||
getInterpreter: () => Promise<{ interpreter: Interpreter }>; | ||
}; | ||
} | ||
|
||
async function runFn( | ||
expressionOrAst: string | Ast, | ||
element: Element, | ||
renderersRegistry: RenderFunctionsRegistry, | ||
interpreter: Interpreter | ||
) { | ||
const ast = | ||
typeof expressionOrAst === 'string' ? fromExpression(expressionOrAst) : expressionOrAst; | ||
const response = await interpreter.interpretAst( | ||
ast, | ||
{ type: 'null' }, | ||
{ | ||
getInitialContext: () => ({}), | ||
inspectorAdapters: { | ||
// TODO connect real adapters | ||
requests: new RequestAdapter(), | ||
data: new DataAdapter(), | ||
}, | ||
} | ||
); | ||
|
||
if (response.type === 'render' && response.as) { | ||
renderersRegistry.get(response.as).render(element, response.value, { | ||
onDestroy: fn => { | ||
// TODO implement | ||
}, | ||
done: () => { | ||
// TODO implement | ||
}, | ||
}); | ||
} else { | ||
// eslint-disable-next-line no-console | ||
console.log('Unexpected result of expression', response); | ||
} | ||
|
||
return response; | ||
} | ||
|
||
/** | ||
* Expression Executor Service | ||
* @internal | ||
*/ | ||
export class ExpressionExecutorService { | ||
private interpreterInstance: Interpreter | null = null; | ||
|
||
// TODO core won't ever be null once this is switched to the new platform | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was a misunderstanding from my side, sorry. I thought the individual services were planned to become plugins in the future because of the very similar call signature. I removed the core parameter completely, this also should make clear the service isn't depending on it |
||
public setup(_core: CoreSetup | null, plugins: ExpressionExecutorSetupPlugins) { | ||
/** | ||
* **experimential** This API is experimential and might be removed in the future | ||
* without notice | ||
* | ||
* Executes the given expression string or ast and renders the result into the | ||
* given DOM element. | ||
* | ||
* | ||
* @param expressionOrAst | ||
* @param element | ||
*/ | ||
const run = async (expressionOrAst: string | Ast, element: Element) => { | ||
if (!this.interpreterInstance) { | ||
flash1293 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
this.interpreterInstance = (await plugins.interpreter.getInterpreter()).interpreter; | ||
} | ||
return await runFn( | ||
expressionOrAst, | ||
element, | ||
plugins.interpreter.renderersRegistry, | ||
this.interpreterInstance | ||
); | ||
}; | ||
return { | ||
run, | ||
/** | ||
* **experimential** This API is experimential and might be removed in the future | ||
* without notice | ||
* | ||
* Component which executes and renders the given expression in a div element. | ||
* The expression is re-executed on updating the props. | ||
* | ||
* This is a React bridge of the `run` method | ||
* @param props | ||
*/ | ||
ExpressionRenderer({ expression }: { expression: string }) { | ||
flash1293 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const mountpoint: React.MutableRefObject<null | HTMLDivElement> = useRef(null); | ||
|
||
useEffect( | ||
() => { | ||
if (mountpoint.current) { | ||
run(expression, mountpoint.current); | ||
} | ||
}, | ||
[expression, mountpoint.current] | ||
); | ||
|
||
return ( | ||
<div | ||
ref={el => { | ||
mountpoint.current = el; | ||
}} | ||
/> | ||
); | ||
}, | ||
}; | ||
} | ||
|
||
public stop() { | ||
// nothing to do here yet | ||
} | ||
} | ||
|
||
/** @public */ | ||
export type ExpressionExecutorSetup = ReturnType<ExpressionExecutorService['setup']>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
react shouldn't be part of the API, we can have react components wrapping the api but pls put in separate file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we don't do stateful components at all? Or is this just about the React import in the service itself?
The nice thing about delivering components like that is that they are usable directly (
<ExpressionRenderer expression="..." />
) instead of having to pass the stateful thing in (e.g.<ExpressionRenderer runFn={plugins.expressionExecutor.run} expression="..." />
)We can postpone this and I just remove the react component for now - there is already a pretty similar component in Lens which can handle the integration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah agreed with @ppisljar here -- I'd suggest providing a
run
function to just run the expression, and a in a separate file a "convenience" method for React, which usesrun
under the hood.Then both can be returned from
setup
as part of the public contract. This way people have the option of using the React wrapper or therun
method directly. Also some people might want to run an expression and get the output without having to use a renderer.