From 3655120d011d923d67e71bc1d041fdeee6caf31e Mon Sep 17 00:00:00 2001
From: Anton Dosov <anton.dosov@elastic.co>
Date: Thu, 21 Nov 2019 11:16:38 +0100
Subject: [PATCH 1/6] fix expressions service render swallows rendering errors
 #51153

prettier

remove error$ and errorRenderer, use onRenderError callback instead for both

use custom error handler only if it is provided by consumer, otherwise fallback to default handler

add comment

fix linter issues

fix
---
 .../expressions/public/execute.test.ts        |   7 +
 .../public/expression_renderer.test.tsx       |  83 ++++++-----
 .../public/expression_renderer.tsx            |  95 ++++++++-----
 src/plugins/expressions/public/loader.test.ts |   7 +
 src/plugins/expressions/public/loader.ts      |   4 +-
 src/plugins/expressions/public/plugin.ts      |   9 +-
 src/plugins/expressions/public/render.test.ts | 134 ++++++++++++------
 src/plugins/expressions/public/render.ts      |  68 ++++-----
 .../public/render_error_handler.ts            |  36 +++++
 src/plugins/expressions/public/services.ts    |   4 +
 src/plugins/expressions/public/types/index.ts |  14 ++
 11 files changed, 300 insertions(+), 161 deletions(-)
 create mode 100644 src/plugins/expressions/public/render_error_handler.ts

diff --git a/src/plugins/expressions/public/execute.test.ts b/src/plugins/expressions/public/execute.test.ts
index b60c4aed89fcf..6700ec38df940 100644
--- a/src/plugins/expressions/public/execute.test.ts
+++ b/src/plugins/expressions/public/execute.test.ts
@@ -29,6 +29,13 @@ jest.mock('./services', () => ({
       },
     };
   },
