GrafanaRoute: Use React.Lazy instead of react-loadable and improve error handling (#55339)

* Things are working

* Add new GrafanaRoute tests

* removed old file

* Remove from package.json

.betterer.results

@@ -2782,8 +2782,7 @@ exports[`better eslint`] = {
       [0, 0, 0, "Unexpected any. Specify a different type.", "1"],
       [0, 0, 0, "Unexpected any. Specify a different type.", "2"],
       [0, 0, 0, "Unexpected any. Specify a different type.", "3"],
-      [0, 0, 0, "Unexpected any. Specify a different type.", "4"],
-      [0, 0, 0, "Unexpected any. Specify a different type.", "5"]
+      [0, 0, 0, "Unexpected any. Specify a different type.", "4"]
     ],
     "public/app/core/navigation/RouterDebugger.tsx:5381": [
       [0, 0, 0, "Unexpected any. Specify a different type.", "0"]

package.json

@@ -146,7 +146,6 @@
     "@types/react-dom": "17.0.14",
     "@types/react-grid-layout": "1.3.2",
     "@types/react-highlight-words": "0.16.4",
-    "@types/react-loadable": "5.5.6",
     "@types/react-redux": "7.1.24",
     "@types/react-router-dom": "5.3.3",
     "@types/react-table": "7.7.12",
@@ -364,7 +363,6 @@
     "react-highlight-words": "0.18.0",
     "react-hook-form": "7.5.3",
     "react-inlinesvg": "3.0.0",
-    "react-loadable": "5.5.0",
     "react-moveable": "0.38.4",
     "react-popper": "2.3.0",
     "react-popper-tooltip": "4.4.2",

public/app/core/components/DynamicImports/ErrorLoadingChunk.tsx

@@ -1,46 +0,0 @@
-import { css } from '@emotion/css';
-import React, { FunctionComponent } from 'react';
-
-import { Button, stylesFactory } from '@grafana/ui';
-import { useUrlParams } from 'app/core/navigation/hooks';
-
-const getStyles = stylesFactory(() => {
-  return css`
-    width: 508px;
-    margin: 128px auto;
-  `;
-});
-
-interface Props {
-  error: Error | null;
-}
-
-export const ErrorLoadingChunk: FunctionComponent<Props> = ({ error }) => {
-  const [params, updateUrlParams] = useUrlParams();
-
-  if (!params.get('chunkNotFound')) {
-    updateUrlParams({ chunkNotFound: true }, true);
-    window.location.reload();
-  }
-
-  return (
-    <div className={getStyles()}>
-      <h2>Unable to find application file</h2>
-      <br />
-      <h2 className="page-heading">Grafana has likely been updated. Please try reloading the page.</h2>
-      <br />
-      <div className="gf-form-group">
-        <Button size="md" variant="secondary" icon="repeat" onClick={() => window.location.reload()}>
-          Reload
-        </Button>
-      </div>
-      <details style={{ whiteSpace: 'pre-wrap' }}>
-        {error && error.message ? error.message : 'Unexpected error occurred'}
-        <br />
-        {error && error.stack ? error.stack : null}
-      </details>
-    </div>
-  );
-};
-
-ErrorLoadingChunk.displayName = 'ErrorLoadingChunk';

public/app/core/components/DynamicImports/LoadingChunkPlaceHolder.tsx

@@ -1,11 +0,0 @@
-import React, { FunctionComponent } from 'react';
-
-import { LoadingPlaceholder } from '@grafana/ui';
-
-export const LoadingChunkPlaceHolder: FunctionComponent = React.memo(() => (
-  <div className="preloader">
-    <LoadingPlaceholder text={'Loading...'} />
-  </div>
-));
-
-LoadingChunkPlaceHolder.displayName = 'LoadingChunkPlaceHolder';

public/app/core/components/DynamicImports/SafeDynamicImport.test.tsx

@@ -1,37 +0,0 @@
-import React from 'react';
-
-import { ErrorLoadingChunk } from './ErrorLoadingChunk';
-import { LoadingChunkPlaceHolder } from './LoadingChunkPlaceHolder';
-import { loadComponentHandler } from './SafeDynamicImport';
-
-describe('loadComponentHandler', () => {
-  describe('when there is no error and pastDelay is false', () => {
-    it('then it should return null', () => {
-      const error: Error | null = null;
-      const pastDelay = false;
-      const element = loadComponentHandler({ error: error as unknown as Error, pastDelay });
-
-      expect(element).toBe(null);
-    });
-  });
-
-  describe('when there is an error', () => {
-    it('then it should return ErrorLoadingChunk', () => {
-      const error: Error = new Error('Some chunk failed to load');
-      const pastDelay = false;
-      const element = loadComponentHandler({ error, pastDelay });
-
-      expect(element).toEqual(<ErrorLoadingChunk error={error} />);
-    });
-  });
-
-  describe('when loading is taking more then default delay of 200ms', () => {
-    it('then it should return LoadingChunkPlaceHolder', () => {
-      const error: Error | null = null;
-      const pastDelay = true;
-      const element = loadComponentHandler({ error: error as unknown as Error, pastDelay });
-
-      expect(element).toEqual(<LoadingChunkPlaceHolder />);
-    });
-  });
-});

public/app/core/components/DynamicImports/SafeDynamicImport.tsx

@@ -1,27 +1,5 @@
 import React from 'react';
-import Loadable from 'react-loadable';
 
 import { GrafanaRouteComponent } from 'app/core/navigation/types';
 
-import { ErrorLoadingChunk } from './ErrorLoadingChunk';
-import { LoadingChunkPlaceHolder } from './LoadingChunkPlaceHolder';
-
-export const loadComponentHandler = (props: { error: Error; pastDelay: boolean }) => {
-  const { error, pastDelay } = props;
-
-  if (error) {
-    return <ErrorLoadingChunk error={error} />;
-  }
-
-  if (pastDelay) {
-    return <LoadingChunkPlaceHolder />;
-  }
-
-  return null;
-};
-
-export const SafeDynamicImport = (loader: () => Promise<any>): GrafanaRouteComponent =>
-  Loadable({
-    loader: loader,
-    loading: loadComponentHandler,
-  });
+export const SafeDynamicImport = (loader: () => Promise<any>): GrafanaRouteComponent => React.lazy(loader);

public/app/core/navigation/GrafanaRoute.test.tsx

@@ -1,5 +1,6 @@
-import { render } from '@testing-library/react';
-import React from 'react';
+import { render, screen } from '@testing-library/react';
+import React, { ComponentType } from 'react';
+import { BrowserRouter } from 'react-router-dom';
 import { getGrafanaContextMock } from 'test/mocks/getGrafanaContextMock';
 
 import { setEchoSrv } from '@grafana/runtime';
@@ -7,7 +8,28 @@ import { setEchoSrv } from '@grafana/runtime';
 import { GrafanaContext } from '../context/GrafanaContext';
 import { Echo } from '../services/echo/Echo';
 
-import { GrafanaRoute } from './GrafanaRoute';
+import { GrafanaRoute, Props } from './GrafanaRoute';
+
+function setup(overrides: Partial<Props>) {
+  const props: Props = {
+    location: { search: '?query=hello&test=asd' } as any,
+    history: {} as any,
+    match: {} as any,
+    route: {
+      path: '/',
+      component: () => <div />,
+    },
+    ...overrides,
+  };
+
+  render(
+    <BrowserRouter>
+      <GrafanaContext.Provider value={getGrafanaContextMock()}>
+        <GrafanaRoute {...props} />
+      </GrafanaContext.Provider>
+    </BrowserRouter>
+  );
+}
 
 describe('GrafanaRoute', () => {
   beforeEach(() => {
@@ -21,16 +43,31 @@ describe('GrafanaRoute', () => {
       return <div />;
     };
 
-    const location = { search: '?query=hello&test=asd' } as any;
-    const history = {} as any;
-    const match = {} as any;
-
-    render(
-      <GrafanaContext.Provider value={getGrafanaContextMock()}>
-        <GrafanaRoute location={location} history={history} match={match} route={{ component: PageComponent } as any} />
-      </GrafanaContext.Provider>
-    );
-
+    setup({ route: { component: PageComponent, path: '' } });
     expect(capturedProps.queryParams.query).toBe('hello');
   });
+
+  it('Shows loading on lazy load', async () => {
+    const PageComponent = React.lazy(() => {
+      return new Promise<{ default: ComponentType }>(() => {});
+    });
+
+    setup({ route: { component: PageComponent, path: '' } });
+
+    expect(await screen.findByText('Loading...')).toBeInTheDocument();
+  });
+
+  it('Shows error on page error', async () => {
+    const PageComponent = () => {
+      throw new Error('Page threw error');
+    };
+
+    const consoleError = jest.fn();
+    jest.spyOn(console, 'error').mockImplementation(consoleError);
+
+    setup({ route: { component: PageComponent, path: '' } });
+
+    expect(await screen.findByRole('heading', { name: 'An unexpected error happend' })).toBeInTheDocument();
+    expect(consoleError).toHaveBeenCalled();
+  });
 });

public/app/core/navigation/GrafanaRoute.tsx

@@ -1,11 +1,14 @@
-import React, { useEffect } from 'react';
+import React, { Suspense, useEffect } from 'react';
 // @ts-ignore
 import Drop from 'tether-drop';
 
 import { locationSearchToObject, navigationLogger, reportPageview } from '@grafana/runtime';
+import { ErrorBoundary } from '@grafana/ui';
 
 import { useGrafana } from '../context/GrafanaContext';
 
+import { GrafanaRouteError } from './GrafanaRouteError';
+import { GrafanaRouteLoading } from './GrafanaRouteLoading';
 import { GrafanaRouteComponentProps, RouteDescriptor } from './types';
 
 export interface Props extends Omit<GrafanaRouteComponentProps, 'queryParams'> {}
@@ -38,7 +41,21 @@ export function GrafanaRoute(props: Props) {
 
   navigationLogger('GrafanaRoute', false, 'Rendered', props.route);
 
-  return <props.route.component {...props} queryParams={locationSearchToObject(props.location.search)} />;
+  return (
+    <ErrorBoundary>
+      {({ error, errorInfo }) => {
+        if (error) {
+          return <GrafanaRouteError error={error} errorInfo={errorInfo} />;
+        }
+
+        return (
+          <Suspense fallback={<GrafanaRouteLoading />}>
+            <props.route.component {...props} queryParams={locationSearchToObject(props.location.search)} />
+          </Suspense>
+        );
+      }}
+    </ErrorBoundary>
+  );
 }
 
 function getPageClasses(route: RouteDescriptor) {

public/app/core/navigation/GrafanaRouteError.tsx

@@ -0,0 +1,55 @@
+import { css } from '@emotion/css';
+import React, { ErrorInfo, useEffect } from 'react';
+import { useLocation } from 'react-router-dom';
+
+import { locationUtil, PageLayoutType } from '@grafana/data';
+import { Button, ErrorWithStack, stylesFactory } from '@grafana/ui';
+
+import { Page } from '../components/Page/Page';
+
+interface Props {
+  error: Error | null;
+  errorInfo: ErrorInfo | null;
+}
+
+export function GrafanaRouteError({ error, errorInfo }: Props) {
+  const location = useLocation();
+  const isChunkLoadingError = error?.name === 'ChunkLoadError';
+
+  useEffect(() => {
+    // Auto reload page 1 time if we have a chunk load error
+    if (isChunkLoadingError && location.search.indexOf('chunkNotFound') === -1) {
+      window.location.href = locationUtil.getUrlForPartial(location, { chunkNotFound: true });
+    }
+  }, [location, isChunkLoadingError]);
+
+  // Would be good to know the page navId here but needs a pretty big refactoring
+
+  return (
+    <Page navId="error" layout={PageLayoutType.Canvas}>
+      <div className={getStyles()}>
+        {isChunkLoadingError && (
+          <div>
+            <h2>Unable to find application file</h2>
+            <br />
+            <h2 className="page-heading">Grafana has likely been updated. Please try reloading the page.</h2>
+            <br />
+            <div className="gf-form-group">
+              <Button size="md" variant="secondary" icon="repeat" onClick={() => window.location.reload()}>
+                Reload
+              </Button>
+            </div>
+            <ErrorWithStack title={'Error details'} error={error} errorInfo={errorInfo} />
+          </div>
+        )}
+        {!isChunkLoadingError && (
+          <ErrorWithStack title={'An unexpected error happend'} error={error} errorInfo={errorInfo} />
+        )}
+      </div>
+    </Page>
+  );
+}
+
+const getStyles = stylesFactory(() => {
+  return css``;
+});

public/app/core/navigation/GrafanaRouteLoading.tsx

@@ -0,0 +1,24 @@
+import { css } from '@emotion/css';
+import React from 'react';
+
+import { LoadingPlaceholder, useStyles2 } from '@grafana/ui';
+
+export function GrafanaRouteLoading() {
+  const styles = useStyles2(getStyles);
+
+  return (
+    <div className={styles.loadingPage}>
+      <LoadingPlaceholder text={'Loading...'} />
+    </div>
+  );
+}
+
+const getStyles = () => ({
+  loadingPage: css({
+    height: '100%',
+    flexDrection: 'column',
+    display: 'flex',
+    justifyContent: 'center',
+    alignItems: 'center',
+  }),
+});

public/app/core/reducers/navModel.ts

@@ -23,6 +23,7 @@ function buildNavIndex(navIndex: NavIndex, children: NavModelItem[], parentItem?
   }
 
   navIndex['not-found'] = { ...buildWarningNav('Page not found', '404 Error').node };
+  navIndex['error'] = { ...buildWarningNav('Page error', 'An unexpected error').node };
 }
 
 function buildWarningNav(text: string, subTitle?: string): NavModel {

yarn.lock

@@ -12225,16 +12225,6 @@ __metadata:
   languageName: node
   linkType: hard
 
-"@types/react-loadable@npm:5.5.6":
-  version: 5.5.6
-  resolution: "@types/react-loadable@npm:5.5.6"
-  dependencies:
-    "@types/react": "*"
-    "@types/webpack": ^4
-  checksum: 395ff992cf8dbbd425aa79cc298987b464eab875b7057cb4475bfcabd17780d074cf3b7e2511f494fb8e4105a3fe5c9218237f8205f498c38b63671605463415
-  languageName: node
-  linkType: hard
-
 "@types/react-redux@npm:7.1.24":
   version: 7.1.24
   resolution: "@types/react-redux@npm:7.1.24"
@@ -22344,7 +22334,6 @@ __metadata:
     "@types/react-dom": 17.0.14
     "@types/react-grid-layout": 1.3.2
     "@types/react-highlight-words": 0.16.4
-    "@types/react-loadable": 5.5.6
     "@types/react-redux": 7.1.24
     "@types/react-resizable": 3.0.2
     "@types/react-router-dom": 5.3.3
@@ -22492,7 +22481,6 @@ __metadata:
     react-highlight-words: 0.18.0
     react-hook-form: 7.5.3
     react-inlinesvg: 3.0.0
-    react-loadable: 5.5.0
     react-moveable: 0.38.4
     react-popper: 2.3.0
     react-popper-tooltip: 4.4.2
@@ -31359,7 +31347,7 @@ __metadata:
   languageName: node
   linkType: hard
 
-"prop-types@npm:15.x, prop-types@npm:^15.0.0, prop-types@npm:^15.5.0, prop-types@npm:^15.5.10, prop-types@npm:^15.5.4, prop-types@npm:^15.5.7, prop-types@npm:^15.5.8, prop-types@npm:^15.6.0, prop-types@npm:^15.6.2, prop-types@npm:^15.7.0, prop-types@npm:^15.7.2":
+"prop-types@npm:15.x, prop-types@npm:^15.0.0, prop-types@npm:^15.5.10, prop-types@npm:^15.5.4, prop-types@npm:^15.5.7, prop-types@npm:^15.5.8, prop-types@npm:^15.6.0, prop-types@npm:^15.6.2, prop-types@npm:^15.7.0, prop-types@npm:^15.7.2":
   version: 15.7.2
   resolution: "prop-types@npm:15.7.2"
   dependencies:
@@ -32571,17 +32559,6 @@ __metadata:
   languageName: node
   linkType: hard
 
-"react-loadable@npm:5.5.0":
-  version: 5.5.0
-  resolution: "react-loadable@npm:5.5.0"
-  dependencies:
-    prop-types: ^15.5.0
-  peerDependencies:
-    react: "*"
-  checksum: 72329cfd2f2c8b3d4666acf97c36e9d653a3620970d2702d22282b1eb4bcb3c709695d06489f2844da2d541e42e45bcf43fe89eeedb508680f071984cee0eddd
-  languageName: node
-  linkType: hard
-
 "react-moveable@npm:0.38.4, react-moveable@npm:~0.38.4":
   version: 0.38.4
   resolution: "react-moveable@npm:0.38.4"