[READY] Update SuperChart onRenderXXX listeners (#6376)

* Revise LoadableRenderer (+3 squashed commits)
Squashed commits:
[f1614c7c] extract createLoadableRenderer into a separate function
[8689bc94] extend LoadableRenderer
[3d04ff2b] remove skipRendering

* add number of times function was called to the test

superset/assets/spec/javascripts/visualizations/core/createLoadableRenderer_spec.jsx

@@ -0,0 +1,94 @@
+import React from 'react';
+import { shallow } from 'enzyme';
+import createLoadableRenderer from 'src/visualizations/core/components/createLoadableRenderer';
+
+describe('createLoadableRenderer', () => {
+  function TestComponent() {
+    return (
+      <div className="test-component">test</div>
+    );
+  }
+  let loadChartSuccess;
+  let render;
+  let loading;
+  let LoadableRenderer;
+
+  beforeEach(() => {
+    loadChartSuccess = jest.fn(() => Promise.resolve(TestComponent));
+    render = jest.fn((loaded) => {
+      const { Chart } = loaded;
+      return (<Chart />);
+    });
+    loading = jest.fn(() => (<div>Loading</div>));
+    LoadableRenderer = createLoadableRenderer({
+      loader: {
+        Chart: loadChartSuccess,
+      },
+      loading,
+      render,
+    });
+  });
+
+  describe('returns a LoadableRenderer class', () => {
+    it('LoadableRenderer.preload() preloads the lazy-load components', () => {
+      expect(LoadableRenderer.preload).toBeInstanceOf(Function);
+      LoadableRenderer.preload();
+      expect(loadChartSuccess).toHaveBeenCalledTimes(1);
+    });
+
+    it('calls onRenderSuccess when succeeds', (done) => {
+      const onRenderSuccess = jest.fn();
+      const onRenderFailure = jest.fn();
+      shallow(
+        <LoadableRenderer
+          onRenderSuccess={onRenderSuccess}
+          onRenderFailure={onRenderFailure}
+        />,
+      );
+      expect(loadChartSuccess).toHaveBeenCalled();
+      setTimeout(() => {
+        expect(render).toHaveBeenCalledTimes(1);
+        expect(onRenderSuccess).toHaveBeenCalledTimes(1);
+        expect(onRenderFailure).not.toHaveBeenCalled();
+        done();
+      }, 10);
+    });
+
+    it('calls onRenderFailure when fails', (done) => {
+      const loadChartFailure = jest.fn(() => Promise.reject('Invalid chart'));
+      const FailedRenderer = createLoadableRenderer({
+        loader: {
+          Chart: loadChartFailure,
+        },
+        loading,
+        render,
+      });
+      const onRenderSuccess = jest.fn();
+      const onRenderFailure = jest.fn();
+      shallow(
+        <FailedRenderer
+          onRenderSuccess={onRenderSuccess}
+          onRenderFailure={onRenderFailure}
+        />,
+      );
+      expect(loadChartFailure).toHaveBeenCalledTimes(1);
+      setTimeout(() => {
+        expect(render).not.toHaveBeenCalled();
+        expect(onRenderSuccess).not.toHaveBeenCalled();
+        expect(onRenderFailure).toHaveBeenCalledTimes(1);
+        done();
+      }, 10);
+    });
+
+    it('renders the lazy-load components', (done) => {
+      const wrapper = shallow(<LoadableRenderer />);
+      // lazy-loaded component not rendered immediately
+      expect(wrapper.find(TestComponent)).toHaveLength(0);
+      setTimeout(() => {
+        // but rendered after the component is loaded.
+        expect(wrapper.find(TestComponent)).toHaveLength(1);
+        done();
+      }, 10);
+    });
+  });
+});

superset/assets/src/chart/Chart.jsx

@@ -211,7 +211,6 @@ class Chart extends React.PureComponent {
             chartProps={skipChartRendering ? null : this.prepareChartProps()}
             onRenderSuccess={this.handleRenderSuccess}
             onRenderFailure={this.handleRenderFailure}
-            skipRendering={skipChartRendering}
           />
         </div>
       </ErrorBoundary>

superset/assets/src/visualizations/core/components/SuperChart.jsx

@@ -1,8 +1,8 @@
 import React from 'react';
-import Loadable from 'react-loadable';
 import PropTypes from 'prop-types';
 import { createSelector } from 'reselect';
 import { getChartComponentRegistry, getChartTransformPropsRegistry, ChartProps } from '@superset-ui/chart';
+import createLoadableRenderer from './createLoadableRenderer';
 
 const IDENTITY = x => x;
 
@@ -16,7 +16,6 @@ const propTypes = {
   postTransformProps: PropTypes.func,
   onRenderSuccess: PropTypes.func,
   onRenderFailure: PropTypes.func,
-  skipRendering: PropTypes.bool,
 };
 const defaultProps = {
   id: '',
@@ -26,7 +25,6 @@ const defaultProps = {
   postTransformProps: IDENTITY,
   onRenderSuccess() {},
   onRenderFailure() {},
-  skipRendering: false,
 };
 
 class SuperChart extends React.PureComponent {
@@ -66,7 +64,7 @@ class SuperChart extends React.PureComponent {
       input => input.overrideTransformProps,
       (chartType, overrideTransformProps) => {
         if (chartType) {
-          return Loadable.Map({
+          const LoadableRenderer = createLoadableRenderer({
             loader: {
               Chart: () => componentRegistry.getAsPromise(chartType),
               transformProps: overrideTransformProps
@@ -76,12 +74,18 @@ class SuperChart extends React.PureComponent {
             loading: loadingProps => this.renderLoading(loadingProps, chartType),
             render: this.renderChart,
           });
+
+          // Trigger preloading.
+          LoadableRenderer.preload();
+
+          return LoadableRenderer;
         }
         return null;
       },
     );
   }
 
+
   renderChart(loaded, props) {
     const Chart = loaded.Chart.default || loaded.Chart;
     const transformProps = loaded.transformProps.default || loaded.transformProps;
@@ -91,7 +95,7 @@ class SuperChart extends React.PureComponent {
       postTransformProps,
     } = props;
 
-    const result = (
+    return (
       <Chart
         {...this.processChartProps({
           preTransformProps,
@@ -101,23 +105,19 @@ class SuperChart extends React.PureComponent {
         })}
       />
     );
-    setTimeout(() => this.props.onRenderSuccess(), 0);
-    return result;
   }
 
-  renderLoading(loadableProps, chartType) {
-    const { error } = loadableProps;
+  renderLoading(loadingProps, chartType) {
+    const { error } = loadingProps;
 
     if (error) {
-      const result = (
+      return (
         <div className="alert alert-warning" role="alert">
           <strong>ERROR</strong>&nbsp;
           <code>chartType="{chartType}"</code> &mdash;
           {JSON.stringify(error)}
         </div>
       );
-      setTimeout(() => this.props.onRenderFailure(error), 0);
-      return result;
     }
 
     return null;
@@ -130,14 +130,18 @@ class SuperChart extends React.PureComponent {
       preTransformProps,
       postTransformProps,
       chartProps,
-      skipRendering,
+      onRenderSuccess,
+      onRenderFailure,
     } = this.props;
 
+    // Create LoadableRenderer and start preloading
+    // the lazy-loaded Chart components
     const LoadableRenderer = this.createLoadableRenderer(this.props);
 
-    // Use this to allow loading the vis components
-    // without rendering (while waiting for data)
-    if (skipRendering || !chartProps) {
+    // Do not render if chartProps is not available.
+    // but the pre-loading has been started in this.createLoadableRenderer
+    // to prepare for rendering once chartProps becomes available.
+    if (!chartProps) {
       return null;
     }
 
@@ -148,6 +152,8 @@ class SuperChart extends React.PureComponent {
             preTransformProps={preTransformProps}
             postTransformProps={postTransformProps}
             chartProps={chartProps}
+            onRenderSuccess={onRenderSuccess}
+            onRenderFailure={onRenderFailure}
           />
         )}
       </div>

superset/assets/src/visualizations/core/components/createLoadableRenderer.js

@@ -0,0 +1,46 @@
+import Loadable from 'react-loadable';
+import PropTypes from 'prop-types';
+
+const propTypes = {
+  onRenderSuccess: PropTypes.func,
+  onRenderFailure: PropTypes.func,
+};
+
+const defaultProps = {
+  onRenderSuccess() {},
+  onRenderFailure() {},
+};
+
+export default function createLoadableRenderer(options) {
+  const LoadableRenderer = Loadable.Map(options);
+
+  // Extends the behavior of LoadableComponent
+  // generated by react-loadable
+  // to provide post-render listeners
+  class CustomLoadableRenderer extends LoadableRenderer {
+    componentDidMount() {
+      this.afterRender();
+    }
+
+    componentDidUpdate() {
+      this.afterRender();
+    }
+
+    afterRender() {
+      const { loaded, loading, error } = this.state;
+      if (!loading) {
+        if (error) {
+          this.props.onRenderFailure(error);
+        } else if (loaded && Object.keys(loaded).length > 0) {
+          this.props.onRenderSuccess();
+        }
+      }
+    }
+  }
+
+  CustomLoadableRenderer.defaultProps = defaultProps;
+  CustomLoadableRenderer.propTypes = propTypes;
+  CustomLoadableRenderer.preload = LoadableRenderer.preload;
+
+  return CustomLoadableRenderer;
+}