superset-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From kri...@apache.org
Subject [incubator-superset] branch master updated: [Bug Fix]Prevent re-rendering when non-instant controls change (#6483)
Date Thu, 06 Dec 2018 01:32:05 GMT
This is an automated email from the ASF dual-hosted git repository.

kristw pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-superset.git


The following commit(s) were added to refs/heads/master by this push:
     new fc8acf2  [Bug Fix]Prevent re-rendering when non-instant controls change (#6483)
fc8acf2 is described below

commit fc8acf27c82ef3030d641289fa80f926f2a83b76
Author: Conglei <shiconglei@gmail.com>
AuthorDate: Wed Dec 5 17:31:59 2018 -0800

    [Bug Fix]Prevent re-rendering when non-instant controls change (#6483)
    
    fix: Prevent re-rendering when non-instant controls change
---
 superset/assets/src/chart/Chart.jsx                | 120 ++--------------
 .../src/chart/{Chart.jsx => ChartRenderer.jsx}     | 156 ++++++++-------------
 superset/assets/src/chart/chartReducer.js          |   6 +-
 .../assets/src/components/RefreshChartOverlay.jsx  |   7 -
 .../src/explore/components/ExploreChartPanel.jsx   |   4 +-
 .../explore/components/ExploreViewContainer.jsx    |   6 +-
 .../assets/src/explore/reducers/exploreReducer.js  |   5 +-
 superset/assets/src/logger.js                      |   2 +
 8 files changed, 87 insertions(+), 219 deletions(-)

diff --git a/superset/assets/src/chart/Chart.jsx b/superset/assets/src/chart/Chart.jsx
index f5aa347..e6fc230 100644
--- a/superset/assets/src/chart/Chart.jsx
+++ b/superset/assets/src/chart/Chart.jsx
@@ -1,15 +1,11 @@
-import dompurify from 'dompurify';
-import { snakeCase } from 'lodash';
 import PropTypes from 'prop-types';
 import React from 'react';
-import { Tooltip } from 'react-bootstrap';
-import { ChartProps } from '@superset-ui/chart';
-import { Logger, LOG_ACTIONS_RENDER_CHART } from '../logger';
+import { Logger, LOG_ACTIONS_RENDER_CHART_CONTAINER } from '../logger';
 import Loading from '../components/Loading';
 import RefreshChartOverlay from '../components/RefreshChartOverlay';
 import StackTraceMessage from '../components/StackTraceMessage';
-import SuperChart from '../visualizations/core/components/SuperChart';
 import ErrorBoundary from '../components/ErrorBoundary';
+import ChartRenderer from './ChartRenderer';
 import './chart.css';
 
 const propTypes = {
@@ -24,6 +20,7 @@ const propTypes = {
   setControlValue: PropTypes.func,
   timeout: PropTypes.number,
   vizType: PropTypes.string.isRequired,
+  triggerRender: PropTypes.bool,
   // state
   chartAlert: PropTypes.string,
   chartStatus: PropTypes.string,
@@ -35,7 +32,6 @@ const propTypes = {
   // dashboard callbacks
   addFilter: PropTypes.func,
   onQuery: PropTypes.func,
-  onDismissRefreshOverlay: PropTypes.func,
 };
 
 const BLANK = {};
@@ -44,20 +40,10 @@ const defaultProps = {
   addFilter: () => BLANK,
   filters: BLANK,
   setControlValue() {},
+  triggerRender: false,
 };
 
 class Chart extends React.PureComponent {
-  constructor(props) {
-    super(props);
-    this.state = {};
-
-    this.createChartProps = ChartProps.createSelector();
-    this.handleAddFilter = this.handleAddFilter.bind(this);
-    this.handleRenderSuccess = this.handleRenderSuccess.bind(this);
-    this.handleRenderFailure = this.handleRenderFailure.bind(this);
-    this.setTooltip = this.setTooltip.bind(this);
-  }
-
   componentDidMount() {
     if (this.props.triggerQuery) {
       this.props.actions.runQuery(
@@ -69,34 +55,12 @@ class Chart extends React.PureComponent {
     }
   }
 
-  setTooltip(tooltip) {
-    this.setState({ tooltip });
-  }
-
-  handleAddFilter(col, vals, merge = true, refresh = true) {
-    this.props.addFilter(col, vals, merge, refresh);
-  }
-
-  handleRenderSuccess() {
-    const { actions, chartStatus, chartId, vizType } = this.props;
-    if (['loading', 'rendered'].indexOf(chartStatus) < 0) {
-      actions.chartRenderingSucceeded(chartId);
-    }
-
-    Logger.append(LOG_ACTIONS_RENDER_CHART, {
-      slice_id: chartId,
-      viz_type: vizType,
-      start_offset: this.renderStartTime,
-      duration: Logger.getTimestamp() - this.renderStartTime,
-    });
-  }
-
   handleRenderFailure(error, info) {
     const { actions, chartId } = this.props;
     console.warn(error); // eslint-disable-line
     actions.chartRenderingFailed(error.toString(), chartId, info ? info.componentStack :
null);
 
-    Logger.append(LOG_ACTIONS_RENDER_CHART, {
+    Logger.append(LOG_ACTIONS_RENDER_CHART_CONTAINER, {
       slice_id: chartId,
       has_err: true,
       error_details: error.toString(),
@@ -105,57 +69,6 @@ class Chart extends React.PureComponent {
     });
   }
 
-  prepareChartProps() {
-    const {
-      width,
-      height,
-      annotationData,
-      datasource,
-      filters,
-      formData,
-      queryResponse,
-      setControlValue,
-    } = this.props;
-
-    return this.createChartProps({
-      width,
-      height,
-      annotationData,
-      datasource,
-      filters,
-      formData,
-      onAddFilter: this.handleAddFilter,
-      onError: this.handleRenderFailure,
-      payload: queryResponse,
-      setControlValue,
-      setTooltip: this.setTooltip,
-    });
-  }
-
-  renderTooltip() {
-    const { tooltip } = this.state;
-    if (tooltip && tooltip.content) {
-      return (
-        <Tooltip
-          className="chart-tooltip"
-          id="chart-tooltip"
-          placement="right"
-          positionTop={tooltip.y + 30}
-          positionLeft={tooltip.x + 30}
-          arrowOffsetTop={10}
-        >
-          {typeof (tooltip.content) === 'string' ?
-            <div // eslint-disable-next-line react/no-danger
-              dangerouslySetInnerHTML={{ __html: dompurify.sanitize(tooltip.content) }}
-            />
-            : tooltip.content
-          }
-        </Tooltip>
-      );
-    }
-    return null;
-  }
-
   render() {
     const {
       width,
@@ -164,11 +77,9 @@ class Chart extends React.PureComponent {
       chartStackTrace,
       chartStatus,
       errorMessage,
-      onDismissRefreshOverlay,
       onQuery,
       queryResponse,
       refreshOverlayVisible,
-      vizType,
     } = this.props;
 
     const isLoading = chartStatus === 'loading';
@@ -176,18 +87,16 @@ class Chart extends React.PureComponent {
     // this allows <Loading /> to be positioned in the middle of the chart
     const containerStyles = isLoading ? { height, width } : null;
     const isFaded = refreshOverlayVisible && !errorMessage;
-    const skipChartRendering = isLoading || !!chartAlert;
-    this.renderStartTime = Logger.getTimestamp();
+    this.renderContainerStartTime = Logger.getTimestamp();
 
     return (
-      <ErrorBoundary onError={this.handleRenderFailure} showMessage={false}>
+      <ErrorBoundary onError={this.handleRenderContainerFailure} showMessage={false}>
         <div
           className={`chart-container ${isLoading ? 'is-loading' : ''}`}
           style={containerStyles}
         >
-          {this.renderTooltip()}
 
-          {['loading', 'success'].indexOf(chartStatus) >= 0 && <Loading size={50}
/>}
+          {isLoading && <Loading size={50} />}
 
           {chartAlert && (
             <StackTraceMessage
@@ -202,16 +111,13 @@ class Chart extends React.PureComponent {
               width={width}
               height={height}
               onQuery={onQuery}
-              onDismiss={onDismissRefreshOverlay}
             />
           )}
-          <SuperChart
-            className={`slice_container ${snakeCase(vizType)} ${isFaded ? ' faded' : ''}`}
-            chartType={vizType}
-            chartProps={skipChartRendering ? null : this.prepareChartProps()}
-            onRenderSuccess={this.handleRenderSuccess}
-            onRenderFailure={this.handleRenderFailure}
-          />
+          <div className={`slice_container ${isFaded ? ' faded' : ''}`}>
+            <ChartRenderer
+              {...this.props}
+            />
+          </div>
         </div>
       </ErrorBoundary>
     );
diff --git a/superset/assets/src/chart/Chart.jsx b/superset/assets/src/chart/ChartRenderer.jsx
similarity index 63%
copy from superset/assets/src/chart/Chart.jsx
copy to superset/assets/src/chart/ChartRenderer.jsx
index f5aa347..5730ff9 100644
--- a/superset/assets/src/chart/Chart.jsx
+++ b/superset/assets/src/chart/ChartRenderer.jsx
@@ -2,15 +2,10 @@ import dompurify from 'dompurify';
 import { snakeCase } from 'lodash';
 import PropTypes from 'prop-types';
 import React from 'react';
-import { Tooltip } from 'react-bootstrap';
 import { ChartProps } from '@superset-ui/chart';
+import { Tooltip } from 'react-bootstrap';
 import { Logger, LOG_ACTIONS_RENDER_CHART } from '../logger';
-import Loading from '../components/Loading';
-import RefreshChartOverlay from '../components/RefreshChartOverlay';
-import StackTraceMessage from '../components/StackTraceMessage';
 import SuperChart from '../visualizations/core/components/SuperChart';
-import ErrorBoundary from '../components/ErrorBoundary';
-import './chart.css';
 
 const propTypes = {
   annotationData: PropTypes.object,
@@ -22,20 +17,16 @@ const propTypes = {
   height: PropTypes.number,
   width: PropTypes.number,
   setControlValue: PropTypes.func,
-  timeout: PropTypes.number,
   vizType: PropTypes.string.isRequired,
+  triggerRender: PropTypes.bool,
   // state
   chartAlert: PropTypes.string,
   chartStatus: PropTypes.string,
-  chartStackTrace: PropTypes.string,
   queryResponse: PropTypes.object,
   triggerQuery: PropTypes.bool,
   refreshOverlayVisible: PropTypes.bool,
-  errorMessage: PropTypes.node,
   // dashboard callbacks
   addFilter: PropTypes.func,
-  onQuery: PropTypes.func,
-  onDismissRefreshOverlay: PropTypes.func,
 };
 
 const BLANK = {};
@@ -44,35 +35,70 @@ const defaultProps = {
   addFilter: () => BLANK,
   filters: BLANK,
   setControlValue() {},
+  triggerRender: false,
 };
 
-class Chart extends React.PureComponent {
+class ChartRenderer extends React.PureComponent {
   constructor(props) {
     super(props);
     this.state = {};
 
     this.createChartProps = ChartProps.createSelector();
+
+    this.setTooltip = this.setTooltip.bind(this);
     this.handleAddFilter = this.handleAddFilter.bind(this);
     this.handleRenderSuccess = this.handleRenderSuccess.bind(this);
     this.handleRenderFailure = this.handleRenderFailure.bind(this);
-    this.setTooltip = this.setTooltip.bind(this);
   }
 
-  componentDidMount() {
-    if (this.props.triggerQuery) {
-      this.props.actions.runQuery(
-        this.props.formData,
-        false,
-        this.props.timeout,
-        this.props.chartId,
-      );
+  shouldComponentUpdate(nextProps) {
+    if (
+      nextProps.queryResponse &&
+      ['success', 'rendered'].indexOf(nextProps.chartStatus) > -1 &&
+      !nextProps.queryResponse.error &&
+      !nextProps.refreshOverlayVisible &&
+      (nextProps.annotationData !== this.props.annotationData ||
+        nextProps.queryResponse !== this.props.queryResponse ||
+        nextProps.height !== this.props.height ||
+        nextProps.width !== this.props.width ||
+        nextProps.triggerRender)
+    ) {
+      return true;
     }
+    return false;
   }
 
   setTooltip(tooltip) {
     this.setState({ tooltip });
   }
 
+  prepareChartProps() {
+    const {
+      width,
+      height,
+      annotationData,
+      datasource,
+      filters,
+      formData,
+      queryResponse,
+      setControlValue,
+    } = this.props;
+
+    return this.createChartProps({
+      width,
+      height,
+      annotationData,
+      datasource,
+      filters,
+      formData,
+      onAddFilter: this.handleAddFilter,
+      onError: this.handleRenderFailure,
+      payload: queryResponse,
+      setControlValue,
+      setTooltip: this.setTooltip,
+    });
+  }
+
   handleAddFilter(col, vals, merge = true, refresh = true) {
     this.props.addFilter(col, vals, merge, refresh);
   }
@@ -105,33 +131,6 @@ class Chart extends React.PureComponent {
     });
   }
 
-  prepareChartProps() {
-    const {
-      width,
-      height,
-      annotationData,
-      datasource,
-      filters,
-      formData,
-      queryResponse,
-      setControlValue,
-    } = this.props;
-
-    return this.createChartProps({
-      width,
-      height,
-      annotationData,
-      datasource,
-      filters,
-      formData,
-      onAddFilter: this.handleAddFilter,
-      onError: this.handleRenderFailure,
-      payload: queryResponse,
-      setControlValue,
-      setTooltip: this.setTooltip,
-    });
-  }
-
   renderTooltip() {
     const { tooltip } = this.state;
     if (tooltip && tooltip.content) {
@@ -158,67 +157,32 @@ class Chart extends React.PureComponent {
 
   render() {
     const {
-      width,
-      height,
       chartAlert,
-      chartStackTrace,
       chartStatus,
-      errorMessage,
-      onDismissRefreshOverlay,
-      onQuery,
-      queryResponse,
-      refreshOverlayVisible,
       vizType,
     } = this.props;
 
     const isLoading = chartStatus === 'loading';
 
-    // this allows <Loading /> to be positioned in the middle of the chart
-    const containerStyles = isLoading ? { height, width } : null;
-    const isFaded = refreshOverlayVisible && !errorMessage;
     const skipChartRendering = isLoading || !!chartAlert;
     this.renderStartTime = Logger.getTimestamp();
 
     return (
-      <ErrorBoundary onError={this.handleRenderFailure} showMessage={false}>
-        <div
-          className={`chart-container ${isLoading ? 'is-loading' : ''}`}
-          style={containerStyles}
-        >
-          {this.renderTooltip()}
-
-          {['loading', 'success'].indexOf(chartStatus) >= 0 && <Loading size={50}
/>}
-
-          {chartAlert && (
-            <StackTraceMessage
-              message={chartAlert}
-              link={queryResponse ? queryResponse.link : null}
-              stackTrace={chartStackTrace}
-            />
-          )}
-
-          {!isLoading && !chartAlert && isFaded && (
-            <RefreshChartOverlay
-              width={width}
-              height={height}
-              onQuery={onQuery}
-              onDismiss={onDismissRefreshOverlay}
-            />
-          )}
-          <SuperChart
-            className={`slice_container ${snakeCase(vizType)} ${isFaded ? ' faded' : ''}`}
-            chartType={vizType}
-            chartProps={skipChartRendering ? null : this.prepareChartProps()}
-            onRenderSuccess={this.handleRenderSuccess}
-            onRenderFailure={this.handleRenderFailure}
-          />
-        </div>
-      </ErrorBoundary>
+      <React.Fragment>
+        {this.renderTooltip()}
+        <SuperChart
+          className={`${snakeCase(vizType)}`}
+          chartType={vizType}
+          chartProps={skipChartRendering ? null : this.prepareChartProps()}
+          onRenderSuccess={this.handleRenderSuccess}
+          onRenderFailure={this.handleRenderFailure}
+        />
+      </React.Fragment>
     );
   }
 }
 
-Chart.propTypes = propTypes;
-Chart.defaultProps = defaultProps;
+ChartRenderer.propTypes = propTypes;
+ChartRenderer.defaultProps = defaultProps;
 
-export default Chart;
+export default ChartRenderer;
diff --git a/superset/assets/src/chart/chartReducer.js b/superset/assets/src/chart/chartReducer.js
index 3936f9c..4dec8dd 100644
--- a/superset/assets/src/chart/chartReducer.js
+++ b/superset/assets/src/chart/chartReducer.js
@@ -85,7 +85,11 @@ export default function chartReducer(charts = {}, action) {
       };
     },
     [actions.TRIGGER_QUERY](state) {
-      return { ...state, triggerQuery: action.value };
+      return {
+        ...state,
+        triggerQuery: action.value,
+        chartStatus: 'loading',
+      };
     },
     [actions.RENDER_TRIGGERED](state) {
       return { ...state, lastRendered: action.value };
diff --git a/superset/assets/src/components/RefreshChartOverlay.jsx b/superset/assets/src/components/RefreshChartOverlay.jsx
index 841559a..2e0a53d 100644
--- a/superset/assets/src/components/RefreshChartOverlay.jsx
+++ b/superset/assets/src/components/RefreshChartOverlay.jsx
@@ -7,7 +7,6 @@ const propTypes = {
   height: PropTypes.number.isRequired,
   width: PropTypes.number.isRequired,
   onQuery: PropTypes.func,
-  onDismiss: PropTypes.func,
 };
 
 class RefreshChartOverlay extends React.PureComponent {
@@ -25,12 +24,6 @@ class RefreshChartOverlay extends React.PureComponent {
           >
             {t('Run Query')}
           </Button>
-          <Button
-            className="dismiss-overlay-btn"
-            onClick={this.props.onDismiss}
-          >
-            {t('Dismiss')}
-          </Button>
         </div>
       </div>
     );
diff --git a/superset/assets/src/explore/components/ExploreChartPanel.jsx b/superset/assets/src/explore/components/ExploreChartPanel.jsx
index 1cdc94c..d9769d5 100644
--- a/superset/assets/src/explore/components/ExploreChartPanel.jsx
+++ b/superset/assets/src/explore/components/ExploreChartPanel.jsx
@@ -10,7 +10,6 @@ const propTypes = {
   actions: PropTypes.object.isRequired,
   addHistory: PropTypes.func,
   onQuery: PropTypes.func,
-  onDismissRefreshOverlay: PropTypes.func,
   can_overwrite: PropTypes.bool.isRequired,
   can_download: PropTypes.bool.isRequired,
   datasource: PropTypes.object,
@@ -28,6 +27,7 @@ const propTypes = {
   refreshOverlayVisible: PropTypes.bool,
   chart: chartPropShape,
   errorMessage: PropTypes.node,
+  triggerRender: PropTypes.bool,
 };
 
 class ExploreChartPanel extends React.PureComponent {
@@ -46,10 +46,10 @@ class ExploreChartPanel extends React.PureComponent {
             chartStackTrace={chart.chartStackTrace}
             chartId={chart.id}
             chartStatus={chart.chartStatus}
+            triggerRender={this.props.triggerRender}
             datasource={this.props.datasource}
             errorMessage={this.props.errorMessage}
             formData={this.props.form_data}
-            onDismissRefreshOverlay={this.props.onDismissRefreshOverlay}
             onQuery={this.props.onQuery}
             queryResponse={chart.queryResponse}
             refreshOverlayVisible={this.props.refreshOverlayVisible}
diff --git a/superset/assets/src/explore/components/ExploreViewContainer.jsx b/superset/assets/src/explore/components/ExploreViewContainer.jsx
index bded187..162cb16 100644
--- a/superset/assets/src/explore/components/ExploreViewContainer.jsx
+++ b/superset/assets/src/explore/components/ExploreViewContainer.jsx
@@ -122,10 +122,6 @@ class ExploreViewContainer extends React.Component {
     this.addHistory({});
   }
 
-  onDismissRefreshOverlay() {
-    this.setState({ refreshOverlayVisible: false });
-  }
-
   onStop() {
     if (this.props.chart && this.props.chart.queryController) {
       this.props.chart.queryController.abort();
@@ -247,7 +243,6 @@ class ExploreViewContainer extends React.Component {
         refreshOverlayVisible={this.state.refreshOverlayVisible}
         addHistory={this.addHistory}
         onQuery={this.onQuery.bind(this)}
-        onDismissRefreshOverlay={this.onDismissRefreshOverlay.bind(this)}
       />
     );
   }
@@ -319,6 +314,7 @@ function mapStateToProps(state) {
     containerId: explore.slice ? `slice-container-${explore.slice.slice_id}` : 'slice-container',
     isStarred: explore.isStarred,
     slice: explore.slice,
+    triggerRender: explore.triggerRender,
     form_data,
     table_name: form_data.datasource_name,
     vizType: form_data.viz_type,
diff --git a/superset/assets/src/explore/reducers/exploreReducer.js b/superset/assets/src/explore/reducers/exploreReducer.js
index 7d4c5d5..77f1378 100644
--- a/superset/assets/src/explore/reducers/exploreReducer.js
+++ b/superset/assets/src/explore/reducers/exploreReducer.js
@@ -80,11 +80,14 @@ export default function exploreReducer(state = {}, action) {
       };
       if (control.renderTrigger) {
         changes.triggerRender = true;
+      } else {
+        changes.triggerRender = false;
       }
-      return {
+      const newState = {
         ...state,
         ...changes,
       };
+      return newState;
     },
     [actions.SET_EXPLORE_CONTROLS]() {
       return {
diff --git a/superset/assets/src/logger.js b/superset/assets/src/logger.js
index 3f45477..a7a21bd 100644
--- a/superset/assets/src/logger.js
+++ b/superset/assets/src/logger.js
@@ -132,6 +132,7 @@ export const LOG_ACTIONS_MOUNT_EXPLORER = 'mount_explorer';
 export const LOG_ACTIONS_FIRST_DASHBOARD_LOAD = 'first_dashboard_load';
 export const LOG_ACTIONS_LOAD_DASHBOARD_PANE = 'load_dashboard_pane';
 export const LOG_ACTIONS_LOAD_CHART = 'load_chart_data';
+export const LOG_ACTIONS_RENDER_CHART_CONTAINER = 'render_chart_container';
 export const LOG_ACTIONS_RENDER_CHART = 'render_chart';
 export const LOG_ACTIONS_REFRESH_CHART = 'force_refresh_chart';
 
@@ -158,4 +159,5 @@ export const EXPLORE_EVENT_NAMES = [
   LOG_ACTIONS_LOAD_CHART,
   LOG_ACTIONS_RENDER_CHART,
   LOG_ACTIONS_REFRESH_CHART,
+  LOG_ACTIONS_RENDER_CHART_CONTAINER,
 ];


Mime
View raw message