Fix: recreate handlers and reset completed state on expression change (#33900)

* fix: add expression and filter to ElementWrapper propso

cause the component to re-render when these values change

* fix: correctly spread additional props

* chore: convert ElementWrapper to functional component

* chore: refactor ElementWrapper container

use connectAdvanced instead of connect since it provides a way to get a hold of dispatch just one time, so the handlers object can be built in the container and only updated when something actually changes

* fix: reset handlers isComplete when element changes

allow completeFn to be called again, required so that the correct external actions happen

* feat: make expression available on shapes object

* fix: reset done checker on function change

* fix: only rebuild handlers when element changes

rebuild on happens when id, filter, or expression change

* chore: remove unused ElementWrapper props

x-pack/plugins/canvas/public/components/element_content/element_content.js

@@ -24,6 +24,7 @@ const branches = [
   branch(({ renderable, state }) => {
     return !state || !renderable;
   }, renderComponent(({ backgroundColor }) => <Loading backgroundColor={backgroundColor} />)),
+
   // renderable is available, but no matching element is found, render invalid
   branch(({ renderable, renderFunction }) => {
     return renderable && getType(renderable) !== 'render' && !renderFunction;

x-pack/plugins/canvas/public/components/element_share_container/element_share_container.js

@@ -20,30 +20,21 @@ export class ElementShareContainer extends React.PureComponent {
   };
 
   componentDidMount() {
-    const { functionName, onComplete } = this.props;
-    const isDevelopment = process.env.NODE_ENV !== 'production';
+    const { onComplete } = this.props;
 
     // check that the done event is called within a certain time
-    if (isDevelopment) {
-      const timeout = 15; // timeout, in seconds
-      this.timeout = setTimeout(() => {
-        // TODO: show this message in a proper notification
-        console.warn(
-          `done handler not called in render function after ${timeout} seconds: ${functionName}`
-        );
-      }, timeout * 1000);
-    }
+    this.createDoneChecker();
 
     // dispatches a custom DOM event on the container when the element is complete
     onComplete(() => {
-      clearTimeout(this.timeout);
+      this.clearDoneChecker();
       if (!this.sharedItemRef) {
         return;
       } // without this, crazy fast forward/backward paging leads to an error
       const ev = new CustomEvent('renderComplete');
       this.sharedItemRef.dispatchEvent(ev);
 
-      // if the element is finished before reporting is listening for then
+      // if the element is finished before reporting is listening for the
       // renderComplete event, the report never completes. to get around that
       // issue, track the completed state locally and set the
       // [data-render-complete] value accordingly.
@@ -53,10 +44,42 @@ export class ElementShareContainer extends React.PureComponent {
     });
   }
 
-  componentWillUnmount() {
-    clearTimeout(this.timeout);
+  getSnapshotBeforeUpdate(prevProps) {
+    return { functionName: prevProps.functionName };
   }
 
+  componentDidUpdate(prevProps, prevState, snapshot) {
+    // if function name changed, clear and recreate done checker
+    if (snapshot.functionName !== this.props.functionName) {
+      this.clearDoneChecker();
+      this.createDoneChecker();
+    }
+  }
+
+  componentWillUnmount() {
+    this.clearDoneChecker();
+  }
+
+  createDoneChecker = () => {
+    const isDevelopment = process.env.NODE_ENV !== 'production';
+    if (!isDevelopment) {
+      return;
+    }
+
+    const { functionName } = this.props;
+    const timeout = 15; // timeout, in seconds
+    this.timeout = setTimeout(() => {
+      // TODO: show this message in a proper notification
+      console.warn(
+        `done handler not called in render function after ${timeout} seconds: ${functionName}`
+      );
+    }, timeout * 1000);
+  };
+
+  clearDoneChecker = () => {
+    clearTimeout(this.timeout);
+  };
+
   render() {
     // NOTE: the data-shared-item and data-render-complete attributes are used for reporting
     return (

x-pack/plugins/canvas/public/components/element_wrapper/element_wrapper.js

@@ -9,30 +9,23 @@ import PropTypes from 'prop-types';
 import { Positionable } from '../positionable';
 import { ElementContent } from '../element_content';
 
-export class ElementWrapper extends React.PureComponent {
-  static propTypes = {
-    renderable: PropTypes.object,
-    transformMatrix: PropTypes.arrayOf(PropTypes.number).isRequired,
-    width: PropTypes.number.isRequired,
-    height: PropTypes.number.isRequired,
-    state: PropTypes.string,
-    createHandlers: PropTypes.func.isRequired,
-  };
+export const ElementWrapper = props => {
+  const { renderable, transformMatrix, width, height, state, handlers } = props;
 
-  constructor(props) {
-    super(props);
-    this._handlers = props.createHandlers(props.selectedPage);
-  }
+  return (
+    <Positionable transformMatrix={transformMatrix} width={width} height={height}>
+      <ElementContent renderable={renderable} state={state} handlers={handlers} />
+    </Positionable>
+  );
+};
 
-  _handlers = null;
-
-  render() {
-    const { renderable, transformMatrix, width, height, state } = this.props;
-
-    return (
-      <Positionable transformMatrix={transformMatrix} width={width} height={height}>
-        <ElementContent renderable={renderable} state={state} handlers={this._handlers} />
-      </Positionable>
-    );
-  }
-}
+ElementWrapper.propTypes = {
+  // positionable props (from element object)
+  transformMatrix: PropTypes.arrayOf(PropTypes.number).isRequired,
+  width: PropTypes.number.isRequired,
+  height: PropTypes.number.isRequired,
+  // ElementContent pass-through props
+  renderable: PropTypes.object,
+  state: PropTypes.string,
+  handlers: PropTypes.object.isRequired,
+};

x-pack/plugins/canvas/public/components/element_wrapper/index.js

@@ -5,52 +5,73 @@
  */
 
 import PropTypes from 'prop-types';
-import { connect } from 'react-redux';
-import { compose, shouldUpdate } from 'recompose';
+import { connectAdvanced } from 'react-redux';
+import { compose, withPropsOnChange, mapProps } from 'recompose';
 import isEqual from 'react-fast-compare';
 import { getResolvedArgs, getSelectedPage } from '../../state/selectors/workpad';
-import { getState, getValue, getError } from '../../lib/resolved_arg';
+import { getState, getValue } from '../../lib/resolved_arg';
 import { ElementWrapper as Component } from './element_wrapper';
 import { createHandlers as createHandlersWithDispatch } from './lib/handlers';
 
-const mapStateToProps = (state, { element }) => ({
-  resolvedArg: getResolvedArgs(state, element.id, 'expressionRenderable'),
-  selectedPage: getSelectedPage(state),
-});
+function selectorFactory(dispatch) {
+  let result = {};
+  const createHandlers = createHandlersWithDispatch(dispatch);
 
-const mapDispatchToProps = (dispatch, { element }) => ({
-  createHandlers: pageId => createHandlersWithDispatch(element, pageId, dispatch),
-});
+  return (nextState, nextOwnProps) => {
+    const { element, ...restOwnProps } = nextOwnProps;
+    const { transformMatrix, width, height } = element;
 
-const mergeProps = (stateProps, dispatchProps, ownProps) => {
-  const { resolvedArg, selectedPage } = stateProps;
-  const { element, restProps } = ownProps;
-  const { id, transformMatrix, width, height } = element;
+    const resolvedArg = getResolvedArgs(nextState, element.id, 'expressionRenderable');
+    const selectedPage = getSelectedPage(nextState);
 
-  return {
-    selectedPage,
-    ...restProps, // pass through unused props
-    id, //pass through useful parts of the element object
-    transformMatrix,
-    width,
-    height,
-    state: getState(resolvedArg),
-    error: getError(resolvedArg),
-    renderable: getValue(resolvedArg),
-    createHandlers: dispatchProps.createHandlers,
+    // build interim props object
+    const nextResult = {
+      ...restOwnProps,
+      // state and state-derived props
+      selectedPage,
+      state: getState(resolvedArg),
+      renderable: getValue(resolvedArg),
+      // pass along the handlers creation function
+      createHandlers,
+      // required parts of the element object
+      transformMatrix,
+      width,
+      height,
+      // pass along only the useful parts of the element object
+      // so handlers object can be created
+      element: {
+        id: element.id,
+        filter: element.filter,
+        expression: element.expression,
+      },
+    };
+
+    // update props only if something actually changed
+    if (!isEqual(result, nextResult)) {
+      result = nextResult;
+    }
+
+    return result;
   };
-};
+}
 
 export const ElementWrapper = compose(
-  connect(
-    mapStateToProps,
-    mapDispatchToProps,
-    mergeProps,
-    {
-      areOwnPropsEqual: isEqual,
+  connectAdvanced(selectorFactory),
+  withPropsOnChange(
+    (props, nextProps) => !isEqual(props.element, nextProps.element),
+    props => {
+      const { element, createHandlers } = props;
+      const handlers = createHandlers(element, props.selectedPage);
+      // this removes element and createHandlers from passed props
+      return { handlers };
     }
   ),
-  shouldUpdate((props, nextProps) => !isEqual(props, nextProps))
+  mapProps(props => {
+    // remove elements and createHandlers from props passed to component
+    // eslint-disable-next-line no-unused-vars
+    const { element, createHandlers, selectedPage, ...restProps } = props;
+    return restProps;
+  })
 )(Component);
 
 ElementWrapper.propTypes = {
@@ -59,5 +80,9 @@ ElementWrapper.propTypes = {
     transformMatrix: PropTypes.arrayOf(PropTypes.number).isRequired,
     width: PropTypes.number.isRequired,
     height: PropTypes.number.isRequired,
+    // sometimes we get a shape, which lacks an expression
+    // so element properties can not be marked as required
+    expression: PropTypes.string,
+    filter: PropTypes.string,
   }).isRequired,
 };

x-pack/plugins/canvas/public/components/element_wrapper/lib/handlers.js

@@ -4,31 +4,43 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
+import { isEqual } from 'lodash';
 import { setFilter } from '../../../state/actions/elements';
 
-export function createHandlers(element, pageId, dispatch) {
+export const createHandlers = dispatch => {
   let isComplete = false;
+  let oldElement;
   let completeFn = () => {};
 
-  return {
-    setFilter(text) {
-      dispatch(setFilter(text, element.id, pageId, true));
-    },
+  return (element, pageId) => {
+    // reset isComplete when element changes
+    if (!isEqual(oldElement, element)) {
+      isComplete = false;
+      oldElement = element;
+    }
 
-    getFilter() {
-      return element.filter;
-    },
+    return {
+      setFilter(text) {
+        dispatch(setFilter(text, element.id, pageId, true));
+      },
 
-    onComplete(fn) {
-      completeFn = fn;
-    },
+      getFilter() {
+        return element.filter;
+      },
 
-    done() {
-      if (isComplete) {
-        return;
-      } // don't emit if the element is already done
-      isComplete = true;
-      completeFn();
-    },
+      onComplete(fn) {
+        completeFn = fn;
+      },
+
+      done() {
+        // don't emit if the element is already done
+        if (isComplete) {
+          return;
+        }
+
+        isComplete = true;
+        completeFn();
+      },
+    };
   };
-}
+};

x-pack/plugins/canvas/public/components/workpad_page/index.js

@@ -115,7 +115,7 @@ const layoutProps = ({ forceUpdate, page, elements: pageElements }) => {
       }
     }
     // instead of just combining `element` with `shape`, we make property transfer explicit
-    return element ? { ...shape, filter: element.filter } : shape;
+    return element ? { ...shape, filter: element.filter, expression: element.expression } : shape;
   });
   return {
     elements,