[7.x] Fix: Re-render renderers on filter changes (#34823) (#36823)

* fix: rebuild elementHandlers when handlers change

and call renderer render function when handlers change

* fix: remove state from TimeFilter

filter changes always come in as props, so the component can be stateless, which fixes all the weird sync issues that have shown up

x-pack/plugins/canvas/canvas_plugin_src/renderers/time_filter/components/time_filter/time_filter.js

@@ -4,62 +4,37 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import React, { Component } from 'react';
+import React from 'react';
 import PropTypes from 'prop-types';
 import { get } from 'lodash';
 import { fromExpression } from '@kbn/interpreter/common';
 import { TimePicker } from '../time_picker';
 import { TimePickerMini } from '../time_picker_mini';
 
-export class TimeFilter extends Component {
-  static propTypes = {
-    filter: PropTypes.string,
-    commit: PropTypes.func, // Canvas filter
-    compact: PropTypes.bool,
-  };
-
-  state = {
-    filter: this.props.filter,
-  };
-
-  shouldComponentUpdate(nextProps, nextState) {
-    const nextPropsColumn = get(fromExpression(nextProps.filter), 'chain[0].arguments.column[0]');
-    const ast = fromExpression(this.state.filter);
-    const from = get(ast, 'chain[0].arguments.from[0]');
-    const to = get(ast, 'chain[0].arguments.to[0]');
-    const column = get(ast, 'chain[0].arguments.column[0]');
-
-    // if the column in the prop filter changes, we need to update the column in state
-    // while preserving the date ranges in state
-    if (column !== nextPropsColumn) {
-      this.setFilter(nextPropsColumn)(from, to);
-      return true;
-    }
-
-    return this.state.filter !== nextState.filter;
-  }
-
-  setFilter = column => (from, to) => {
-    const { commit } = this.props;
-    const filter = `timefilter from="${from}" to=${to} column=${column}`;
-
-    // TODO: Changes to element.filter do not cause a re-render
-    if (filter !== this.state.filter) {
-      this.setState({ filter });
-      commit(filter);
-    }
-  };
-
-  render() {
-    const ast = fromExpression(this.state.filter);
-    const from = get(ast, 'chain[0].arguments.from[0]');
-    const to = get(ast, 'chain[0].arguments.to[0]');
-    const column = get(ast, 'chain[0].arguments.column[0]');
-
-    if (this.props.compact) {
-      return <TimePickerMini from={from} to={to} onSelect={this.setFilter(column)} />;
-    } else {
-      return <TimePicker from={from} to={to} onSelect={this.setFilter(column)} />;
-    }
-  }
+function getFilterMeta(filter) {
+  const ast = fromExpression(filter);
+  const column = get(ast, 'chain[0].arguments.column[0]');
+  const from = get(ast, 'chain[0].arguments.from[0]');
+  const to = get(ast, 'chain[0].arguments.to[0]');
+  return { column, from, to };
 }
+
+export const TimeFilter = ({ filter, commit, compact }) => {
+  const setFilter = column => (from, to) => {
+    commit(`timefilter from="${from}" to=${to} column=${column}`);
+  };
+
+  const { column, from, to } = getFilterMeta(filter);
+
+  if (compact) {
+    return <TimePickerMini from={from} to={to} onSelect={setFilter(column)} />;
+  } else {
+    return <TimePicker from={from} to={to} onSelect={setFilter(column)} />;
+  }
+};
+
+TimeFilter.propTypes = {
+  filter: PropTypes.string,
+  commit: PropTypes.func, // Canvas filter
+  compact: PropTypes.bool,
+};

x-pack/plugins/canvas/canvas_plugin_src/renderers/time_filter/index.js

@@ -18,8 +18,9 @@ export const timeFilter = () => ({
   render(domNode, config, handlers) {
     const ast = fromExpression(handlers.getFilter());
 
-    // Check if the current column is what we expect it to be. If the user changes column this will be called again,
-    // but we don't want to run setFilter() unless we have to because it will cause a data refresh
+    // Check if the current column is what we expect it to be. If the user changes column this
+    // will be called again, but we don't want to run setFilter() unless we have to because it
+    // will cause a data refresh
     const column = get(ast, 'chain[0].arguments.column[0]');
     if (column !== config.column) {
       set(ast, 'chain[0].arguments.column[0]', config.column);

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

@@ -67,7 +67,7 @@ export const ElementWrapper = compose(
     }
   ),
   mapProps(props => {
-    // remove elements and createHandlers from props passed to component
+    // remove element and createHandlers from props passed to component
     // eslint-disable-next-line no-unused-vars
     const { element, createHandlers, selectedPage, ...restProps } = props;
     return restProps;

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

@@ -4,26 +4,26 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import { compose, withPropsOnChange, withProps } from 'recompose';
+import { compose, withProps, withPropsOnChange } from 'recompose';
 import PropTypes from 'prop-types';
+import isEqual from 'react-fast-compare';
 import { notify } from '../../lib/notify';
 import { RenderWithFn as Component } from './render_with_fn';
 import { ElementHandlers } from './lib/handlers';
 
 export const RenderWithFn = compose(
   withPropsOnChange(
-    () => false,
-    () => ({
-      elementHandlers: new ElementHandlers(),
+    // rebuild elementHandlers when handlers object changes
+    (props, nextProps) => !isEqual(props.handlers, nextProps.handlers),
+    ({ handlers }) => ({
+      handlers: Object.assign(new ElementHandlers(), handlers),
     })
   ),
-  withProps(({ handlers, elementHandlers }) => ({
-    handlers: Object.assign(elementHandlers, handlers),
+  withProps({
     onError: notify.error,
-  }))
+  })
 )(Component);
 
 RenderWithFn.propTypes = {
   handlers: PropTypes.object,
-  elementHandlers: PropTypes.object,
 };

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

@@ -120,9 +120,11 @@ export class RenderWithFn extends React.Component {
   };
 
   _shouldFullRerender = prevProps => {
-    // TODO: What a shitty hack. None of these props should update when you move the element.
-    // This should be fixed at a higher level.
+    // required to stop re-renders on element move, anything that should
+    // cause a re-render needs to be checked here
+    // TODO: fix props passed in to remove this check
     return (
+      this.props.handlers !== prevProps.handlers ||
       !isEqual(this.props.config, prevProps.config) ||
       !isEqual(this.props.renderFn.toString(), prevProps.renderFn.toString())
     );