This is an automated email from the ASF dual-hosted git repository. beto 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 4ce475f Wrap <LoadableRenderer /> with <ErrorBoundary /> (#6294) 4ce475f is described below commit 4ce475f2872fc4fd3a459cd1ec5d32bfad05dac6 Author: Maxime Beauchemin <maximebeauche...@gmail.com> AuthorDate: Wed Nov 7 17:11:52 2018 -0800 Wrap <LoadableRenderer /> with <ErrorBoundary /> (#6294) * Wrap <LoadableRenderer /> with <ErrorBoundary /> It appears that since the introduction of <SuperChart />, errors in the visualization javascript (which are somewhat common and expected, especially as we'll support plugins) were not handled and the whole page would throw and go missing. Here I'm introducing a new <ErrorBoundary /> component that elegantly wraps other components and handles errors. It's inspired by: https://reactjs.org/docs/error-boundaries.html The default behavior of the component is to simply surface the error as an <Alert bsStyle="danger" /> and exposes the React stacktrace when clicking on the error. It's also possible to use component and pass an onError handler and not show the default message. This also fixes some minor bugs in TimeTable. * Addressing comments --- superset/assets/src/chart/Chart.jsx | 24 +++++++----- superset/assets/src/components/CachedLabel.jsx | 2 +- superset/assets/src/components/ErrorBoundary.jsx | 45 ++++++++++++++++++++++ .../assets/src/components/StackTraceMessage.jsx | 25 +++++------- .../src/visualizations/TimeTable/TimeTable.jsx | 2 +- .../src/visualizations/TimeTable/transformProps.js | 2 +- 6 files changed, 72 insertions(+), 28 deletions(-) diff --git a/superset/assets/src/chart/Chart.jsx b/superset/assets/src/chart/Chart.jsx index a4be8dc..d0d3461 100644 --- a/superset/assets/src/chart/Chart.jsx +++ b/superset/assets/src/chart/Chart.jsx @@ -9,6 +9,7 @@ import RefreshChartOverlay from '../components/RefreshChartOverlay'; import StackTraceMessage from '../components/StackTraceMessage'; import ChartProps from '../visualizations/core/models/ChartProps'; import SuperChart from '../visualizations/core/components/SuperChart'; +import ErrorBoundary from '../components/ErrorBoundary'; import './chart.css'; const propTypes = { @@ -92,7 +93,7 @@ class Chart extends React.PureComponent { handleRenderFailure(e) { const { actions, chartId } = this.props; console.warn(e); // eslint-disable-line - actions.chartRenderingFailed(e, chartId); + actions.chartRenderingFailed(e.toString(), chartId); } prepareChartProps() { @@ -181,7 +182,8 @@ class Chart extends React.PureComponent { {chartAlert && ( <StackTraceMessage message={chartAlert} - queryResponse={queryResponse} + link={queryResponse ? queryResponse.link : null} + stackTrace={queryResponse ? queryResponse.stacktrace : null} /> )} @@ -194,14 +196,16 @@ class Chart extends React.PureComponent { /> )} - <SuperChart - className={`slice_container ${snakeCase(vizType)} ${isFaded ? ' faded' : ''}`} - chartType={vizType} - chartProps={skipChartRendering ? null : this.prepareChartProps()} - onRenderSuccess={this.handleRenderSuccess} - onRenderFailure={this.handleRenderFailure} - skipRendering={skipChartRendering} - /> + <ErrorBoundary onError={this.handleRenderFailure} showMessage={false}> + <SuperChart + className={`slice_container ${snakeCase(vizType)} ${isFaded ? ' faded' : ''}`} + chartType={vizType} + chartProps={skipChartRendering ? null : this.prepareChartProps()} + onRenderSuccess={this.handleRenderSuccess} + onRenderFailure={this.handleRenderFailure} + skipRendering={skipChartRendering} + /> + </ErrorBoundary> </div> ); } diff --git a/superset/assets/src/components/CachedLabel.jsx b/superset/assets/src/components/CachedLabel.jsx index 845eac5..f529cde 100644 --- a/superset/assets/src/components/CachedLabel.jsx +++ b/superset/assets/src/components/CachedLabel.jsx @@ -60,7 +60,7 @@ class CacheLabel extends React.PureComponent { onMouseOver={this.mouseOver.bind(this)} onMouseOut={this.mouseOut.bind(this)} > - cached <i className="fa fa-refresh" /> + {t('cached')} <i className="fa fa-refresh" /> </Label> </TooltipWrapper>); } diff --git a/superset/assets/src/components/ErrorBoundary.jsx b/superset/assets/src/components/ErrorBoundary.jsx new file mode 100644 index 0000000..1be2d2e --- /dev/null +++ b/superset/assets/src/components/ErrorBoundary.jsx @@ -0,0 +1,45 @@ +import React from 'react'; +import PropTypes from 'prop-types'; +import { t } from '@superset-ui/translation'; +import StackTraceMessage from './StackTraceMessage'; + +const propTypes = { + children: PropTypes.node.isRequired, + onError: PropTypes.func, + showMessage: PropTypes.bool, +}; +const defaultProps = { + onError: () => {}, + showMessage: true, +}; + +export default class ErrorBoundary extends React.Component { + constructor(props) { + super(props); + this.state = { error: null, info: null }; + } + + componentDidCatch(error, info) { + this.props.onError(error, info); + this.setState({ error, info }); + } + + render() { + const { error, info } = this.state; + if (error) { + const firstLine = error ? error.toString() : null; + const message = ( + <span> + <strong>{t('Unexpected error')}</strong>{firstLine ? `: ${firstLine}` : ''} + </span>); + if (this.props.showMessage) { + return ( + <StackTraceMessage message={message} stackTrace={info ? info.componentStack : null} />); + } + return null; + } + return this.props.children; + } +} +ErrorBoundary.propTypes = propTypes; +ErrorBoundary.defaultProps = defaultProps; diff --git a/superset/assets/src/components/StackTraceMessage.jsx b/superset/assets/src/components/StackTraceMessage.jsx index 9253108..d3a0f68 100644 --- a/superset/assets/src/components/StackTraceMessage.jsx +++ b/superset/assets/src/components/StackTraceMessage.jsx @@ -4,12 +4,15 @@ import PropTypes from 'prop-types'; import { Alert, Collapse } from 'react-bootstrap'; const propTypes = { - message: PropTypes.string, - queryResponse: PropTypes.object, + message: PropTypes.node.isRequired, + link: PropTypes.string, + stackTrace: PropTypes.string, showStackTrace: PropTypes.bool, }; const defaultProps = { showStackTrace: false, + link: null, + stackTrace: null, }; class StackTraceMessage extends React.PureComponent { @@ -21,25 +24,17 @@ class StackTraceMessage extends React.PureComponent { }; } - hasTrace() { - return this.props.queryResponse && this.props.queryResponse.stacktrace; - } - - hasLink() { - return this.props.queryResponse && this.props.queryResponse.link; - } - render() { return ( - <div className={`stack-trace-container${this.hasTrace() ? ' has-trace' : ''}`}> + <div className={`stack-trace-container${this.props.stackTrace ? ' has-trace' : ''}`}> <Alert bsStyle="warning" onClick={() => this.setState({ showStackTrace: !this.state.showStackTrace })} > {this.props.message} - {this.hasLink() && + {this.props.link && <a - href={this.props.queryResponse.link} + href={this.props.link} target="_blank" rel="noopener noreferrer" > @@ -47,10 +42,10 @@ class StackTraceMessage extends React.PureComponent { </a> } </Alert> - {this.hasTrace() && + {this.props.stackTrace && <Collapse in={this.state.showStackTrace}> <pre> - {this.props.queryResponse.stacktrace} + {this.props.stackTrace} </pre> </Collapse> } diff --git a/superset/assets/src/visualizations/TimeTable/TimeTable.jsx b/superset/assets/src/visualizations/TimeTable/TimeTable.jsx index c414a7c..0eaafc7 100644 --- a/superset/assets/src/visualizations/TimeTable/TimeTable.jsx +++ b/superset/assets/src/visualizations/TimeTable/TimeTable.jsx @@ -226,7 +226,7 @@ class TimeTable extends React.PureComponent { .map(time => ({ ...data[time], time })); const reversedEntries = entries.concat().reverse(); - const defaultSort = rowType === 'column' ? { + const defaultSort = rowType === 'column' && columnConfigs.length ? { column: columnConfigs[0].key, direction: 'desc', } : false; diff --git a/superset/assets/src/visualizations/TimeTable/transformProps.js b/superset/assets/src/visualizations/TimeTable/transformProps.js index d880fae..d52088b 100644 --- a/superset/assets/src/visualizations/TimeTable/transformProps.js +++ b/superset/assets/src/visualizations/TimeTable/transformProps.js @@ -1,7 +1,7 @@ export default function transformProps(chartProps) { const { height, datasource, formData, payload } = chartProps; const { - columnCollection, + columnCollection = 0, groupby, metrics, url,