superset-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From b...@apache.org
Subject [incubator-superset] branch master updated: Wrap <LoadableRenderer /> with <ErrorBoundary /> (#6294)
Date Thu, 08 Nov 2018 01:11:57 GMT
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 <maximebeauchemin@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,


Mime
View raw message