Expressions service fixes: better error and loading states handling

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', () => {

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,68 +56,49 @@ 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();
     const render$ = renderSubject.asObservable().pipe(share());
     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);
   });
 });

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<ExpressionRendererProps>;
@@ -53,73 +56,94 @@ 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 });
+  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);
 
-  // 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<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 (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 (
     <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.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"
         style={expressionStyles}

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);