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 <[email protected]>
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,
];
