This is an automated email from the ASF dual-hosted git repository. michellet pushed a commit to branch 0.30 in repository https://gitbox.apache.org/repos/asf/incubator-superset.git
commit 77800cbbc0ea16d9ecdd39d5d835a733a9a3d2b5 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 (cherry picked from commit fc8acf27c82ef3030d641289fa80f926f2a83b76) --- 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, ];
