From 1279a33259a967b731605046fcbc1dfed9c0a835 Mon Sep 17 00:00:00 2001 From: Oliver Sand Date: Tue, 6 Apr 2021 15:23:52 +0200 Subject: [PATCH 1/2] Handle chunk loading errors Signed-off-by: Oliver Sand --- .changeset/forty-queens-hear.md | 7 +++ packages/core-api/src/app/types.ts | 2 +- .../core-api/src/extensions/extensions.tsx | 56 +++++++++++-------- packages/core/src/api-wrappers/createApp.tsx | 34 ++++++++--- 4 files changed, 67 insertions(+), 32 deletions(-) create mode 100644 .changeset/forty-queens-hear.md diff --git a/.changeset/forty-queens-hear.md b/.changeset/forty-queens-hear.md new file mode 100644 index 0000000000..5e0b35162a --- /dev/null +++ b/.changeset/forty-queens-hear.md @@ -0,0 +1,7 @@ +--- +'@backstage/core': patch +'@backstage/core-api': patch +--- + +Introduce a `load-chunk` step in the `BootErrorPage` to show make chunk loading +errors visible to the user. diff --git a/packages/core-api/src/app/types.ts b/packages/core-api/src/app/types.ts index 5bdf51837d..cf3aaac931 100644 --- a/packages/core-api/src/app/types.ts +++ b/packages/core-api/src/app/types.ts @@ -24,7 +24,7 @@ import { AppConfig } from '@backstage/config'; import { SubRouteRef } from '../routing/types'; export type BootErrorPageProps = { - step: 'load-config'; + step: 'load-config' | 'load-chunk'; error: Error; }; diff --git a/packages/core-api/src/extensions/extensions.tsx b/packages/core-api/src/extensions/extensions.tsx index 9463e437e3..8ff4055e61 100644 --- a/packages/core-api/src/extensions/extensions.tsx +++ b/packages/core-api/src/extensions/extensions.tsx @@ -15,9 +15,10 @@ */ import React, { lazy, Suspense } from 'react'; +import { useApp } from '../app'; +import { BackstagePlugin, Extension } from '../plugin/types'; import { RouteRef, useRouteRef } from '../routing'; import { attachComponentData } from './componentData'; -import { Extension, BackstagePlugin } from '../plugin/types'; type ComponentLoader = | { @@ -40,30 +41,41 @@ export function createRoutableExtension< return createReactExtension({ component: { lazy: () => - component().then(InnerComponent => { - const RoutableExtensionWrapper: any = (props: any) => { - // Validate that the routing is wired up correctly in the App.tsx - try { - useRouteRef(mountPoint); - } catch { - throw new Error( - `Routable extension component with mount point ${mountPoint} was not discovered in the app element tree. ` + - 'Routable extension components may not be rendered by other components and must be ' + - 'directly available as an element within the App provider component.', - ); - } - return ; - }; + component().then( + InnerComponent => { + const RoutableExtensionWrapper: any = (props: any) => { + // Validate that the routing is wired up correctly in the App.tsx + try { + useRouteRef(mountPoint); + } catch { + throw new Error( + `Routable extension component with mount point ${mountPoint} was not discovered in the app element tree. ` + + 'Routable extension components may not be rendered by other components and must be ' + + 'directly available as an element within the App provider component.', + ); + } + return ; + }; - const componentName = - (InnerComponent as { displayName?: string }).displayName || - InnerComponent.name || - 'LazyComponent'; + const componentName = + (InnerComponent as { displayName?: string }).displayName || + InnerComponent.name || + 'LazyComponent'; - RoutableExtensionWrapper.displayName = `RoutableExtension(${componentName})`; + RoutableExtensionWrapper.displayName = `RoutableExtension(${componentName})`; - return RoutableExtensionWrapper as T; - }), + return RoutableExtensionWrapper as T; + }, + error => { + const RoutableExtensionWrapper: any = (_: any) => { + const app = useApp(); + const { BootErrorPage } = app.getComponents(); + + return ; + }; + return RoutableExtensionWrapper; + }, + ), }, data: { 'core.mountPoint': mountPoint, diff --git a/packages/core/src/api-wrappers/createApp.tsx b/packages/core/src/api-wrappers/createApp.tsx index db8d0d0636..b071c2d014 100644 --- a/packages/core/src/api-wrappers/createApp.tsx +++ b/packages/core/src/api-wrappers/createApp.tsx @@ -97,16 +97,32 @@ export function createApp(options?: AppOptions) { ); const DefaultBootErrorPage = ({ step, error }: BootErrorPageProps) => { - let message = ''; - if (step === 'load-config') { - message = `The configuration failed to load, someone should have a look at this error: ${error.message}`; + switch (step) { + case 'load-config': + // TODO: figure out a nicer way to handle routing on the error page, when it can be done. + return ( + + + + ); + case 'load-chunk': + return ( + + ); + default: + // TODO: figure out a nicer way to handle routing on the error page, when it can be done. + return ( + + + + ); } - // TODO: figure out a nicer way to handle routing on the error page, when it can be done. - return ( - - - - ); }; const apis = options?.apis ?? []; From 21f80f2cfd59b42fbabeeac6b48776348f8af89a Mon Sep 17 00:00:00 2001 From: Oliver Sand Date: Tue, 6 Apr 2021 18:26:49 +0200 Subject: [PATCH 2/2] Inject a router if needed Signed-off-by: Oliver Sand --- .../core/src/api-wrappers/createApp.test.tsx | 25 ++++++++- packages/core/src/api-wrappers/createApp.tsx | 51 +++++++++---------- 2 files changed, 48 insertions(+), 28 deletions(-) diff --git a/packages/core/src/api-wrappers/createApp.test.tsx b/packages/core/src/api-wrappers/createApp.test.tsx index 31f584b68c..7eaec32b86 100644 --- a/packages/core/src/api-wrappers/createApp.test.tsx +++ b/packages/core/src/api-wrappers/createApp.test.tsx @@ -14,7 +14,10 @@ * limitations under the License. */ -import { defaultConfigLoader } from './createApp'; +import { render } from '@testing-library/react'; +import React from 'react'; +import { MemoryRouter } from 'react-router'; +import { defaultConfigLoader, OptionallyWrapInRouter } from './createApp'; (process as any).env = { NODE_ENV: 'test' }; const anyEnv = process.env as any; @@ -92,3 +95,23 @@ describe('defaultConfigLoader', () => { ]); }); }); + +describe('OptionallyWrapInRouter', () => { + it('should wrap with router if not yet inside a router', async () => { + const { getByText } = render( + Test, + ); + + expect(getByText('Test')).toBeInTheDocument(); + }); + + it('should not wrap with router if already inside a router', async () => { + const { getByText } = render( + + Test + , + ); + + expect(getByText('Test')).toBeInTheDocument(); + }); +}); diff --git a/packages/core/src/api-wrappers/createApp.tsx b/packages/core/src/api-wrappers/createApp.tsx index b071c2d014..e79cdf509f 100644 --- a/packages/core/src/api-wrappers/createApp.tsx +++ b/packages/core/src/api-wrappers/createApp.tsx @@ -14,14 +14,18 @@ * limitations under the License. */ -import React from 'react'; +import React, { PropsWithChildren } from 'react'; import privateExports, { AppOptions, defaultSystemIcons, BootErrorPageProps, AppConfigLoader, } from '@backstage/core-api'; -import { BrowserRouter, MemoryRouter } from 'react-router-dom'; +import { + BrowserRouter, + MemoryRouter, + useInRouterContext, +} from 'react-router-dom'; import LightIcon from '@material-ui/icons/WbSunny'; import DarkIcon from '@material-ui/icons/Brightness2'; import { ErrorPage } from '../layout/ErrorPage'; @@ -89,6 +93,13 @@ export const defaultConfigLoader: AppConfigLoader = async ( // The actual implementation of the app class still lives in core-api, // as it needs to be used by dev- and test-utils. +export function OptionallyWrapInRouter({ children }: PropsWithChildren<{}>) { + if (useInRouterContext()) { + return <>{children}; + } + return {children}; +} + /** * Creates a new Backstage App. */ @@ -97,32 +108,18 @@ export function createApp(options?: AppOptions) { ); const DefaultBootErrorPage = ({ step, error }: BootErrorPageProps) => { - switch (step) { - case 'load-config': - // TODO: figure out a nicer way to handle routing on the error page, when it can be done. - return ( - - - - ); - case 'load-chunk': - return ( - - ); - default: - // TODO: figure out a nicer way to handle routing on the error page, when it can be done. - return ( - - - - ); + let message = ''; + if (step === 'load-config') { + message = `The configuration failed to load, someone should have a look at this error: ${error.message}`; + } else if (step === 'load-chunk') { + message = `Lazy loaded chunk failed to load, try to reload the page: ${error.message}`; } + // TODO: figure out a nicer way to handle routing on the error page, when it can be done. + return ( + + + + ); }; const apis = options?.apis ?? [];