+  getNotifications: jest.fn(() => {
+    return {
+      toasts: {
+        addError: jest.fn(() => {}),
+      },
+    };
+  }),
 }));
 
 describe('execute helper function', () => {
diff --git a/src/plugins/expressions/public/expression_renderer.test.tsx b/src/plugins/expressions/public/expression_renderer.test.tsx
index 26db8753e6403..217618bc3a177 100644
--- a/src/plugins/expressions/public/expression_renderer.test.tsx
+++ b/src/plugins/expressions/public/expression_renderer.test.tsx
@@ -18,12 +18,14 @@
  */
 
 import React from 'react';
+import { act } from 'react-dom/test-utils';
 import { Subject } from 'rxjs';
 import { share } from 'rxjs/operators';
 import { ExpressionRendererImplementation } from './expression_renderer';
 import { ExpressionLoader } from './loader';
 import { mount } from 'enzyme';
 import { EuiProgress } from '@elastic/eui';
+import { RenderErrorHandlerFnType } from './types';
 
 jest.mock('./loader', () => {
   return {
@@ -54,60 +56,38 @@ describe('ExpressionRenderer', () => {
 
     const instance = mount(<ExpressionRendererImplementation expression="" />);
 
-    loadingSubject.next();
+    act(() => {
+      loadingSubject.next();
+    });
+
     instance.update();
 
     expect(instance.find(EuiProgress)).toHaveLength(1);
 
-    renderSubject.next(1);
+    act(() => {
+      renderSubject.next(1);
+    });
 
     instance.update();
 
     expect(instance.find(EuiProgress)).toHaveLength(0);
 
     instance.setProps({ expression: 'something new' });
-    loadingSubject.next();
+    act(() => {
+      loadingSubject.next();
+    });
     instance.update();
 
     expect(instance.find(EuiProgress)).toHaveLength(1);
-
-    renderSubject.next(1);
-    instance.update();
-
-    expect(instance.find(EuiProgress)).toHaveLength(0);
-  });
-
-  it('should display an error message when the expression fails', () => {
-    const dataSubject = new Subject();
-    const data$ = dataSubject.asObservable().pipe(share());
-    const renderSubject = new Subject();
-    const render$ = renderSubject.asObservable().pipe(share());
-    const loadingSubject = new Subject();
-    const loading$ = loadingSubject.asObservable().pipe(share());
-
-    (ExpressionLoader as jest.Mock).mockImplementation(() => {
-      return {
-        render$,
-        data$,
-        loading$,
-        update: jest.fn(),
-      };
-    });
-
-    const instance = mount(<ExpressionRendererImplementation expression="" />);
-
-    dataSubject.next('good data');
-    renderSubject.next({
-      type: 'error',
-      error: { message: 'render error' },
+    act(() => {
+      renderSubject.next(1);
     });
     instance.update();
 
     expect(instance.find(EuiProgress)).toHaveLength(0);
-    expect(instance.find('[data-test-subj="expression-renderer-error"]')).toHaveLength(1);
   });
 
-  it('should display a custom error message if the user provides one', () => {
+  it('should display a custom error message if the user provides one and then remove it after successful render', () => {
     const dataSubject = new Subject();
     const data$ = dataSubject.asObservable().pipe(share());
     const renderSubject = new Subject();
@@ -115,7 +95,10 @@ describe('ExpressionRenderer', () => {
     const loadingSubject = new Subject();
     const loading$ = loadingSubject.asObservable().pipe(share());
 
-    (ExpressionLoader as jest.Mock).mockImplementation(() => {
+    let onRenderError: RenderErrorHandlerFnType;
+    (ExpressionLoader as jest.Mock).mockImplementation((...args) => {
+      const params = args[2];
+      onRenderError = params.onRenderError;
       return {
         render$,
         data$,
@@ -124,18 +107,32 @@ describe('ExpressionRenderer', () => {
       };
     });
 
-    const renderErrorFn = jest.fn().mockReturnValue(null);
-
     const instance = mount(
-      <ExpressionRendererImplementation expression="" renderError={renderErrorFn} />
+      <ExpressionRendererImplementation
+        expression=""
+        renderError={message => <div data-test-subj={'custom-error'}>{message}</div>}
+      />
     );
 
-    renderSubject.next({
-      type: 'error',
-      error: { message: 'render error' },
+    act(() => {
+      onRenderError!(instance.getDOMNode(), new Error('render error'), {
+        done: () => {
+          renderSubject.next(1);
+        },
+      } as any);
     });
+
     instance.update();
+    expect(instance.find(EuiProgress)).toHaveLength(0);
+    expect(instance.find('[data-test-subj="custom-error"]')).toHaveLength(1);
+    expect(instance.find('[data-test-subj="custom-error"]').contains('render error')).toBeTruthy();
 
-    expect(renderErrorFn).toHaveBeenCalledWith('render error');
+    act(() => {
+      loadingSubject.next();
+      renderSubject.next(2);
+    });
+    instance.update();
+    expect(instance.find(EuiProgress)).toHaveLength(0);
+    expect(instance.find('[data-test-subj="custom-error"]')).toHaveLength(0);
   });
 });
diff --git a/src/plugins/expressions/public/expression_renderer.tsx b/src/plugins/expressions/public/expression_renderer.tsx
index b4f0a509c81b6..912f2c2895f5e 100644
--- a/src/plugins/expressions/public/expression_renderer.tsx
+++ b/src/plugins/expressions/public/expression_renderer.tsx
@@ -17,12 +17,14 @@
  * under the License.
  */
 
-import { useRef, useEffect, useState } from 'react';
+import { useRef, useEffect, useState, useLayoutEffect } from 'react';
 import React from 'react';
 import classNames from 'classnames';
+import { Subscription } from 'rxjs';
+import { filter } from 'rxjs/operators';
 import { EuiLoadingChart, EuiProgress } from '@elastic/eui';
 import theme from '@elastic/eui/dist/eui_theme_light.json';
-import { IExpressionLoaderParams } from './types';
+import { IExpressionLoaderParams, IInterpreterRenderHandlers, RenderError } from './types';
 import { ExpressionAST } from '../common/types';
 import { ExpressionLoader } from './loader';
 
@@ -39,7 +41,7 @@ export interface ExpressionRendererProps extends IExpressionLoaderParams {
 interface State {
   isEmpty: boolean;
   isLoading: boolean;
-  error: null | { message: string };
+  error: null | RenderError;
 }
 
 export type ExpressionRenderer = React.FC<ExpressionRendererProps>;
@@ -77,49 +79,70 @@ export const ExpressionRendererImplementation = ({
   ]);
   /* eslint-enable react-hooks/exhaustive-deps */
 
+  // flag to skip next render$ notification,
+  // because of just handled error
+  const hasHandledErrorRef = useRef(false);
+
+  // will call done() in LayoutEffect when done with rendering custom error state
+  const errorRenderHandlerRef: React.MutableRefObject<null | IInterpreterRenderHandlers> = useRef(
+    null
+  );
   // Initialize the loader only once
   useEffect(() => {
-    if (mountpoint.current && !handlerRef.current) {
-      handlerRef.current = new ExpressionLoader(mountpoint.current, expression, options);
+    if (handlerRef.current) return;
+    const subs: Subscription[] = [];
 
-      handlerRef.current.loading$.subscribe(() => {
-        if (!handlerRef.current) {
-          return;
-        }
-        setState(prevState => ({ ...prevState, isLoading: true }));
-      });
-      handlerRef.current.render$.subscribe(item => {
-        if (!handlerRef.current) {
-          return;
-        }
-        if (typeof item !== 'number') {
-          setState(() => ({
-            ...defaultState,
-            isEmpty: false,
-            error: item.error,
-          }));
-        } else {
+    handlerRef.current = new ExpressionLoader(mountpoint.current!, expression, {
+      ...options,
+      // react component wrapper provides different
+      // error handling api which is easier to work with from react
+      // if custom renderError is not provided then we fallback to default error handling from ExpressionLoader
+      // TODO: track renderError prop change as dep?
+      onRenderError:
+        renderError &&
+        ((domNode, error, handlers) => {
+          errorRenderHandlerRef.current = handlers;
           setState(() => ({
             ...defaultState,
             isEmpty: false,
+            error,
           }));
-        }
-      });
-    }
-  /* eslint-disable */
-  // TODO: Replace mountpoint.current by something else.
-  }, [mountpoint.current]);
-  /* eslint-enable */
+        }),
+    });
+    subs.push(
+      handlerRef.current.loading$.subscribe(() => {
+        hasHandledErrorRef.current = false;
+        setState(prevState => ({ ...prevState, isLoading: true }));
+      }),
+      handlerRef.current.render$.pipe(filter(() => !hasHandledErrorRef.current)).subscribe(item => {
+        setState(() => ({
+          ...defaultState,
+          isEmpty: false,
+        }));
+      })
+    );
 
-  useEffect(() => {
-    // We only want a clean up to run when the entire component is unloaded, not on every render
-    return function cleanup() {
+    return () => {
+      subs.forEach(s => s.unsubscribe());
       if (handlerRef.current) {
         handlerRef.current.destroy();
         handlerRef.current = null;
       }
+      errorRenderHandlerRef.current = null;
     };
+
+    /* eslint-disable react-hooks/exhaustive-deps */
   }, []);
+  /* eslint-enable react-hooks/exhaustive-deps */
+
+  // call expression loader's done() handler when finished rendering custom error state
+  useLayoutEffect(() => {
+    if (state.error && errorRenderHandlerRef.current) {
+      hasHandledErrorRef.current = true;
+      errorRenderHandlerRef.current.done();
+      errorRenderHandlerRef.current = null;
+    }
+  }, [state.error]);
 
   const classes = classNames('expExpressionRenderer', {
     'expExpressionRenderer-isEmpty': state.isEmpty,
@@ -137,13 +160,7 @@ export const ExpressionRendererImplementation = ({
     <div {...dataAttrs} className={classes}>
       {state.isEmpty ? <EuiLoadingChart mono size="l" /> : null}
       {state.isLoading ? <EuiProgress size="xs" color="accent" position="absolute" /> : null}
-      {!state.isLoading && state.error ? (
-        renderError ? (
-          renderError(state.error.message)
-        ) : (
-          <div data-test-subj="expression-renderer-error">{state.error.message}</div>
-        )
-      ) : null}
+      {!state.isLoading && state.error && renderError && renderError(state.error.message)}
       <div
         className="expExpressionRenderer__expression"
         style={expressionStyles}
diff --git a/src/plugins/expressions/public/loader.test.ts b/src/plugins/expressions/public/loader.test.ts
index 1a9c878c8808d..b6fec8ee97b15 100644
--- a/src/plugins/expressions/public/loader.test.ts
+++ b/src/plugins/expressions/public/loader.test.ts
@@ -46,6 +46,13 @@ jest.mock('./services', () => {
     getRenderersRegistry: () => ({
       get: (id: string) => renderers[id],
     }),
+    getNotifications: jest.fn(() => {
+      return {
+        toasts: {
+          addError: jest.fn(() => {}),
+        },
+      };
+    }),
   };
 });
 
diff --git a/src/plugins/expressions/public/loader.ts b/src/plugins/expressions/public/loader.ts
index 200249b60c773..a956259ca1016 100644
--- a/src/plugins/expressions/public/loader.ts
+++ b/src/plugins/expressions/public/loader.ts
@@ -51,7 +51,9 @@ export class ExpressionLoader {
     this.loadingSubject = new Subject();
     this.loading$ = this.loadingSubject.asObservable().pipe(share());
 
-    this.renderHandler = new ExpressionRenderHandler(element);
+    this.renderHandler = new ExpressionRenderHandler(element, {
+      onRenderError: params && params.onRenderError,
+    });
     this.render$ = this.renderHandler.render$;
     this.update$ = this.renderHandler.update$;
     this.events$ = this.renderHandler.events$;
diff --git a/src/plugins/expressions/public/plugin.ts b/src/plugins/expressions/public/plugin.ts
index 3a28256d57162..7471326cdd749 100644
--- a/src/plugins/expressions/public/plugin.ts
+++ b/src/plugins/expressions/public/plugin.ts
@@ -21,7 +21,13 @@ import { PluginInitializerContext, CoreSetup, CoreStart, Plugin } from '../../..
 import { ExpressionInterpretWithHandlers, ExpressionExecutor } from './types';
 import { FunctionsRegistry, RenderFunctionsRegistry, TypesRegistry } from './registries';
 import { Setup as InspectorSetup, Start as InspectorStart } from '../../inspector/public';
-import { setCoreStart, setInspector, setInterpreter, setRenderersRegistry } from './services';
+import {
+  setCoreStart,
+  setInspector,
+  setInterpreter,
+  setRenderersRegistry,
+  setNotifications,
+} from './services';
 import { clog as clogFunction } from './functions/clog';
 import { font as fontFunction } from './functions/font';
 import { kibana as kibanaFunction } from './functions/kibana';
@@ -158,6 +164,7 @@ export class ExpressionsPublicPlugin
   public start(core: CoreStart, { inspector }: ExpressionsStartDeps): ExpressionsStart {
     setCoreStart(core);
     setInspector(inspector);
+    setNotifications(core.notifications);
 
     return {
       execute,
diff --git a/src/plugins/expressions/public/render.test.ts b/src/plugins/expressions/public/render.test.ts
index 6b5acc8405fd2..56eb43a9bd133 100644
--- a/src/plugins/expressions/public/render.test.ts
+++ b/src/plugins/expressions/public/render.test.ts
@@ -17,14 +17,18 @@
  * under the License.
  */
 
-import { render, ExpressionRenderHandler } from './render';
+import { ExpressionRenderHandler, render } from './render';
 import { Observable } from 'rxjs';
-import { IInterpreterRenderHandlers } from './types';
+import { IInterpreterRenderHandlers, RenderError } from './types';
 import { getRenderersRegistry } from './services';
-import { first } from 'rxjs/operators';
+import { first, take, toArray } from 'rxjs/operators';
 
 const element: HTMLElement = {} as HTMLElement;
-
+const mockNotificationService = {
+  toasts: {
+    addError: jest.fn(() => {}),
+  },
+};
 jest.mock('./services', () => {
   const renderers: Record<string, unknown> = {
     test: {
@@ -38,9 +42,24 @@ jest.mock('./services', () => {
     getRenderersRegistry: jest.fn(() => ({
       get: jest.fn((id: string) => renderers[id]),
     })),
+    getNotifications: jest.fn(() => {
+      return mockNotificationService;
+    }),
   };
 });
 
+const mockMockErrorRenderFunction = jest.fn(
+  (el: HTMLElement, error: RenderError, handlers: IInterpreterRenderHandlers) => handlers.done()
+);
+// extracts data from mockMockErrorRenderFunction call to assert in tests
+const getHandledError = () => {
+  try {
+    return mockMockErrorRenderFunction.mock.calls[0][1];
+  } catch (e) {
+    return null;
+  }
+};
+
 describe('render helper function', () => {
   it('returns ExpressionRenderHandler instance', () => {
     const response = render(element, {});
@@ -62,40 +81,33 @@ describe('ExpressionRenderHandler', () => {
   });
 
   describe('render()', () => {
-    it('sends an observable error and keeps it open if invalid data is provided', async () => {
+    beforeEach(() => {
+      mockMockErrorRenderFunction.mockClear();
+      mockNotificationService.toasts.addError.mockClear();
+    });
+
+    it('in case of error render$ should emit when error renderer is finished', async () => {
       const expressionRenderHandler = new ExpressionRenderHandler(element);
-      const promise1 = expressionRenderHandler.render$.pipe(first()).toPromise();
       expressionRenderHandler.render(false);
-      await expect(promise1).resolves.toEqual({
-        type: 'error',
-        error: {
-          message: 'invalid data provided to the expression renderer',
-        },
-      });
+      const promise1 = expressionRenderHandler.render$.pipe(first()).toPromise();
+      await expect(promise1).resolves.toEqual(1);
 
-      const promise2 = expressionRenderHandler.render$.pipe(first()).toPromise();
       expressionRenderHandler.render(false);
-      await expect(promise2).resolves.toEqual({
-        type: 'error',
-        error: {
-          message: 'invalid data provided to the expression renderer',
-        },
-      });
+      const promise2 = expressionRenderHandler.render$.pipe(first()).toPromise();
+      await expect(promise2).resolves.toEqual(2);
     });
 
-    it('sends an observable error if renderer does not exist', async () => {
-      const expressionRenderHandler = new ExpressionRenderHandler(element);
-      const promise = expressionRenderHandler.render$.pipe(first()).toPromise();
-      expressionRenderHandler.render({ type: 'render', as: 'something' });
-      await expect(promise).resolves.toEqual({
-        type: 'error',
-        error: {
-          message: `invalid renderer id 'something'`,
-        },
+    it('should use custom error handler if provided', async () => {
+      const expressionRenderHandler = new ExpressionRenderHandler(element, {
+        onRenderError: mockMockErrorRenderFunction,
       });
+      await expressionRenderHandler.render(false);
+      expect(getHandledError()!.message).toEqual(
+        `invalid data provided to the expression renderer`
+      );
     });
 
-    it('sends an observable error if the rendering function throws', async () => {
+    it('should throw error if the rendering function throws', async () => {
       (getRenderersRegistry as jest.Mock).mockReturnValueOnce({ get: () => true });
       (getRenderersRegistry as jest.Mock).mockReturnValueOnce({
         get: () => ({
@@ -105,15 +117,11 @@ describe('ExpressionRenderHandler', () => {
         }),
       });
 
-      const expressionRenderHandler = new ExpressionRenderHandler(element);
-      const promise = expressionRenderHandler.render$.pipe(first()).toPromise();
-      expressionRenderHandler.render({ type: 'render', as: 'something' });
-      await expect(promise).resolves.toEqual({
-        type: 'error',
-        error: {
-          message: 'renderer error',
-        },
+      const expressionRenderHandler = new ExpressionRenderHandler(element, {
+        onRenderError: mockMockErrorRenderFunction,
       });
+      await expressionRenderHandler.render({ type: 'render', as: 'something' });
+      expect(getHandledError()!.message).toEqual('renderer error');
     });
 
     it('sends a next observable once rendering is complete', () => {
@@ -129,18 +137,56 @@ describe('ExpressionRenderHandler', () => {
       });
     });
 
+    it('default renderer should use notification service', async () => {
+      const expressionRenderHandler = new ExpressionRenderHandler(element);
+      const promise1 = expressionRenderHandler.render$.pipe(first()).toPromise();
+      expressionRenderHandler.render(false);
+      await expect(promise1).resolves.toEqual(1);
+      expect(mockNotificationService.toasts.addError).toBeCalledWith(
+        expect.objectContaining({
+          message: 'invalid data provided to the expression renderer',
+        }),
+        {
+          title: 'Error in visualisation',
+          toastMessage: 'invalid data provided to the expression renderer',
+        }
+      );
+    });
+
     // in case render$ subscription happen after render() got called
     // we still want to be notified about sync render$ updates
     it("doesn't swallow sync render errors", async () => {
+      const expressionRenderHandler1 = new ExpressionRenderHandler(element, {
+        onRenderError: mockMockErrorRenderFunction,
+      });
+      expressionRenderHandler1.render(false);
+      const renderPromiseAfterRender = expressionRenderHandler1.render$.pipe(first()).toPromise();
+      await expect(renderPromiseAfterRender).resolves.toEqual(1);
+      expect(getHandledError()!.message).toEqual(
+        'invalid data provided to the expression renderer'
+      );
+
+      mockMockErrorRenderFunction.mockClear();
+
+      const expressionRenderHandler2 = new ExpressionRenderHandler(element, {
+        onRenderError: mockMockErrorRenderFunction,
+      });
+      const renderPromiseBeforeRender = expressionRenderHandler2.render$.pipe(first()).toPromise();
+      expressionRenderHandler2.render(false);
+      await expect(renderPromiseBeforeRender).resolves.toEqual(1);
+      expect(getHandledError()!.message).toEqual(
+        'invalid data provided to the expression renderer'
+      );
+    });
+
+    // it is expected side effect of using BehaviorSubject for render$,
+    // that observables will emit previous result if subscription happens after render
+    it('should emit previous render and error results', async () => {
       const expressionRenderHandler = new ExpressionRenderHandler(element);
       expressionRenderHandler.render(false);
-      const promise = expressionRenderHandler.render$.pipe(first()).toPromise();
-      await expect(promise).resolves.toEqual({
-        type: 'error',
-        error: {
-          message: 'invalid data provided to the expression renderer',
-        },
-      });
+      const renderPromise = expressionRenderHandler.render$.pipe(take(2), toArray()).toPromise();
+      expressionRenderHandler.render(false);
+      await expect(renderPromise).resolves.toEqual([1, 2]);
     });
   });
 });
diff --git a/src/plugins/expressions/public/render.ts b/src/plugins/expressions/public/render.ts
index 364d5f587bb6f..84e7b445b1541 100644
--- a/src/plugins/expressions/public/render.ts
+++ b/src/plugins/expressions/public/render.ts
@@ -20,40 +20,53 @@
 import { Observable } from 'rxjs';
 import * as Rx from 'rxjs';
 import { filter, share } from 'rxjs/operators';
-import { event, RenderId, Data, IInterpreterRenderHandlers } from './types';
+import {
+  event,
+  RenderId,
+  Data,
+  IInterpreterRenderHandlers,
+  RenderErrorHandlerFnType,
+  RenderError,
+} from './types';
 import { getRenderersRegistry } from './services';
-
-interface RenderError {
-  type: 'error';
-  error: { type?: string; message: string };
-}
+import { renderErrorHandler as defaultRenderErrorHandler } from './render_error_handler';
 
 export type IExpressionRendererExtraHandlers = Record<string, any>;
 
+export interface ExpressionRenderHandlerParams {
+  onRenderError: RenderErrorHandlerFnType;
+}
+
 export class ExpressionRenderHandler {
-  render$: Observable<RenderId | RenderError>;
+  render$: Observable<RenderId>;
   update$: Observable<any>;
   events$: Observable<event>;
 
   private element: HTMLElement;
   private destroyFn?: any;
   private renderCount: number = 0;
-  private renderSubject: Rx.BehaviorSubject<RenderId | RenderError | null>;
+  private renderSubject: Rx.BehaviorSubject<RenderId | null>;
   private eventsSubject: Rx.Subject<unknown>;
   private updateSubject: Rx.Subject<unknown>;
   private handlers: IInterpreterRenderHandlers;
+  private onRenderError: RenderErrorHandlerFnType;
 
-  constructor(element: HTMLElement) {
+  constructor(
+    element: HTMLElement,
+    { onRenderError }: Partial<ExpressionRenderHandlerParams> = {}
+  ) {
     this.element = element;
 
     this.eventsSubject = new Rx.Subject();
     this.events$ = this.eventsSubject.asObservable().pipe(share());
 
-    this.renderSubject = new Rx.BehaviorSubject(null as RenderId | RenderError | null);
+    this.onRenderError = onRenderError || defaultRenderErrorHandler;
+
+    this.renderSubject = new Rx.BehaviorSubject(null as RenderId | null);
     this.render$ = this.renderSubject.asObservable().pipe(
       share(),
       filter(_ => _ !== null)
-    ) as Observable<RenderId | RenderError>;
+    ) as Observable<RenderId>;
 
     this.updateSubject = new Rx.Subject();
     this.update$ = this.updateSubject.asObservable().pipe(share());
@@ -80,33 +93,21 @@ export class ExpressionRenderHandler {
 
   render = async (data: Data, extraHandlers: IExpressionRendererExtraHandlers = {}) => {
     if (!data || typeof data !== 'object') {
-      this.renderSubject.next({
-        type: 'error',
-        error: {
-          message: 'invalid data provided to the expression renderer',
-        },
-      });
-      return;
+      return this.handleRenderError(new Error('invalid data provided to the expression renderer'));
     }
 
     if (data.type !== 'render' || !data.as) {
       if (data.type === 'error') {
-        this.renderSubject.next(data);
+        return this.handleRenderError(data.error);
       } else {
-        this.renderSubject.next({
-          type: 'error',
-          error: { message: 'invalid data provided to the expression renderer' },
-        });
+        return this.handleRenderError(
+          new Error('invalid data provided to the expression renderer')
+        );
       }
-      return;
     }
 
     if (!getRenderersRegistry().get(data.as)) {
-      this.renderSubject.next({
-        type: 'error',
-        error: { message: `invalid renderer id '${data.as}'` },
-      });
-      return;
+      return this.handleRenderError(new Error(`invalid renderer id '${data.as}'`));
     }
 
     try {
@@ -115,10 +116,7 @@ export class ExpressionRenderHandler {
         .get(data.as)!
         .render(this.element, data.value, { ...this.handlers, ...extraHandlers });
     } catch (e) {
-      this.renderSubject.next({
-        type: 'error',
-        error: { type: e.type, message: e.message },
-      });
+      return this.handleRenderError(e);
     }
   };
 
@@ -134,6 +132,10 @@ export class ExpressionRenderHandler {
   getElement = () => {
     return this.element;
   };
+
+  handleRenderError = (error: RenderError) => {
+    this.onRenderError(this.element, error, this.handlers);
+  };
 }
 
 export function render(element: HTMLElement, data: Data): ExpressionRenderHandler {
diff --git a/src/plugins/expressions/public/render_error_handler.ts b/src/plugins/expressions/public/render_error_handler.ts
new file mode 100644
index 0000000000000..4d6bee1e375e0
--- /dev/null
+++ b/src/plugins/expressions/public/render_error_handler.ts
@@ -0,0 +1,36 @@
+/*
+ * 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 { i18n } from '@kbn/i18n';
+import { RenderErrorHandlerFnType, IInterpreterRenderHandlers, RenderError } from './types';
+import { getNotifications } from './services';
+
+export const renderErrorHandler: RenderErrorHandlerFnType = (
+  element: HTMLElement,
+  error: RenderError,
+  handlers: IInterpreterRenderHandlers
+) => {
+  getNotifications().toasts.addError(error, {
+    title: i18n.translate('expressions.defaultErrorRenderer.errorTitle', {
+      defaultMessage: 'Error in visualisation',
+    }),
+    toastMessage: error.message,
+  });
+  handlers.done();
+};
diff --git a/src/plugins/expressions/public/services.ts b/src/plugins/expressions/public/services.ts
index a1a42aa85e670..75ec4826ea45a 100644
--- a/src/plugins/expressions/public/services.ts
+++ b/src/plugins/expressions/public/services.ts
@@ -17,6 +17,7 @@
  * under the License.
  */
 
+import { NotificationsStart } from 'kibana/public';
 import { createKibanaUtilsCore, createGetterSetter } from '../../kibana_utils/public';
 import { ExpressionInterpreter } from './types';
 import { Start as IInspector } from '../../inspector/public';
@@ -29,6 +30,9 @@ export const [getInspector, setInspector] = createGetterSetter<IInspector>('Insp
 export const [getInterpreter, setInterpreter] = createGetterSetter<ExpressionInterpreter>(
   'Interpreter'
 );
+export const [getNotifications, setNotifications] = createGetterSetter<NotificationsStart>(
+  'Notifications'
+);
 
 export const [getRenderersRegistry, setRenderersRegistry] = createGetterSetter<
   ExpressionsSetup['__LEGACY']['renderers']
diff --git a/src/plugins/expressions/public/types/index.ts b/src/plugins/expressions/public/types/index.ts
index d86e042bca15c..66a3da48dbee9 100644
--- a/src/plugins/expressions/public/types/index.ts
+++ b/src/plugins/expressions/public/types/index.ts
@@ -20,6 +20,7 @@
 import { ExpressionInterpret } from '../interpreter_provider';
 import { TimeRange, Query, esFilters } from '../../../data/public';
 import { Adapters } from '../../../inspector/public';
+import { ExpressionRenderDefinition } from '../registries';
 
 export type ExpressionInterpretWithHandlers = (
   ast: Parameters<ExpressionInterpret>[0],
@@ -58,6 +59,7 @@ export interface IExpressionLoaderParams {
   customRenderers?: [];
   extraHandlers?: Record<string, any>;
   inspectorAdapters?: Adapters;
+  onRenderError?: RenderErrorHandlerFnType;
 }
 
 export interface IInterpreterHandlers {
@@ -99,3 +101,15 @@ export interface IInterpreterSuccessResult {
 }
 
 export type IInterpreterResult = IInterpreterSuccessResult & IInterpreterErrorResult;
+
+export { ExpressionRenderDefinition };
+
+export interface RenderError extends Error {
+  type?: string;
+}
+
+export type RenderErrorHandlerFnType = (
+  domNode: HTMLElement,
+  error: RenderError,
+  handlers: IInterpreterRenderHandlers
+) => void;

From 21d8c9c92d39b93a1cef51c59428ecb9231edc54 Mon Sep 17 00:00:00 2001
From: Anton Dosov <anton.dosov@elastic.co>
Date: Fri, 22 Nov 2019 13:49:21 +0100
Subject: [PATCH 2/6] fix expression service missing initial loading

move utilities to kibana_react/util
---
 .../public/expression_renderer.tsx            | 103 +++++++++---------
 src/plugins/expressions/public/loader.test.ts |  16 +--
 src/plugins/expressions/public/loader.ts      |  25 +++--
 src/plugins/expressions/public/render.ts      |  21 ++--
 src/plugins/kibana_react/public/index.ts      |   2 +-
 src/plugins/kibana_react/public/util/index.ts |   1 +
 .../util/use_shallow_compare_effect.test.ts   |  86 +++++++++++++++
 .../public/util/use_shallow_compare_effect.ts |  80 ++++++++++++++
 8 files changed, 253 insertions(+), 81 deletions(-)
 create mode 100644 src/plugins/kibana_react/public/util/use_shallow_compare_effect.test.ts
 create mode 100644 src/plugins/kibana_react/public/util/use_shallow_compare_effect.ts

diff --git a/src/plugins/expressions/public/expression_renderer.tsx b/src/plugins/expressions/public/expression_renderer.tsx
index 912f2c2895f5e..3989f4ed7d698 100644
--- a/src/plugins/expressions/public/expression_renderer.tsx
+++ b/src/plugins/expressions/public/expression_renderer.tsx
@@ -24,6 +24,7 @@ import { Subscription } from 'rxjs';
 import { filter } from 'rxjs/operators';
 import { EuiLoadingChart, EuiProgress } from '@elastic/eui';
 import theme from '@elastic/eui/dist/eui_theme_light.json';
+import { useShallowCompareEffect } from '../../kibana_react/public';
 import { IExpressionLoaderParams, IInterpreterRenderHandlers, RenderError } from './types';
 import { ExpressionAST } from '../common/types';
 import { ExpressionLoader } from './loader';
@@ -55,30 +56,15 @@ const defaultState: State = {
 export const ExpressionRendererImplementation = ({
   className,
   dataAttrs,
-  expression,
-  renderError,
   padding,
-  ...options
+  renderError,
+  expression,
+  ...expressionLoaderOptions
 }: ExpressionRendererProps) => {
   const mountpoint: React.MutableRefObject<null | HTMLDivElement> = useRef(null);
-  const handlerRef: React.MutableRefObject<null | ExpressionLoader> = useRef(null);
   const [state, setState] = useState<State>({ ...defaultState });
-
-  // Re-fetch data automatically when the inputs change
-  /* eslint-disable react-hooks/exhaustive-deps */
-  useEffect(() => {
-    if (handlerRef.current) {
-      handlerRef.current.update(expression, options);
-    }
-  }, [
-    expression,
-    options.searchContext,
-    options.context,
-    options.variables,
-    options.disableCaching,
-  ]);
-  /* eslint-enable react-hooks/exhaustive-deps */
-
+  const hasCustomRenderErrorHandler = !!renderError;
+  const expressionLoaderRef: React.MutableRefObject<null | ExpressionLoader> = useRef(null);
   // flag to skip next render$ notification,
   // because of just handled error
   const hasHandledErrorRef = useRef(false);
@@ -87,54 +73,69 @@ export const ExpressionRendererImplementation = ({
   const errorRenderHandlerRef: React.MutableRefObject<null | IInterpreterRenderHandlers> = useRef(
     null
   );
-  // Initialize the loader only once
+
+  /* eslint-disable react-hooks/exhaustive-deps */
+  // OK to ignore react-hooks/exhaustive-deps because options update is handled by calling .update()
   useEffect(() => {
-    if (handlerRef.current) return;
     const subs: Subscription[] = [];
-
-    handlerRef.current = new ExpressionLoader(mountpoint.current!, expression, {
-      ...options,
+    expressionLoaderRef.current = new ExpressionLoader(mountpoint.current!, expression, {
+      ...expressionLoaderOptions,
       // react component wrapper provides different
       // error handling api which is easier to work with from react
       // if custom renderError is not provided then we fallback to default error handling from ExpressionLoader
-      // TODO: track renderError prop change as dep?
-      onRenderError:
-        renderError &&
-        ((domNode, error, handlers) => {
-          errorRenderHandlerRef.current = handlers;
-          setState(() => ({
-            ...defaultState,
-            isEmpty: false,
-            error,
-          }));
-        }),
+      onRenderError: hasCustomRenderErrorHandler
+        ? (domNode, error, handlers) => {
+            errorRenderHandlerRef.current = handlers;
+            setState(() => ({
+              ...defaultState,
+              isEmpty: false,
+              error,
+            }));
+
+            if (expressionLoaderOptions.onRenderError) {
+              expressionLoaderOptions.onRenderError(domNode, error, handlers);
+            }
+          }
+        : expressionLoaderOptions.onRenderError,
     });
     subs.push(
-      handlerRef.current.loading$.subscribe(() => {
+      expressionLoaderRef.current.loading$.subscribe(() => {
         hasHandledErrorRef.current = false;
         setState(prevState => ({ ...prevState, isLoading: true }));
       }),
-      handlerRef.current.render$.pipe(filter(() => !hasHandledErrorRef.current)).subscribe(item => {
-        setState(() => ({
-          ...defaultState,
-          isEmpty: false,
-        }));
-      })
+      expressionLoaderRef.current.render$
+        .pipe(filter(() => !hasHandledErrorRef.current))
+        .subscribe(item => {
+          setState(() => ({
+            ...defaultState,
+            isEmpty: false,
+          }));
+        })
     );
 
     return () => {
       subs.forEach(s => s.unsubscribe());
-      if (handlerRef.current) {
-        handlerRef.current.destroy();
-        handlerRef.current = null;
+      if (expressionLoaderRef.current) {
+        expressionLoaderRef.current.destroy();
+        expressionLoaderRef.current = null;
       }
+
       errorRenderHandlerRef.current = null;
     };
+  }, [hasCustomRenderErrorHandler]);
 
-    /* eslint-disable react-hooks/exhaustive-deps */
-  }, []);
-  /* eslint-enable react-hooks/exhaustive-deps */
+  // Re-fetch data automatically when the inputs change
+  useShallowCompareEffect(
+    () => {
+      if (expressionLoaderRef.current) {
+        expressionLoaderRef.current.update(expression, expressionLoaderOptions);
+      }
+    },
+    // when expression is changed by reference and when any other loaderOption is changed by reference
+    [{ expression, ...expressionLoaderOptions }]
+  );
 
+  /* eslint-enable react-hooks/exhaustive-deps */
   // call expression loader's done() handler when finished rendering custom error state
   useLayoutEffect(() => {
     if (state.error && errorRenderHandlerRef.current) {
@@ -158,8 +159,8 @@ export const ExpressionRendererImplementation = ({
 
   return (
     <div {...dataAttrs} className={classes}>
-      {state.isEmpty ? <EuiLoadingChart mono size="l" /> : null}
-      {state.isLoading ? <EuiProgress size="xs" color="accent" position="absolute" /> : null}
+      {state.isEmpty && <EuiLoadingChart mono size="l" />}
+      {state.isLoading && <EuiProgress size="xs" color="accent" position="absolute" />}
       {!state.isLoading && state.error && renderError && renderError(state.error.message)}
       <div
         className="expExpressionRenderer__expression"
diff --git a/src/plugins/expressions/public/loader.test.ts b/src/plugins/expressions/public/loader.test.ts
index b6fec8ee97b15..0a01cc29ff9dc 100644
--- a/src/plugins/expressions/public/loader.test.ts
+++ b/src/plugins/expressions/public/loader.test.ts
@@ -17,7 +17,7 @@
  * under the License.
  */
 
-import { first, skip } from 'rxjs/operators';
+import { first, skip, toArray } from 'rxjs/operators';
 import { fromExpression } from '@kbn/interpreter/common';
 import { loader, ExpressionLoader } from './loader';
 import { ExpressionDataHandler } from './execute';
@@ -104,20 +104,14 @@ describe('ExpressionLoader', () => {
     expect(response).toEqual({ type: 'render', as: 'test' });
   });
 
-  it('emits on loading$ when starting to load', async () => {
+  it('emits on loading$ on initial load and on updates', async () => {
     const expressionLoader = new ExpressionLoader(element, expressionString, {});
-    let loadingPromise = expressionLoader.loading$.pipe(first()).toPromise();
+    const loadingPromise = expressionLoader.loading$.pipe(toArray()).toPromise();
     expressionLoader.update('test');
-    let response = await loadingPromise;
-    expect(response).toBeUndefined();
-    loadingPromise = expressionLoader.loading$.pipe(first()).toPromise();
     expressionLoader.update('');
-    response = await loadingPromise;
-    expect(response).toBeUndefined();
-    loadingPromise = expressionLoader.loading$.pipe(first()).toPromise();
     expressionLoader.update();
-    response = await loadingPromise;
-    expect(response).toBeUndefined();
+    expressionLoader.destroy();
+    expect(await loadingPromise).toHaveLength(4);
   });
 
   it('emits on render$ when rendering is done', async () => {
diff --git a/src/plugins/expressions/public/loader.ts b/src/plugins/expressions/public/loader.ts
index a956259ca1016..0342713f7627b 100644
--- a/src/plugins/expressions/public/loader.ts
+++ b/src/plugins/expressions/public/loader.ts
@@ -17,8 +17,8 @@
  * under the License.
  */
 
-import { Observable, Subject } from 'rxjs';
-import { share } from 'rxjs/operators';
+import { BehaviorSubject, Observable, Subject } from 'rxjs';
+import { filter, map } from 'rxjs/operators';
 import { Adapters, InspectorSession } from '../../inspector/public';
 import { ExpressionDataHandler } from './execute';
 import { ExpressionRenderHandler } from './render';
@@ -36,7 +36,7 @@ export class ExpressionLoader {
   private dataHandler: ExpressionDataHandler | undefined;
   private renderHandler: ExpressionRenderHandler;
   private dataSubject: Subject<Data>;
-  private loadingSubject: Subject<void>;
+  private loadingSubject: Subject<boolean>;
   private data: Data;
   private params: IExpressionLoaderParams = {};
 
@@ -46,10 +46,16 @@ export class ExpressionLoader {
     params?: IExpressionLoaderParams
   ) {
     this.dataSubject = new Subject();
-    this.data$ = this.dataSubject.asObservable().pipe(share());
+    this.data$ = this.dataSubject.asObservable();
 
-    this.loadingSubject = new Subject();
-    this.loading$ = this.loadingSubject.asObservable().pipe(share());
+    this.loadingSubject = new BehaviorSubject<boolean>(false);
+    // loading is a "hot" observable,
+    // as loading$ could emit straight away in the constructor
+    // and we want to notify subscribers about it, but all subscriptions will happen later
+    this.loading$ = this.loadingSubject.asObservable().pipe(
+      filter(_ => _ === true),
+      map(() => void 0)
+    );
 
     this.renderHandler = new ExpressionRenderHandler(element, {
       onRenderError: params && params.onRenderError,
@@ -66,9 +72,14 @@ export class ExpressionLoader {
       this.render(data);
     });
 
+    this.render$.subscribe(() => {
+      this.loadingSubject.next(false);
+    });
+
     this.setParams(params);
 
     if (expression) {
+      this.loadingSubject.next(true);
       this.loadData(expression, this.params);
     }
   }
@@ -122,7 +133,7 @@ export class ExpressionLoader {
   update(expression?: string | ExpressionAST, params?: IExpressionLoaderParams): void {
     this.setParams(params);
 
-    this.loadingSubject.next();
+    this.loadingSubject.next(true);
     if (expression) {
       this.loadData(expression, this.params);
     } else if (this.data) {
diff --git a/src/plugins/expressions/public/render.ts b/src/plugins/expressions/public/render.ts
index 84e7b445b1541..e34959f468a13 100644
--- a/src/plugins/expressions/public/render.ts
+++ b/src/plugins/expressions/public/render.ts
@@ -17,16 +17,16 @@
  * under the License.
  */
 
-import { Observable } from 'rxjs';
 import * as Rx from 'rxjs';
-import { filter, share } from 'rxjs/operators';
+import { Observable } from 'rxjs';
+import { filter } from 'rxjs/operators';
 import {
-  event,
-  RenderId,
   Data,
+  event,
   IInterpreterRenderHandlers,
-  RenderErrorHandlerFnType,
   RenderError,
+  RenderErrorHandlerFnType,
+  RenderId,
 } from './types';
 import { getRenderersRegistry } from './services';
 import { renderErrorHandler as defaultRenderErrorHandler } from './render_error_handler';
@@ -58,18 +58,17 @@ export class ExpressionRenderHandler {
     this.element = element;
 
     this.eventsSubject = new Rx.Subject();
-    this.events$ = this.eventsSubject.asObservable().pipe(share());
+    this.events$ = this.eventsSubject.asObservable();
 
     this.onRenderError = onRenderError || defaultRenderErrorHandler;
 
     this.renderSubject = new Rx.BehaviorSubject(null as RenderId | null);
-    this.render$ = this.renderSubject.asObservable().pipe(
-      share(),
-      filter(_ => _ !== null)
-    ) as Observable<RenderId>;
+    this.render$ = this.renderSubject.asObservable().pipe(filter(_ => _ !== null)) as Observable<
+      RenderId
+    >;
 
     this.updateSubject = new Rx.Subject();
-    this.update$ = this.updateSubject.asObservable().pipe(share());
+    this.update$ = this.updateSubject.asObservable();
 
     this.handlers = {
       onDestroy: (fn: any) => {
diff --git a/src/plugins/kibana_react/public/index.ts b/src/plugins/kibana_react/public/index.ts
index 2d82f646c827b..46f330ea0a2c5 100644
--- a/src/plugins/kibana_react/public/index.ts
+++ b/src/plugins/kibana_react/public/index.ts
@@ -24,4 +24,4 @@ export * from './overlays';
 export * from './ui_settings';
 export * from './field_icon';
 export * from './table_list_view';
-export { toMountPoint } from './util';
+export { toMountPoint, useShallowCompareEffect } from './util';
diff --git a/src/plugins/kibana_react/public/util/index.ts b/src/plugins/kibana_react/public/util/index.ts
index 1053ca01603e3..4f64d6c9c81ab 100644
--- a/src/plugins/kibana_react/public/util/index.ts
+++ b/src/plugins/kibana_react/public/util/index.ts
@@ -20,3 +20,4 @@
 export * from './use_observable';
 export * from './use_unmount';
 export * from './react_mount';
+export * from './use_shallow_compare_effect';
diff --git a/src/plugins/kibana_react/public/util/use_shallow_compare_effect.test.ts b/src/plugins/kibana_react/public/util/use_shallow_compare_effect.test.ts
new file mode 100644
index 0000000000000..e5d9c44727c3a
--- /dev/null
+++ b/src/plugins/kibana_react/public/util/use_shallow_compare_effect.test.ts
@@ -0,0 +1,86 @@
+/*
+ * 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 { renderHook } from 'react-hooks-testing-library';
+import { useShallowCompareEffect } from './use_shallow_compare_effect';
+
+describe('useShallowCompareEffect', () => {
+  test("doesn't run effect on shallow change", () => {
+    const callback = jest.fn();
+    let deps = [1, { a: 'b' }, true];
+    const { rerender } = renderHook(() => useShallowCompareEffect(callback, deps));
+
+    expect(callback).toHaveBeenCalledTimes(1);
+    callback.mockClear();
+
+    // no change
+    rerender();
+    expect(callback).toHaveBeenCalledTimes(0);
+    callback.mockClear();
+
+    // no-change (new object with same properties)
+    deps = [1, { a: 'b' }, true];
+    rerender();
+    expect(callback).toHaveBeenCalledTimes(0);
+    callback.mockClear();
+
+    // change (new primitive value)
+    deps = [2, { a: 'b' }, true];
+    rerender();
+    expect(callback).toHaveBeenCalledTimes(1);
+    callback.mockClear();
+
+    // no-change
+    rerender();
+    expect(callback).toHaveBeenCalledTimes(0);
+    callback.mockClear();
+
+    // change (new primitive value)
+    deps = [1, { a: 'b' }, false];
+    rerender();
+    expect(callback).toHaveBeenCalledTimes(1);
+    callback.mockClear();
+
+    // change (new properties on object)
+    deps = [1, { a: 'c' }, false];
+    rerender();
+    expect(callback).toHaveBeenCalledTimes(1);
+    callback.mockClear();
+  });
+
+  test('runs effect on deep change', () => {
+    const callback = jest.fn();
+    let deps = [1, { a: { b: 'c' } }, true];
+    const { rerender } = renderHook(() => useShallowCompareEffect(callback, deps));
+
+    expect(callback).toHaveBeenCalledTimes(1);
+    callback.mockClear();
+
+    // no change
+    rerender();
+    expect(callback).toHaveBeenCalledTimes(0);
+    callback.mockClear();
+
+    // change (new nested object )
+    deps = [1, { a: { b: 'c' } }, true];
+    rerender();
+    expect(callback).toHaveBeenCalledTimes(1);
+    callback.mockClear();
+  });
+});
diff --git a/src/plugins/kibana_react/public/util/use_shallow_compare_effect.ts b/src/plugins/kibana_react/public/util/use_shallow_compare_effect.ts
new file mode 100644
index 0000000000000..dfba7b907f5fb
--- /dev/null
+++ b/src/plugins/kibana_react/public/util/use_shallow_compare_effect.ts
@@ -0,0 +1,80 @@
+/*
+ * 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 React, { useEffect, useRef } from 'react';
+
+/**
+ * Similar to https://github.com/kentcdodds/use-deep-compare-effect
+ * but uses shallow compare instead of deep
+ */
+export function useShallowCompareEffect(
+  callback: React.EffectCallback,
+  deps: React.DependencyList
+) {
+  useEffect(callback, useShallowCompareMemoize(deps));
+}
+function useShallowCompareMemoize(deps: React.DependencyList) {
+  const ref = useRef<React.DependencyList | undefined>(undefined);
+
+  if (!ref.current || deps.some((dep, index) => !shallowEqual(dep, ref.current![index]))) {
+    ref.current = deps;
+  }
+
+  return ref.current;
+}
+// https://github.com/facebook/fbjs/blob/master/packages/fbjs/src/core/shallowEqual.js
+function shallowEqual(objA: any, objB: any): boolean {
+  if (is(objA, objB)) {
+    return true;
+  }
+
+  if (typeof objA !== 'object' || objA === null || typeof objB !== 'object' || objB === null) {
+    return false;
+  }
+
+  const keysA = Object.keys(objA);
+  const keysB = Object.keys(objB);
+
+  if (keysA.length !== keysB.length) {
+    return false;
+  }
+
+  // Test for A's keys different from B.
+  for (let i = 0; i < keysA.length; i++) {
+    if (
+      !Object.prototype.hasOwnProperty.call(objB, keysA[i]) ||
+      !is(objA[keysA[i]], objB[keysA[i]])
+    ) {
+      return false;
+    }
+  }
+
+  return true;
+}
+
+/**
+ * IE11 does not support Object.is
+ */
+function is(x: any, y: any): boolean {
+  if (x === y) {
+    return x !== 0 || y !== 0 || 1 / x === 1 / y;
+  } else {
+    return x !== x && y !== y;
+  }
+}

From 9a3282f3ef806513bf1aecf6548a82b9763a39e0 Mon Sep 17 00:00:00 2001
From: Anton Dosov <anton.dosov@elastic.co>
Date: Tue, 26 Nov 2019 12:18:48 +0100
Subject: [PATCH 3/6] Render error inline for lens emedbale as it was previosly

---
 .../public/editor_frame_plugin/embeddable/expression_wrapper.tsx | 1 +
 1 file changed, 1 insertion(+)

diff --git a/x-pack/legacy/plugins/lens/public/editor_frame_plugin/embeddable/expression_wrapper.tsx b/x-pack/legacy/plugins/lens/public/editor_frame_plugin/embeddable/expression_wrapper.tsx
index 21a69bfc3a0b3..72d339b0a3d4b 100644
--- a/x-pack/legacy/plugins/lens/public/editor_frame_plugin/embeddable/expression_wrapper.tsx
+++ b/x-pack/legacy/plugins/lens/public/editor_frame_plugin/embeddable/expression_wrapper.tsx
@@ -50,6 +50,7 @@ export function ExpressionWrapper({
             padding="m"
             expression={expression}
             searchContext={{ ...context, type: 'kibana_context' }}
+            renderError={error => <div>{error}</div>}
           />
         </div>
       )}

From 4351781a5a5524d197df9c4b9a7ff5e6ae665aa0 Mon Sep 17 00:00:00 2001
From: Anton Dosov <anton.dosov@elastic.co>
Date: Tue, 26 Nov 2019 12:25:03 +0100
Subject: [PATCH 4/6] Add data-test-subj

---
 .../editor_frame_plugin/embeddable/expression_wrapper.tsx       | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/x-pack/legacy/plugins/lens/public/editor_frame_plugin/embeddable/expression_wrapper.tsx b/x-pack/legacy/plugins/lens/public/editor_frame_plugin/embeddable/expression_wrapper.tsx
index 72d339b0a3d4b..3dd4373347129 100644
--- a/x-pack/legacy/plugins/lens/public/editor_frame_plugin/embeddable/expression_wrapper.tsx
+++ b/x-pack/legacy/plugins/lens/public/editor_frame_plugin/embeddable/expression_wrapper.tsx
@@ -50,7 +50,7 @@ export function ExpressionWrapper({
             padding="m"
             expression={expression}
             searchContext={{ ...context, type: 'kibana_context' }}
-            renderError={error => <div>{error}</div>}
+            renderError={error => <div data-test-subj="expression-renderer-error">{error}</div>}
           />
         </div>
       )}

From cca211e6aa1e6979ab3810cd855a2a92c950562f Mon Sep 17 00:00:00 2001
From: Anton Dosov <anton.dosov@elastic.co>
Date: Tue, 26 Nov 2019 15:35:40 +0100
Subject: [PATCH 5/6] fix ts errors - change RenderResult to RenderId

---
 src/plugins/expressions/public/index.ts                     | 2 +-
 .../plugins/kbn_tp_run_pipeline/public/np_ready/types.ts    | 4 ++--
 .../test_suites/run_pipeline/helpers.ts                     | 6 +++---
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/plugins/expressions/public/index.ts b/src/plugins/expressions/public/index.ts
index 6dc88fd23f29a..951d643c9df68 100644
--- a/src/plugins/expressions/public/index.ts
+++ b/src/plugins/expressions/public/index.ts
@@ -29,7 +29,7 @@ export { interpreterProvider, ExpressionInterpret } from './interpreter_provider
 export { ExpressionRenderer, ExpressionRendererProps } from './expression_renderer';
 export { ExpressionDataHandler } from './execute';
 
-export { RenderResult, ExpressionRenderHandler } from './render';
+export { ExpressionRenderHandler } from './render';
 
 export function plugin(initializerContext: PluginInitializerContext) {
   return new ExpressionsPublicPlugin(initializerContext);
diff --git a/test/interpreter_functional/plugins/kbn_tp_run_pipeline/public/np_ready/types.ts b/test/interpreter_functional/plugins/kbn_tp_run_pipeline/public/np_ready/types.ts
index 082bb47d80066..cc4190bd099fa 100644
--- a/test/interpreter_functional/plugins/kbn_tp_run_pipeline/public/np_ready/types.ts
+++ b/test/interpreter_functional/plugins/kbn_tp_run_pipeline/public/np_ready/types.ts
@@ -22,7 +22,7 @@ import {
   Context,
   ExpressionRenderHandler,
   ExpressionDataHandler,
-  RenderResult,
+  RenderId,
 } from 'src/plugins/expressions/public';
 
 import { Adapters } from 'src/plugins/inspector/public';
@@ -32,6 +32,6 @@ export {
   Context,
   ExpressionRenderHandler,
   ExpressionDataHandler,
-  RenderResult,
+  RenderId,
   Adapters,
 };
diff --git a/test/interpreter_functional/test_suites/run_pipeline/helpers.ts b/test/interpreter_functional/test_suites/run_pipeline/helpers.ts
index e1ec18fae5e3a..7fedf1723908a 100644
--- a/test/interpreter_functional/test_suites/run_pipeline/helpers.ts
+++ b/test/interpreter_functional/test_suites/run_pipeline/helpers.ts
@@ -21,8 +21,8 @@ import expect from '@kbn/expect';
 import { FtrProviderContext } from '../../../functional/ftr_provider_context';
 import {
   ExpressionDataHandler,
-  RenderResult,
   Context,
+  RenderId,
 } from '../../plugins/kbn_tp_run_pipeline/public/np_ready/types';
 
 type UnWrapPromise<T> = T extends Promise<infer U> ? U : T;
@@ -168,8 +168,8 @@ export function expectExpressionProvider({
       toMatchScreenshot: async () => {
         const pipelineResponse = await handler.getResponse();
         log.debug('starting to render');
-        const result = await browser.executeAsync<RenderResult>(
-          (_context: ExpressionResult, done: (renderResult: RenderResult) => void) =>
+        const result = await browser.executeAsync<RenderId>(
+          (_context: ExpressionResult, done: (renderResult: RenderId) => void) =>
             window.renderPipelineResponse(_context).then(renderResult => {
               done(renderResult);
               return renderResult;

From 4b730df923c4b356dd64dbbf2183e3f77bbd4075 Mon Sep 17 00:00:00 2001
From: Anton Dosov <anton.dosov@elastic.co>
Date: Tue, 26 Nov 2019 16:27:11 +0100
Subject: [PATCH 6/6] fix types

---
 .../public/np_ready/app/components/main.tsx                   | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/interpreter_functional/plugins/kbn_tp_run_pipeline/public/np_ready/app/components/main.tsx b/test/interpreter_functional/plugins/kbn_tp_run_pipeline/public/np_ready/app/components/main.tsx
index eec261c985732..daa19f22a7023 100644
--- a/test/interpreter_functional/plugins/kbn_tp_run_pipeline/public/np_ready/app/components/main.tsx
+++ b/test/interpreter_functional/plugins/kbn_tp_run_pipeline/public/np_ready/app/components/main.tsx
@@ -29,7 +29,7 @@ import {
   Context,
   ExpressionRenderHandler,
   ExpressionDataHandler,
-  RenderResult,
+  RenderId,
 } from '../../types';
 import { getExpressions } from '../../services';
 
@@ -40,7 +40,7 @@ declare global {
       context?: Context,
       initialContext?: Context
     ) => ReturnType<ExpressionDataHandler['getData']>;
-    renderPipelineResponse: (context?: Context) => Promise<RenderResult>;
+    renderPipelineResponse: (context?: Context) => Promise<RenderId>;
   }
 }