diff --git a/src/plugins/expressions/public/execute.test.ts b/src/plugins/expressions/public/execute.test.ts index ec0bbfebb1093..9e2bb322ad5ff 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(); - 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(); - - 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( - +
{message}
} + /> ); - 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..3989f4ed7d698 100644 --- a/src/plugins/expressions/public/expression_renderer.tsx +++ b/src/plugins/expressions/public/expression_renderer.tsx @@ -17,12 +17,15 @@ * 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 { useShallowCompareEffect } from '../../kibana_react/public'; +import { IExpressionLoaderParams, IInterpreterRenderHandlers, RenderError } from './types'; import { ExpressionAST } from '../common/types'; import { ExpressionLoader } from './loader'; @@ -39,7 +42,7 @@ export interface ExpressionRendererProps extends IExpressionLoaderParams { interface State { isEmpty: boolean; isLoading: boolean; - error: null | { message: string }; + error: null | RenderError; } export type ExpressionRenderer = React.FC; @@ -53,73 +56,94 @@ const defaultState: State = { export const ExpressionRendererImplementation = ({ className, dataAttrs, - expression, - renderError, padding, - ...options + renderError, + expression, + ...expressionLoaderOptions }: ExpressionRendererProps) => { const mountpoint: React.MutableRefObject = useRef(null); - const handlerRef: React.MutableRefObject = useRef(null); const [state, setState] = useState({ ...defaultState }); + const hasCustomRenderErrorHandler = !!renderError; + const expressionLoaderRef: React.MutableRefObject = useRef(null); + // flag to skip next render$ notification, + // because of just handled error + const hasHandledErrorRef = useRef(false); - // 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 */ + // will call done() in LayoutEffect when done with rendering custom error state + const errorRenderHandlerRef: React.MutableRefObject = 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 (mountpoint.current && !handlerRef.current) { - handlerRef.current = new ExpressionLoader(mountpoint.current, expression, options); + const subs: Subscription[] = []; + 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 + onRenderError: hasCustomRenderErrorHandler + ? (domNode, error, handlers) => { + errorRenderHandlerRef.current = handlers; + setState(() => ({ + ...defaultState, + isEmpty: false, + error, + })); - handlerRef.current.loading$.subscribe(() => { - if (!handlerRef.current) { - return; - } + if (expressionLoaderOptions.onRenderError) { + expressionLoaderOptions.onRenderError(domNode, error, handlers); + } + } + : expressionLoaderOptions.onRenderError, + }); + subs.push( + expressionLoaderRef.current.loading$.subscribe(() => { + hasHandledErrorRef.current = false; setState(prevState => ({ ...prevState, isLoading: true })); - }); - handlerRef.current.render$.subscribe(item => { - if (!handlerRef.current) { - return; - } - if (typeof item !== 'number') { + }), + expressionLoaderRef.current.render$ + .pipe(filter(() => !hasHandledErrorRef.current)) + .subscribe(item => { setState(() => ({ ...defaultState, isEmpty: false, - error: item.error, })); - } else { - setState(() => ({ - ...defaultState, - isEmpty: false, - })); - } - }); - } - /* eslint-disable */ - // TODO: Replace mountpoint.current by something else. - }, [mountpoint.current]); - /* eslint-enable */ + }) + ); - useEffect(() => { - // We only want a clean up to run when the entire component is unloaded, not on every render - return function cleanup() { - if (handlerRef.current) { - handlerRef.current.destroy(); - handlerRef.current = null; + return () => { + subs.forEach(s => s.unsubscribe()); + if (expressionLoaderRef.current) { + expressionLoaderRef.current.destroy(); + expressionLoaderRef.current = null; } + + errorRenderHandlerRef.current = null; }; - }, []); + }, [hasCustomRenderErrorHandler]); + + // 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) { + hasHandledErrorRef.current = true; + errorRenderHandlerRef.current.done(); + errorRenderHandlerRef.current = null; + } + }, [state.error]); const classes = classNames('expExpressionRenderer', { 'expExpressionRenderer-isEmpty': state.isEmpty, @@ -135,15 +159,9 @@ export const ExpressionRendererImplementation = ({ return (
- {state.isEmpty ? : null} - {state.isLoading ? : null} - {!state.isLoading && state.error ? ( - renderError ? ( - renderError(state.error.message) - ) : ( -
{state.error.message}
- ) - ) : null} + {state.isEmpty && } + {state.isLoading && } + {!state.isLoading && state.error && renderError && renderError(state.error.message)}
{ getRenderersRegistry: () => ({ get: (id: string) => renderers[id], }), + getNotifications: jest.fn(() => { + return { + toasts: { + addError: jest.fn(() => {}), + }, + }; + }), }; }); @@ -97,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 200249b60c773..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; - private loadingSubject: Subject; + private loadingSubject: Subject; private data: Data; private params: IExpressionLoaderParams = {}; @@ -46,12 +46,20 @@ export class ExpressionLoader { params?: IExpressionLoaderParams ) { this.dataSubject = new Subject(); - this.data$ = this.dataSubject.asObservable().pipe(share()); - - this.loadingSubject = new Subject(); - this.loading$ = this.loadingSubject.asObservable().pipe(share()); - - this.renderHandler = new ExpressionRenderHandler(element); + this.data$ = this.dataSubject.asObservable(); + + this.loadingSubject = new BehaviorSubject(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, + }); this.render$ = this.renderHandler.render$; this.update$ = this.renderHandler.update$; this.events$ = this.renderHandler.events$; @@ -64,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); } } @@ -120,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/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 = { 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 3c7008806e779..62bde12490fbe 100644 --- a/src/plugins/expressions/public/render.ts +++ b/src/plugins/expressions/public/render.ts @@ -17,48 +17,58 @@ * under the License. */ -import { Observable } from 'rxjs'; import * as Rx from 'rxjs'; -import { filter, share } from 'rxjs/operators'; -import { event, RenderId, Data, IInterpreterRenderHandlers } from './types'; +import { Observable } from 'rxjs'; +import { filter } from 'rxjs/operators'; +import { + Data, + event, + IInterpreterRenderHandlers, + RenderError, + RenderErrorHandlerFnType, + RenderId, +} 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; -export type RenderResult = RenderId | RenderError; +export interface ExpressionRenderHandlerParams { + onRenderError: RenderErrorHandlerFnType; +} export class ExpressionRenderHandler { - render$: Observable; + render$: Observable; update$: Observable; events$: Observable; private element: HTMLElement; private destroyFn?: any; private renderCount: number = 0; - private renderSubject: Rx.BehaviorSubject; + private renderSubject: Rx.BehaviorSubject; private eventsSubject: Rx.Subject; private updateSubject: Rx.Subject; private handlers: IInterpreterRenderHandlers; + private onRenderError: RenderErrorHandlerFnType; - constructor(element: HTMLElement) { + constructor( + element: HTMLElement, + { onRenderError }: Partial = {} + ) { 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 RenderResult | null); - this.render$ = this.renderSubject.asObservable().pipe( - share(), - filter(_ => _ !== null) - ) as Observable; + this.renderSubject = new Rx.BehaviorSubject(null as RenderId | null); + 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) => { @@ -82,33 +92,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 { @@ -117,10 +115,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); } }; @@ -136,10 +131,18 @@ 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 { - const handler = new ExpressionRenderHandler(element); +export function render( + element: HTMLElement, + data: Data, + options?: Partial +): ExpressionRenderHandler { + const handler = new ExpressionRenderHandler(element, options); handler.render(data); return handler; } 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('Insp export const [getInterpreter, setInterpreter] = createGetterSetter( 'Interpreter' ); +export const [getNotifications, setNotifications] = createGetterSetter( + '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[0], @@ -58,6 +59,7 @@ export interface IExpressionLoaderParams { customRenderers?: []; extraHandlers?: Record; 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; 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(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; + } +} 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 c091765619a19..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; - renderPipelineResponse: (context?: Context) => Promise; + renderPipelineResponse: (context?: Context) => Promise; } } @@ -85,16 +85,16 @@ class Main extends React.Component<{}, State> { lastRenderHandler.destroy(); } - lastRenderHandler = getExpressions().render(this.chartRef.current!, context); - const renderResult = await lastRenderHandler.render$.pipe(first()).toPromise(); + lastRenderHandler = getExpressions().render(this.chartRef.current!, context, { + onRenderError: (el, error, handler) => { + this.setState({ + expression: 'Render error!\n\n' + JSON.stringify(error), + }); + handler.done(); + }, + }); - if (typeof renderResult === 'object' && renderResult.type === 'error') { - this.setState({ - expression: 'Render error!\n\n' + JSON.stringify(renderResult.error), - }); - } - - return renderResult; + return lastRenderHandler.render$.pipe(first()).toPromise(); }; } 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 extends Promise ? 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( - (_context: ExpressionResult, done: (renderResult: RenderResult) => void) => + const result = await browser.executeAsync( + (_context: ExpressionResult, done: (renderResult: RenderId) => void) => window.renderPipelineResponse(_context).then(renderResult => { done(renderResult); return renderResult; 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..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,6 +50,7 @@ export function ExpressionWrapper({ padding="m" expression={expression} searchContext={{ ...context, type: 'kibana_context' }} + renderError={error =>
{error}
} />
)}