expression service #42337

ppisljar · 2019-07-31T11:37:37Z

Summary

Implements basic expression service

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2019-07-31T11:37:39Z

Pinging @elastic/kibana-app-arch

elasticmachine · 2019-07-31T12:36:58Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

flash1293 · 2019-08-01T10:08:26Z

src/legacy/core_plugins/data/public/expressions/expression_renderer.tsx


 // Accept all options of the runner as props except for the
 // dom element which is provided by the component itself
-export type ExpressionRendererProps = Pick<


Removing that makes it impossible to pass in things like the context over the react interface. IExpressionLoaderParams should be part of the renderer props. They are already passed correctly to the loader: https://github.com/elastic/kibana/pull/42337/files#diff-e2b0614a60b75e9f05d4bec84efb896aR50

flash1293 · 2019-08-01T10:18:42Z

src/legacy/core_plugins/data/public/expressions/lib/execute.ts

+    this.abortController = new AbortController();
+    this.inspectorAdapters = this.getActiveInspectorAdapters();
+
+    const getInitialContext = () => ({


The only breaking change for the expression renderer react component is that it won't be possible to pass in a custom getInitialContext function anymore. Maybe I'm missing something, but is that limitation necessary?

The alternative would be adding getInitialContext to IExpressionLoaderParams instead of searchContext. Then there could also be initial contexts other than kibana_context. Not important for the usage in Lens though.

you can pass in the SearchContext (which will be returned by getInitialContext).
for now lets keep it like this, if we find a usecase for sending in something else we can extend this in the future.

flash1293 · 2019-08-01T10:21:30Z

src/legacy/core_plugins/data/public/expressions/expression_renderer.tsx

@@ -50,7 +47,8 @@ export const createRenderer = (run: ExpressionRunner): ExpressionRenderer => ({

  useEffect(() => {
    if (mountpoint.current) {
-      run(expression, { ...options, element: mountpoint.current }).catch(result => {
+      const handler = loader(mountpoint.current, expression, options);


The re-creates the loader every time a new expression is passed into this component, right? Shouldn't we hold the loader in a ref? Otherwise cancel isn't called correctly in this branch https://github.com/elastic/kibana/pull/42337/files#diff-1793959dde8c2d221081302bc14dd984R99 for the old loader.

lizozom · 2019-08-06T14:27:43Z

src/legacy/core_plugins/data/public/expressions/lib/execute.ts

+ */
+
+import { fromExpression } from '@kbn/interpreter/target/common';
+import { DataAdapter, RequestAdapter } from 'ui/inspector/adapters';


Please change the imports to the new plugin.
You will have to export DataAdapter, RequestAdapter from the top level of the inspector plugin.

I started here #42615, but closing this PR now.

elasticmachine · 2019-08-22T12:48:35Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

streamich

LGTM, subject to Joe's questions and green CI.

streamich · 2019-08-22T12:19:39Z

src/legacy/core_plugins/data/public/expressions/lib/render.ts

+import { Observable } from 'rxjs';
+import * as Rx from 'rxjs';
+import { share, first } from 'rxjs/operators';
+import { renderersRegistry } from '../../../../interpreter/public/registries';


FYI, I'm adding getRenderer method to Expressions service in a parallel PR.

streamich · 2019-08-22T12:22:19Z

src/legacy/core_plugins/data/public/expressions/lib/loader.ts

+    this.renderHandler = new ExpressionRenderHandler(element);
+    this.render$ = this.renderHandler.render$;
+    this.update$ = this.renderHandler.update$;
+    this.events$ = this.renderHandler.events$;


I'm not sure why set all these observables on this object, we can already access them through this.renderHandler.

renderHandler is private and not accessible to the outside

streamich · 2019-08-22T12:32:25Z

src/legacy/core_plugins/data/public/expressions/expression_renderer.tsx


 export type ExpressionRenderer = React.FC<ExpressionRendererProps>;

-export const createRenderer = (run: ExpressionRunner): ExpressionRenderer => ({
+export const createRenderer = (loader: IExpressionLoader): ExpressionRenderer => ({


IExpressionLoader is a type, not interface, but it has I prefix.

do we have a convention for naming types ? TExpressionLoader ?

streamich · 2019-08-22T12:50:37Z

src/legacy/core_plugins/data/public/expressions/lib/execute.ts

+
+import { fromExpression } from '@kbn/interpreter/target/common';
+import { DataAdapter, RequestAdapter } from '../../../../../ui/public/inspector/adapters';
+import { Adapters } from '../../../../../ui/public/inspector';


Like Liza mentioned, the above two could be like this:

import { DataAdapter, RequestAdapter, Adapters } from '../../../../../../plugins/inspector/public';

lukeelmers · 2019-08-29T22:25:14Z

src/legacy/core_plugins/data/public/expressions/lib/_types.ts

+ * under the License.
+ */
+
+import { Ast } from '@kbn/interpreter/common';


Right now this is just typed as unknown in the @kbn/interpreter package. We should instead use the ExpressionAST interface from src/plugins/data/common/expressions/types

lukeelmers · 2019-08-29T22:55:54Z

src/legacy/core_plugins/data/public/expressions/lib/render.ts

+      update: (params: any) => {
+        updateSubject.next(params);
+      },
+      event: (data: any) => {


Are the anys needed here, or are these types already provided by IInterpreterRenderHandlers?

lukeelmers · 2019-08-29T22:57:51Z

src/legacy/core_plugins/data/public/index.ts

@@ -28,7 +28,7 @@ export function plugin() {

 /** @public types */
 export type DataSetup = DataSetup;
-export { ExpressionRenderer, ExpressionRendererProps, ExpressionRunner } from './expressions';
+export { ExpressionRenderer, ExpressionRendererProps } from './expressions';


We should export DataStart here too

lukeelmers · 2019-08-29T23:15:55Z

src/legacy/core_plugins/data/public/expressions/lib/loader.ts

+    return data;
+  };
+
+  private async render(data: Data): Promise<RenderId> {


I might be missing something, but this is ultimately calling the renderer from the registry right (via the render handler?)

Are any renderers actually returning a Promise<RenderId> today? Otherwise we should perhaps be adding an ExpressionRenderer generic, similar to ExpressionFunction, so that this can actually be enforced.

hmm ... here we call the expression renderer handler, which will return a promise which will always resolve with the renderId. There internally we call renderer from the registry and pass it the handlers. Once rendering is done the done() handler should be called (and is nowadays i think for all renderers) which then resolves the promise.

elasticmachine · 2019-09-03T12:21:38Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-03T17:41:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

flash1293 · 2019-09-04T07:38:35Z

src/legacy/core_plugins/data/public/expressions/expression_renderer.tsx

  expression,
  onRenderFailure,
  ...options
 }: ExpressionRendererProps) => {
  const mountpoint: React.MutableRefObject<null | HTMLDivElement> = useRef(null);

+  const handlerRef = useRef((null as any) as ExpressionLoader);


nit: can be written as useRef<null | ExpressionLoader>(null); for better type safety.

flash1293 · 2019-09-04T07:40:39Z

src/legacy/core_plugins/data/public/expressions/expression_renderer.tsx

  /**
   * 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;
-};
+  onRenderFailure?: (result: IInterpreterResult) => void;


Could you add a className prop here that's passed to the mount div element?

flash1293 · 2019-09-04T07:42:09Z

src/legacy/core_plugins/data/public/expressions/expression_renderer.tsx

+      } else {
+        handlerRef.current.update(expression, options);
+      }
+      handlerRef.current.data$.toPromise().catch(result => {
        if (onRenderFailure) {
          onRenderFailure(result);
        }


This effect should also re-render if options.context changes

flash1293 · 2019-09-04T07:47:33Z

src/legacy/core_plugins/data/public/expressions/expressions_service.ts

+  }
+
+  public start({ inspector }: ExpressionsServiceStartDependencies) {
+    const ExpressionRenderer = createRenderer(loader);


Can we expose the renderer in the setup phase? Lens currently requires the expression renderer in the embeddable factory which should register its stuff in the setup phase.

EDIT: OK, I see why it is solved this way - the inspector is not ready in the setup phase. I guess we could work around it in Lens and pass the expression renderer to the embeddable factory instance in the start phase - slightly dirty but probably the best way. It's not super clean because it hides the fact that Lens embeddables can only be created after the start phase but it shouldn't cause problems in this particular instance and I can throw an explanatory error if it happens.

Still wondering whether there is a better way though.

yeah, also different plugins (will) register renderers, so in your setup lifecycle there is no guarantee that the functions or renderers exist in the registry yet.

adding options to deps

flash1293

Changes LGTM

elasticmachine · 2019-09-04T12:06:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

wylieconlon · 2019-09-04T18:11:01Z

src/legacy/core_plugins/data/public/expressions/expression_renderer.tsx

-> & {
-  expression: string | Ast;
+export interface ExpressionRendererProps extends IExpressionLoaderParams {
+  className: 'string';


@ppisljar The only allowed className here is the literal string "string"- the quotes should be removed

elasticmachine · 2019-09-30T10:30:40Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

ppisljar added WIP Work in progress Team:AppArch labels Jul 31, 2019

ppisljar requested review from flash1293 and lukeelmers July 31, 2019 11:37

flash1293 requested changes Aug 1, 2019

View reviewed changes

flash1293 mentioned this pull request Aug 6, 2019

use inspector types in data plugin #42615

Closed

7 tasks

lizozom reviewed Aug 6, 2019

View reviewed changes

ppisljar force-pushed the expressionService/1 branch from e5056d3 to 23af8fa Compare August 22, 2019 11:43

ppisljar added review and removed WIP Work in progress labels Aug 22, 2019

ppisljar requested a review from streamich August 22, 2019 11:43

ppisljar added v7.4.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Aug 22, 2019

streamich approved these changes Aug 22, 2019

View reviewed changes

streamich mentioned this pull request Aug 29, 2019

Expressions (Interpreter) 👉 New Platform #44153

Closed

9 tasks

lukeelmers reviewed Aug 29, 2019

View reviewed changes

ppisljar added 3 commits September 3, 2019 01:59

expression service

0615768

updating

199ddb6

updating based on review

3f4cb0d

ppisljar force-pushed the expressionService/1 branch from 23af8fa to 3f4cb0d Compare September 3, 2019 10:48

ppisljar added v7.5.0 and removed v7.4.0 labels Sep 3, 2019

fixes jest tests

d5fb9fa

flash1293 reviewed Sep 4, 2019

View reviewed changes

ppisljar added 2 commits September 4, 2019 03:12

adding classname prop

c1391ae

adding options to deps

ups

f9e0dfd

flash1293 approved these changes Sep 4, 2019

View reviewed changes

ppisljar merged commit 2f5306e into elastic:master Sep 4, 2019

ppisljar mentioned this pull request Sep 4, 2019

[7.x] expression service (#42337) #44765

Merged

wylieconlon reviewed Sep 4, 2019

View reviewed changes

wylieconlon mentioned this pull request Sep 4, 2019

[lens] Fix breakage from app-arch movements #44720

Merged

ppisljar added a commit to ppisljar/kibana that referenced this pull request Sep 9, 2019

expression service (elastic#42337)

70c7d82

ppisljar added a commit that referenced this pull request Sep 9, 2019

[7.x] expression service (#42337) (#44765)

7aac477

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression service #42337

expression service #42337

ppisljar commented Jul 31, 2019 •

edited

Loading

elasticmachine commented Jul 31, 2019

elasticmachine commented Jul 31, 2019

flash1293 Aug 1, 2019

flash1293 Aug 1, 2019

ppisljar Sep 3, 2019

flash1293 Aug 1, 2019

lizozom Aug 6, 2019 •

edited

Loading

elasticmachine commented Aug 22, 2019

streamich left a comment

streamich Aug 22, 2019

streamich Aug 22, 2019

ppisljar Sep 3, 2019

streamich Aug 22, 2019

ppisljar Sep 3, 2019

streamich Aug 22, 2019

lukeelmers Aug 29, 2019

lukeelmers Aug 29, 2019

lukeelmers Aug 29, 2019

lukeelmers Aug 29, 2019

ppisljar Sep 3, 2019

elasticmachine commented Sep 3, 2019

elasticmachine commented Sep 3, 2019

flash1293 Sep 4, 2019

flash1293 Sep 4, 2019

flash1293 Sep 4, 2019

flash1293 Sep 4, 2019 •

edited

Loading

ppisljar Sep 4, 2019

flash1293 left a comment

elasticmachine commented Sep 4, 2019

wylieconlon Sep 4, 2019

elasticmachine commented Sep 30, 2019

expression service #42337

expression service #42337

Conversation

ppisljar commented Jul 31, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Jul 31, 2019

elasticmachine commented Jul 31, 2019

💔 Build Failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lizozom Aug 6, 2019 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Aug 22, 2019

💔 Build Failed

streamich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Sep 3, 2019

💔 Build Failed

elasticmachine commented Sep 3, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 Sep 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

elasticmachine commented Sep 4, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

elasticmachine commented Sep 30, 2019

💔 Build Failed

ppisljar commented Jul 31, 2019 •

edited

Loading

lizozom Aug 6, 2019 •

edited

Loading

flash1293 Sep 4, 2019 •

edited

Loading