From fc8acf27c82ef3030d641289fa80f926f2a83b76 Mon Sep 17 00:00:00 2001 From: Conglei Date: Wed, 5 Dec 2018 17:31:59 -0800 Subject: [PATCH] [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 ++--------- superset/assets/src/chart/ChartRenderer.jsx | 188 ++++++++++++++++++ superset/assets/src/chart/chartReducer.js | 6 +- .../src/components/RefreshChartOverlay.jsx | 7 - .../explore/components/ExploreChartPanel.jsx | 4 +- .../components/ExploreViewContainer.jsx | 6 +- .../src/explore/reducers/exploreReducer.js | 5 +- superset/assets/src/logger.js | 2 + 8 files changed, 215 insertions(+), 123 deletions(-) create mode 100644 superset/assets/src/chart/ChartRenderer.jsx diff --git a/superset/assets/src/chart/Chart.jsx b/superset/assets/src/chart/Chart.jsx index f5aa34767c..e6fc230a37 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 ( - - {typeof (tooltip.content) === 'string' ? -
- : tooltip.content - } - - ); - } - 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 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 ( - +
- {this.renderTooltip()} - {['loading', 'success'].indexOf(chartStatus) >= 0 && } + {isLoading && } {chartAlert && ( )} - +
+ +
); diff --git a/superset/assets/src/chart/ChartRenderer.jsx b/superset/assets/src/chart/ChartRenderer.jsx new file mode 100644 index 0000000000..5730ff9b0c --- /dev/null +++ b/superset/assets/src/chart/ChartRenderer.jsx @@ -0,0 +1,188 @@ +import dompurify from 'dompurify'; +import { snakeCase } from 'lodash'; +import PropTypes from 'prop-types'; +import React from 'react'; +import { ChartProps } from '@superset-ui/chart'; +import { Tooltip } from 'react-bootstrap'; +import { Logger, LOG_ACTIONS_RENDER_CHART } from '../logger'; +import SuperChart from '../visualizations/core/components/SuperChart'; + +const propTypes = { + annotationData: PropTypes.object, + actions: PropTypes.object, + chartId: PropTypes.number.isRequired, + datasource: PropTypes.object.isRequired, + filters: PropTypes.object, + formData: PropTypes.object.isRequired, + height: PropTypes.number, + width: PropTypes.number, + setControlValue: PropTypes.func, + vizType: PropTypes.string.isRequired, + triggerRender: PropTypes.bool, + // state + chartAlert: PropTypes.string, + chartStatus: PropTypes.string, + queryResponse: PropTypes.object, + triggerQuery: PropTypes.bool, + refreshOverlayVisible: PropTypes.bool, + // dashboard callbacks + addFilter: PropTypes.func, +}; + +const BLANK = {}; + +const defaultProps = { + addFilter: () => BLANK, + filters: BLANK, + setControlValue() {}, + triggerRender: false, +}; + +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); + } + + 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); + } + + 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, { + slice_id: chartId, + has_err: true, + error_details: error.toString(), + start_offset: this.renderStartTime, + duration: Logger.getTimestamp() - this.renderStartTime, + }); + } + + renderTooltip() { + const { tooltip } = this.state; + if (tooltip && tooltip.content) { + return ( + + {typeof (tooltip.content) === 'string' ? +
+ : tooltip.content + } + + ); + } + return null; + } + + render() { + const { + chartAlert, + chartStatus, + vizType, + } = this.props; + + const isLoading = chartStatus === 'loading'; + + const skipChartRendering = isLoading || !!chartAlert; + this.renderStartTime = Logger.getTimestamp(); + + return ( + + {this.renderTooltip()} + + + ); + } +} + +ChartRenderer.propTypes = propTypes; +ChartRenderer.defaultProps = defaultProps; + +export default ChartRenderer; diff --git a/superset/assets/src/chart/chartReducer.js b/superset/assets/src/chart/chartReducer.js index 3936f9c83b..4dec8dd1de 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 841559a8b2..2e0a53d936 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')} -
); diff --git a/superset/assets/src/explore/components/ExploreChartPanel.jsx b/superset/assets/src/explore/components/ExploreChartPanel.jsx index 1cdc94cc9d..d9769d55e6 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 bded1877b6..162cb16333 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 7d4c5d5e48..77f1378d4e 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 3f45477636..a7a21bd35c 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, ];