kristw closed pull request #6483: [Bug Fix]Prevent re-rendering when
non-instant controls change
URL: https://github.com/apache/incubator-superset/pull/6483
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
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 (
- <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/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 (
+ <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 {
+ chartAlert,
+ chartStatus,
+ vizType,
+ } = this.props;
+
+ const isLoading = chartStatus === 'loading';
+
+ const skipChartRendering = isLoading || !!chartAlert;
+ this.renderStartTime = Logger.getTimestamp();
+
+ return (
+ <React.Fragment>
+ {this.renderTooltip()}
+ <SuperChart
+ className={`${snakeCase(vizType)}`}
+ chartType={vizType}
+ chartProps={skipChartRendering ? null : this.prepareChartProps()}
+ onRenderSuccess={this.handleRenderSuccess}
+ onRenderFailure={this.handleRenderFailure}
+ />
+ </React.Fragment>
+ );
+ }
+}
+
+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')}
</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 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,
];
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
