Expressions service fixes: better error and loading states handling (e…

…lastic#51183) 1. This pr fixes regression in v7.6 - elastic#51153. Visualisation which are using ExpressionLoader directly are swallowing render errors in 7.6, because generic error handling is happening only on expression_renderer.tsx level (react component wrapper around renderer). Now react agnostic render code render.ts, loader.ts: has it's own default error handler which is toast from notification service. It also allows to pass custom error handler instead of default one React expression renderer expression_renderer.tsx: if consumer wants to render error as react component inline, then the component uses custom error handler from render.ts to wire up react component. 2. This pr fixes issue of loader.ts where initial loading$ was not emitted Had to change loadingSubject from Subject to BehaviorSubject to remember the last value 3. This pr fixes dependencies in effects of expression_renderer.tsx # Conflicts: # src/plugins/expressions/public/loader.test.ts
Dosant · Nov 27, 2019 · bbcebe0 · bbcebe0
1 parent a44ccb7
commit bbcebe0
Show file tree

Hide file tree

Showing 20 changed files with 548 additions and 234 deletions.
diff --git 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
@@ -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);
   });
 });
diff --git 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<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}

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