From 96dfc4bac573a4a006ec2e4087c8c96f36b0e593 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torkel=20=C3=96degaard?= Date: Wed, 21 Sep 2022 05:41:05 +0200 Subject: [PATCH] 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 | 3 +- package.json | 2 - .../DynamicImports/ErrorLoadingChunk.tsx | 46 -------------- .../LoadingChunkPlaceHolder.tsx | 11 ---- .../DynamicImports/SafeDynamicImport.test.tsx | 37 ----------- .../DynamicImports/SafeDynamicImport.tsx | 24 +------ .../app/core/navigation/GrafanaRoute.test.tsx | 63 +++++++++++++++---- public/app/core/navigation/GrafanaRoute.tsx | 21 ++++++- .../app/core/navigation/GrafanaRouteError.tsx | 55 ++++++++++++++++ .../core/navigation/GrafanaRouteLoading.tsx | 24 +++++++ public/app/core/reducers/navModel.ts | 1 + yarn.lock | 25 +------- 12 files changed, 152 insertions(+), 160 deletions(-) delete mode 100644 public/app/core/components/DynamicImports/ErrorLoadingChunk.tsx delete mode 100644 public/app/core/components/DynamicImports/LoadingChunkPlaceHolder.tsx delete mode 100644 public/app/core/components/DynamicImports/SafeDynamicImport.test.tsx create mode 100644 public/app/core/navigation/GrafanaRouteError.tsx create mode 100644 public/app/core/navigation/GrafanaRouteLoading.tsx diff --git a/.betterer.results b/.betterer.results index 48903832942..975d6cc369f 100644 --- a/.betterer.results +++ b/.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"] diff --git a/package.json b/package.json index b5ed070b6d0..1e48a046c6c 100644 --- a/package.json +++ b/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", diff --git a/public/app/core/components/DynamicImports/ErrorLoadingChunk.tsx b/public/app/core/components/DynamicImports/ErrorLoadingChunk.tsx deleted file mode 100644 index 927ce787251..00000000000 --- a/public/app/core/components/DynamicImports/ErrorLoadingChunk.tsx +++ /dev/null @@ -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 = ({ error }) => { - const [params, updateUrlParams] = useUrlParams(); - - if (!params.get('chunkNotFound')) { - updateUrlParams({ chunkNotFound: true }, true); - window.location.reload(); - } - - return ( -
-

Unable to find application file

-
-

Grafana has likely been updated. Please try reloading the page.

-
-
- -
-
- {error && error.message ? error.message : 'Unexpected error occurred'} -
- {error && error.stack ? error.stack : null} -
-
- ); -}; - -ErrorLoadingChunk.displayName = 'ErrorLoadingChunk'; diff --git a/public/app/core/components/DynamicImports/LoadingChunkPlaceHolder.tsx b/public/app/core/components/DynamicImports/LoadingChunkPlaceHolder.tsx deleted file mode 100644 index 220310882a2..00000000000 --- a/public/app/core/components/DynamicImports/LoadingChunkPlaceHolder.tsx +++ /dev/null @@ -1,11 +0,0 @@ -import React, { FunctionComponent } from 'react'; - -import { LoadingPlaceholder } from '@grafana/ui'; - -export const LoadingChunkPlaceHolder: FunctionComponent = React.memo(() => ( -
- -
-)); - -LoadingChunkPlaceHolder.displayName = 'LoadingChunkPlaceHolder'; diff --git a/public/app/core/components/DynamicImports/SafeDynamicImport.test.tsx b/public/app/core/components/DynamicImports/SafeDynamicImport.test.tsx deleted file mode 100644 index 6cd3ffd23ad..00000000000 --- a/public/app/core/components/DynamicImports/SafeDynamicImport.test.tsx +++ /dev/null @@ -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(); - }); - }); - - 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(); - }); - }); -}); diff --git a/public/app/core/components/DynamicImports/SafeDynamicImport.tsx b/public/app/core/components/DynamicImports/SafeDynamicImport.tsx index 193ed6712c4..d8fe4c01722 100644 --- a/public/app/core/components/DynamicImports/SafeDynamicImport.tsx +++ b/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 ; - } - - if (pastDelay) { - return ; - } - - return null; -}; - -export const SafeDynamicImport = (loader: () => Promise): GrafanaRouteComponent => - Loadable({ - loader: loader, - loading: loadComponentHandler, - }); +export const SafeDynamicImport = (loader: () => Promise): GrafanaRouteComponent => React.lazy(loader); diff --git a/public/app/core/navigation/GrafanaRoute.test.tsx b/public/app/core/navigation/GrafanaRoute.test.tsx index 258f4d568c4..5aae04ab446 100644 --- a/public/app/core/navigation/GrafanaRoute.test.tsx +++ b/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) { + const props: Props = { + location: { search: '?query=hello&test=asd' } as any, + history: {} as any, + match: {} as any, + route: { + path: '/', + component: () =>
, + }, + ...overrides, + }; + + render( + + + + + + ); +} describe('GrafanaRoute', () => { beforeEach(() => { @@ -21,16 +43,31 @@ describe('GrafanaRoute', () => { return
; }; - const location = { search: '?query=hello&test=asd' } as any; - const history = {} as any; - const match = {} as any; - - render( - - - - ); - + 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(); + }); }); diff --git a/public/app/core/navigation/GrafanaRoute.tsx b/public/app/core/navigation/GrafanaRoute.tsx index e6521382f43..9cfa4f6e224 100644 --- a/public/app/core/navigation/GrafanaRoute.tsx +++ b/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 {} @@ -38,7 +41,21 @@ export function GrafanaRoute(props: Props) { navigationLogger('GrafanaRoute', false, 'Rendered', props.route); - return ; + return ( + + {({ error, errorInfo }) => { + if (error) { + return ; + } + + return ( + }> + + + ); + }} + + ); } function getPageClasses(route: RouteDescriptor) { diff --git a/public/app/core/navigation/GrafanaRouteError.tsx b/public/app/core/navigation/GrafanaRouteError.tsx new file mode 100644 index 00000000000..97d0d4eb36c --- /dev/null +++ b/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 ( + +
+ {isChunkLoadingError && ( +
+

Unable to find application file

+
+

Grafana has likely been updated. Please try reloading the page.

+
+
+ +
+ +
+ )} + {!isChunkLoadingError && ( + + )} +
+ + ); +} + +const getStyles = stylesFactory(() => { + return css``; +}); diff --git a/public/app/core/navigation/GrafanaRouteLoading.tsx b/public/app/core/navigation/GrafanaRouteLoading.tsx new file mode 100644 index 00000000000..558c264b0ed --- /dev/null +++ b/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 ( +
+ +
+ ); +} + +const getStyles = () => ({ + loadingPage: css({ + height: '100%', + flexDrection: 'column', + display: 'flex', + justifyContent: 'center', + alignItems: 'center', + }), +}); diff --git a/public/app/core/reducers/navModel.ts b/public/app/core/reducers/navModel.ts index 7aa5057c4d1..16ec84daffb 100644 --- a/public/app/core/reducers/navModel.ts +++ b/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 { diff --git a/yarn.lock b/yarn.lock index fa2b564a187..0b69f56ea79 100644 --- a/yarn.lock +++ b/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"