From 75b8537ce11757120f878ac7394ef4d1b2bab651 Mon Sep 17 00:00:00 2001 From: Juan Lulkin Date: Tue, 11 May 2021 13:34:43 +0200 Subject: [PATCH 1/4] Adds error boundary to extensions Signed-off-by: Juan Lulkin --- .changeset/few-cats-type.md | 33 ++++ packages/app/src/App.tsx | 4 + packages/core-api/src/app/App.test.tsx | 1 + packages/core-api/src/app/types.ts | 12 +- .../src/extensions/PluginErrorBoundary.tsx | 56 +++++++ .../src/extensions/extensions.test.tsx | 49 ++++++ .../core-api/src/extensions/extensions.tsx | 28 +++- packages/core-api/src/routing/hooks.test.tsx | 148 ++++++++++-------- packages/core/src/api-wrappers/createApp.tsx | 2 + .../ErrorBoundaryFallback.tsx | 41 +++++ .../components/ErrorBoundaryFallback/index.ts | 17 ++ .../ResponseErrorPanel/ResponseErrorPanel.tsx | 28 +++- .../components/WarningPanel/WarningPanel.tsx | 5 +- packages/core/src/components/index.ts | 1 + .../test-utils/src/testUtils/appWrappers.tsx | 1 + 15 files changed, 345 insertions(+), 81 deletions(-) create mode 100644 .changeset/few-cats-type.md create mode 100644 packages/core-api/src/extensions/PluginErrorBoundary.tsx create mode 100644 packages/core/src/components/ErrorBoundaryFallback/ErrorBoundaryFallback.tsx create mode 100644 packages/core/src/components/ErrorBoundaryFallback/index.ts diff --git a/.changeset/few-cats-type.md b/.changeset/few-cats-type.md new file mode 100644 index 0000000000..7d4c0f69cb --- /dev/null +++ b/.changeset/few-cats-type.md @@ -0,0 +1,33 @@ +--- +'@backstage/core': minor +'@backstage/core-api': patch +--- + +This change adds error boundary on extensions. + +This means all exposed parts of a plugin are wrapped in a general error boundary component, that is plugin aware. The default design for the error box is borrowed from @backstage/errors. To override the default "fallback" one must provide a component named `ErrorBoundaryFallback` to `createApp`, like so: + +```ts +const app = createApp({ + components: { + ErrorBoundaryFallback: props => { + // a custom fallback component + return ( + <> +

Oops.

+

+ The plugin {props.plugin.getId()} failed with {props.error.message} +

+ + + ); + }, + }, +}); +``` + +The props here include: + +- `error`. An Error object or something that inherits it that represents the error that was thrown from any inner component. +- `resetError`. A callback that will simply mount the children of the error boundary again. +- `plugin`. A BackstagePlugin that can be used to lookup info to be presented in the error message. For instance, you may want to keep a map of your internal plugins and team names or slack channels and present these when an error occurs. Typically, you'll do that by getting the plugin id with `plugin.getId()`. diff --git a/packages/app/src/App.tsx b/packages/app/src/App.tsx index 5874465ef6..927e9b4327 100644 --- a/packages/app/src/App.tsx +++ b/packages/app/src/App.tsx @@ -20,6 +20,7 @@ import { FlatRoutes, OAuthRequestDialog, SignInPage, + ErrorBoundaryFallback, } from '@backstage/core'; import { apiDocsPlugin, ApiExplorerPage } from '@backstage/plugin-api-docs'; import { @@ -66,6 +67,9 @@ const app = createApp({ alert: AlarmIcon, }, components: { + ErrorBoundaryFallback: props => { + return ; + }, SignInPage: props => { return ( { BootErrorPage: () => null, Progress: () => null, Router: BrowserRouter, + ErrorBoundaryFallback: () => null, }; it('runs happy paths', async () => { diff --git a/packages/core-api/src/app/types.ts b/packages/core-api/src/app/types.ts index df0f65f33f..40e9913201 100644 --- a/packages/core-api/src/app/types.ts +++ b/packages/core-api/src/app/types.ts @@ -14,13 +14,14 @@ * limitations under the License. */ +import { AppConfig } from '@backstage/config'; import { ComponentType } from 'react'; +import { AppTheme, ProfileInfo } from '../apis/definitions'; +import { AnyApiFactory } from '../apis/system'; +import type { ErrorBoundaryFallbackProps } from '../extensions/PluginErrorBoundary'; import { IconComponent, IconComponentMap, IconKey } from '../icons/types'; import { AnyExternalRoutes, BackstagePlugin } from '../plugin/types'; import { ExternalRouteRef, RouteRef, SubRouteRef } from '../routing/types'; -import { AnyApiFactory } from '../apis/system'; -import { AppTheme, ProfileInfo } from '../apis/definitions'; -import { AppConfig } from '@backstage/config'; export type BootErrorPageProps = { step: 'load-config' | 'load-chunk'; @@ -58,6 +59,11 @@ export type AppComponents = { BootErrorPage: ComponentType; Progress: ComponentType<{}>; Router: ComponentType<{}>; + ErrorBoundaryFallback: ComponentType< + ErrorBoundaryFallbackProps & { + plugin: BackstagePlugin; + } + >; /** * An optional sign-in page that will be rendered instead of the AppRouter at startup. diff --git a/packages/core-api/src/extensions/PluginErrorBoundary.tsx b/packages/core-api/src/extensions/PluginErrorBoundary.tsx new file mode 100644 index 0000000000..4cf772a638 --- /dev/null +++ b/packages/core-api/src/extensions/PluginErrorBoundary.tsx @@ -0,0 +1,56 @@ +/* + * Copyright 2021 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import React from 'react'; + +export type ErrorBoundaryFallbackProps = { + error: Error; + resetError: () => void; +}; + +type FallbackRender = React.FunctionComponent; + +type Props = { + fallbackRender: FallbackRender; +}; + +type State = { error?: Error }; + +export class PluginErrorBoundary extends React.Component { + static getDerivedStateFromError(error: Error) { + return { error }; + } + + state: State = { error: undefined }; + + resetError = () => { + this.setState({ error: undefined }); + }; + + render() { + const { error } = this.state; + const { fallbackRender } = this.props; + + if (error) { + return fallbackRender({ + error, + resetError: this.resetError, + }); + } + + return this.props.children; + } +} diff --git a/packages/core-api/src/extensions/extensions.test.tsx b/packages/core-api/src/extensions/extensions.test.tsx index 26755b3bcf..db67ed18c9 100644 --- a/packages/core-api/src/extensions/extensions.test.tsx +++ b/packages/core-api/src/extensions/extensions.test.tsx @@ -14,7 +14,13 @@ * limitations under the License. */ +import { errorApiRef } from '../apis/definitions/ErrorApi'; +import { ApiRegistry } from '../apis/system/ApiRegistry'; +import { ApiProvider } from '../apis/system/ApiProvider'; +import { withLogCollector } from '@backstage/test-utils-core'; +import { render, screen } from '@testing-library/react'; import React from 'react'; +import { AppContextProvider } from '../app/AppContext'; import { createPlugin } from '../plugin'; import { createRouteRef } from '../routing'; import { getComponentData } from './componentData'; @@ -23,6 +29,9 @@ import { createReactExtension, createRoutableExtension, } from './extensions'; +import { AppComponents } from '../app/types'; +import { AppContext } from '../app'; +import { MockErrorApi } from '@backstage/test-utils'; const plugin = createPlugin({ id: 'my-plugin', @@ -73,4 +82,44 @@ describe('extensions', () => { expect(getComponentData(element2, 'core.plugin')).toBe(plugin); expect(getComponentData(element2, 'core.mountPoint')).toBe(routeRef); }); + + it('should wrap extended component with error boundary', async () => { + const extension = createComponentExtension({ + component: { + sync: () => { + throw new Error('Test error'); + }, + }, + }); + + const BrokenComponent = plugin.provide(extension); + + const errorApi = new MockErrorApi(); + + const apis = ApiRegistry.from([[errorApiRef, errorApi]]); + + const MockFallback: AppComponents['ErrorBoundaryFallback'] = props => ( + <>Error in {props.plugin.getId()} + ); + + const { error: errors } = await withLogCollector(['error'], async () => { + render( + + ({ + ErrorBoundaryFallback: MockFallback, + }), + } as unknown) as AppContext + } + > + + + , + ); + }); + screen.getByText('Error in my-plugin'); + expect(errors[0]).toMatch('Test error'); + }); }); diff --git a/packages/core-api/src/extensions/extensions.tsx b/packages/core-api/src/extensions/extensions.tsx index 9f6412f960..686c467caf 100644 --- a/packages/core-api/src/extensions/extensions.tsx +++ b/packages/core-api/src/extensions/extensions.tsx @@ -19,6 +19,7 @@ import { useApp } from '../app'; import { BackstagePlugin, Extension } from '../plugin/types'; import { RouteRef, useRouteRef } from '../routing'; import { attachComponentData } from './componentData'; +import { PluginErrorBoundary } from './PluginErrorBoundary'; type ComponentLoader = | { @@ -123,11 +124,28 @@ export function createReactExtension< return { expose(plugin: BackstagePlugin) { - const Result: any = (props: any) => ( - - - - ); + const Result: any = (props: any) => { + const app = useApp(); + const { ErrorBoundaryFallback } = app.getComponents(); + + return ( + + { + return ( + + ); + }} + > + + + + ); + }; attachComponentData(Result, 'core.plugin', plugin); for (const [key, value] of Object.entries(data)) { diff --git a/packages/core-api/src/routing/hooks.test.tsx b/packages/core-api/src/routing/hooks.test.tsx index 8a9b70ed7d..a8fdfda5d0 100644 --- a/packages/core-api/src/routing/hooks.test.tsx +++ b/packages/core-api/src/routing/hooks.test.tsx @@ -14,6 +14,7 @@ * limitations under the License. */ +import { AppContext } from '../app'; import { render } from '@testing-library/react'; import { renderHook } from '@testing-library/react-hooks'; import React, { @@ -23,6 +24,7 @@ import React, { useContext, } from 'react'; import { MemoryRouter, Route, Routes } from 'react-router-dom'; +import { AppContextProvider } from '../app/AppContext'; import { createRoutableExtension } from '../extensions'; import { childDiscoverer, @@ -154,23 +156,27 @@ function withRoutingProvider( ); } +const appContext = ({ getComponents: () => ({}) } as unknown) as AppContext; + describe('discovery', () => { it('should handle simple routeRef path creation for routeRefs used in other parts of the app', async () => { const root = ( - - - - - - - - - - - - - - + + + + + + + + + + + + + + + + ); const rendered = render( @@ -205,23 +211,25 @@ describe('discovery', () => { it('should handle routeRefs with parameters', async () => { const root = ( - - - - - - - - + + + + + + + + + + ); const rendered = render(withRoutingProvider(root)); @@ -236,8 +244,37 @@ describe('discovery', () => { it('should handle relative routing within parameterized routePaths', async () => { const root = ( - - + + + + + + + + + + + + + + + ); + + const rendered = render(withRoutingProvider(root)); + + await expect( + rendered.findByText('Path at inside: /foo/blob/baz'), + ).resolves.toBeInTheDocument(); + }); + + it('should throw errors for routing to other routeRefs with unsupported parameters', () => { + const root = ( + + @@ -250,33 +287,8 @@ describe('discovery', () => { routeRef={ref3} params={{ id: 'blob' }} /> - - - ); - - const rendered = render(withRoutingProvider(root)); - - await expect( - rendered.findByText('Path at inside: /foo/blob/baz'), - ).resolves.toBeInTheDocument(); - }); - - it('should throw errors for routing to other routeRefs with unsupported parameters', () => { - const root = ( - - - - - - - - - - + + ); const rendered = render(withRoutingProvider(root)); @@ -295,13 +307,15 @@ describe('discovery', () => { it('should handle relative routing of parameterized routePaths with duplicate param names', () => { const root = ( - - - - - - - + + + + + + + + + ); const { routePaths, routeParents } = traverseElementTree({ diff --git a/packages/core/src/api-wrappers/createApp.tsx b/packages/core/src/api-wrappers/createApp.tsx index e79cdf509f..9b8524c89c 100644 --- a/packages/core/src/api-wrappers/createApp.tsx +++ b/packages/core/src/api-wrappers/createApp.tsx @@ -30,6 +30,7 @@ import LightIcon from '@material-ui/icons/WbSunny'; import DarkIcon from '@material-ui/icons/Brightness2'; import { ErrorPage } from '../layout/ErrorPage'; import { Progress } from '../components/Progress'; +import { ErrorBoundaryFallback } from '../components/ErrorBoundaryFallback'; import { defaultApis } from './defaultApis'; import { lightTheme, darkTheme } from '@backstage/theme'; import { AppConfig, JsonObject } from '@backstage/config'; @@ -130,6 +131,7 @@ export function createApp(options?: AppOptions) { BootErrorPage: DefaultBootErrorPage, Progress: Progress, Router: BrowserRouter, + ErrorBoundaryFallback: ErrorBoundaryFallback, ...options?.components, }; const themes = options?.themes ?? [ diff --git a/packages/core/src/components/ErrorBoundaryFallback/ErrorBoundaryFallback.tsx b/packages/core/src/components/ErrorBoundaryFallback/ErrorBoundaryFallback.tsx new file mode 100644 index 0000000000..f1364061df --- /dev/null +++ b/packages/core/src/components/ErrorBoundaryFallback/ErrorBoundaryFallback.tsx @@ -0,0 +1,41 @@ +/* + * Copyright 2020 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// TODO align this design with the backend errors + +import React from 'react'; +import { AppComponents } from '../..'; +import { ResponseErrorPanel } from '../ResponseErrorPanel'; +import { Button } from '@material-ui/core'; + +export const ErrorBoundaryFallback: AppComponents['ErrorBoundaryFallback'] = ({ + error, + resetError, + plugin, +}) => { + return ( + + Retry + + } + /> + ); +}; diff --git a/packages/core/src/components/ErrorBoundaryFallback/index.ts b/packages/core/src/components/ErrorBoundaryFallback/index.ts new file mode 100644 index 0000000000..ff40974ed7 --- /dev/null +++ b/packages/core/src/components/ErrorBoundaryFallback/index.ts @@ -0,0 +1,17 @@ +/* + * Copyright 2020 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +export { ErrorBoundaryFallback } from './ErrorBoundaryFallback'; diff --git a/packages/core/src/components/ResponseErrorPanel/ResponseErrorPanel.tsx b/packages/core/src/components/ResponseErrorPanel/ResponseErrorPanel.tsx index 52f7ab03f9..42adf1980c 100644 --- a/packages/core/src/components/ResponseErrorPanel/ResponseErrorPanel.tsx +++ b/packages/core/src/components/ResponseErrorPanel/ResponseErrorPanel.tsx @@ -45,6 +45,7 @@ type ResponseErrorListProps = { request?: string; stack?: string; json?: string; + actions?: React.ReactNode; }; const ResponseErrorList = ({ @@ -53,6 +54,7 @@ const ResponseErrorList = ({ message, stack, json, + actions, }: ResponseErrorListProps) => { const classes = useStyles(); @@ -107,12 +109,21 @@ const ResponseErrorList = ({ )} + {actions && ( + <> + + {actions} + + )} ); }; type Props = { error: Error; + defaultExpanded?: boolean; + title?: string; + actions?: React.ReactNode; }; /** @@ -121,13 +132,14 @@ type Props = { * Has special treatment for ResponseError errors, to display rich * server-provided information about what happened. */ -export const ResponseErrorDetails = ({ error }: Props) => { +export const ResponseErrorDetails = ({ error, actions }: Props) => { if (error.name !== 'ResponseError') { return ( ); } @@ -158,10 +170,18 @@ export const ResponseErrorDetails = ({ error }: Props) => { * Has special treatment for ResponseError errors, to display rich * server-provided information about what happened. */ -export const ResponseErrorPanel = ({ error }: Props) => { +export const ResponseErrorPanel = ({ + title, + error, + defaultExpanded, + actions, +}: Props) => { return ( - - + + ); }; diff --git a/packages/core/src/components/WarningPanel/WarningPanel.tsx b/packages/core/src/components/WarningPanel/WarningPanel.tsx index 1faf3bb142..20c71ee106 100644 --- a/packages/core/src/components/WarningPanel/WarningPanel.tsx +++ b/packages/core/src/components/WarningPanel/WarningPanel.tsx @@ -77,6 +77,7 @@ type Props = { title?: string; severity?: 'warning' | 'error' | 'info'; message?: React.ReactNode; + defaultExpanded?: boolean; children?: React.ReactNode; }; @@ -96,14 +97,14 @@ const capitalize = (s: string) => { */ export const WarningPanel = (props: Props) => { const classes = useStyles(props); - const { severity, title, message, children } = props; + const { severity, title, message, children, defaultExpanded } = props; // If no severity or title provided, the heading will read simply "Warning" const subTitle = (severity ? capitalize(severity) : 'Warning') + (title ? `: ${title}` : ''); return ( - + } className={classes.summary} diff --git a/packages/core/src/components/index.ts b/packages/core/src/components/index.ts index 14f7f3cac0..c8f9803eae 100644 --- a/packages/core/src/components/index.ts +++ b/packages/core/src/components/index.ts @@ -23,6 +23,7 @@ export * from './CopyTextButton'; export * from './DependencyGraph'; export * from './DismissableBanner'; export * from './EmptyState'; +export * from './ErrorBoundaryFallback'; export * from './ResponseErrorPanel'; export * from './FeatureDiscovery'; export * from './HeaderIconLinkRow'; diff --git a/packages/test-utils/src/testUtils/appWrappers.tsx b/packages/test-utils/src/testUtils/appWrappers.tsx index add7b4479c..0086bf3293 100644 --- a/packages/test-utils/src/testUtils/appWrappers.tsx +++ b/packages/test-utils/src/testUtils/appWrappers.tsx @@ -96,6 +96,7 @@ export function wrapInTestApp( Router: ({ children }) => ( ), + ErrorBoundaryFallback: () => null, }, icons: defaultSystemIcons, plugins: [], From 467fe2200c68dbd35b47c03dbc48c7f4b89a3684 Mon Sep 17 00:00:00 2001 From: Juan Lulkin Date: Tue, 11 May 2021 13:39:32 +0200 Subject: [PATCH 2/4] Makes plugin optional in ErrorBoundaryFallback Signed-off-by: Juan Lulkin --- packages/core-api/src/app/types.ts | 2 +- packages/core-api/src/extensions/extensions.test.tsx | 2 +- .../components/ErrorBoundaryFallback/ErrorBoundaryFallback.tsx | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/core-api/src/app/types.ts b/packages/core-api/src/app/types.ts index 40e9913201..333eb06afd 100644 --- a/packages/core-api/src/app/types.ts +++ b/packages/core-api/src/app/types.ts @@ -61,7 +61,7 @@ export type AppComponents = { Router: ComponentType<{}>; ErrorBoundaryFallback: ComponentType< ErrorBoundaryFallbackProps & { - plugin: BackstagePlugin; + plugin?: BackstagePlugin; } >; diff --git a/packages/core-api/src/extensions/extensions.test.tsx b/packages/core-api/src/extensions/extensions.test.tsx index db67ed18c9..773dc32ab2 100644 --- a/packages/core-api/src/extensions/extensions.test.tsx +++ b/packages/core-api/src/extensions/extensions.test.tsx @@ -99,7 +99,7 @@ describe('extensions', () => { const apis = ApiRegistry.from([[errorApiRef, errorApi]]); const MockFallback: AppComponents['ErrorBoundaryFallback'] = props => ( - <>Error in {props.plugin.getId()} + <>Error in {props.plugin?.getId()} ); const { error: errors } = await withLogCollector(['error'], async () => { diff --git a/packages/core/src/components/ErrorBoundaryFallback/ErrorBoundaryFallback.tsx b/packages/core/src/components/ErrorBoundaryFallback/ErrorBoundaryFallback.tsx index f1364061df..0fe7d11c1f 100644 --- a/packages/core/src/components/ErrorBoundaryFallback/ErrorBoundaryFallback.tsx +++ b/packages/core/src/components/ErrorBoundaryFallback/ErrorBoundaryFallback.tsx @@ -28,7 +28,7 @@ export const ErrorBoundaryFallback: AppComponents['ErrorBoundaryFallback'] = ({ }) => { return ( Date: Tue, 11 May 2021 14:39:30 +0200 Subject: [PATCH 3/4] Refactor error panel Signed-off-by: Juan Lulkin --- .../ErrorBoundaryFallback.tsx | 17 +- .../src/components/ErrorPanel/ErrorPanel.tsx | 114 ++++++++++++ .../core/src/components/ErrorPanel/index.ts | 18 ++ .../ResponseErrorPanel/ResponseErrorPanel.tsx | 166 ++++-------------- .../components/ResponseErrorPanel/index.ts | 2 +- 5 files changed, 178 insertions(+), 139 deletions(-) create mode 100644 packages/core/src/components/ErrorPanel/ErrorPanel.tsx create mode 100644 packages/core/src/components/ErrorPanel/index.ts diff --git a/packages/core/src/components/ErrorBoundaryFallback/ErrorBoundaryFallback.tsx b/packages/core/src/components/ErrorBoundaryFallback/ErrorBoundaryFallback.tsx index 0fe7d11c1f..8be43995f1 100644 --- a/packages/core/src/components/ErrorBoundaryFallback/ErrorBoundaryFallback.tsx +++ b/packages/core/src/components/ErrorBoundaryFallback/ErrorBoundaryFallback.tsx @@ -14,11 +14,9 @@ * limitations under the License. */ -// TODO align this design with the backend errors - import React from 'react'; import { AppComponents } from '../..'; -import { ResponseErrorPanel } from '../ResponseErrorPanel'; +import { ErrorPanel } from '../ErrorPanel'; import { Button } from '@material-ui/core'; export const ErrorBoundaryFallback: AppComponents['ErrorBoundaryFallback'] = ({ @@ -27,15 +25,14 @@ export const ErrorBoundaryFallback: AppComponents['ErrorBoundaryFallback'] = ({ plugin, }) => { return ( - - Retry - - } - /> + > + + ); }; diff --git a/packages/core/src/components/ErrorPanel/ErrorPanel.tsx b/packages/core/src/components/ErrorPanel/ErrorPanel.tsx new file mode 100644 index 0000000000..1f7d2a4be7 --- /dev/null +++ b/packages/core/src/components/ErrorPanel/ErrorPanel.tsx @@ -0,0 +1,114 @@ +/* + * Copyright 2021 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { List, ListItem, ListItemText, makeStyles } from '@material-ui/core'; +import React, { PropsWithChildren } from 'react'; +import { CopyTextButton } from '../CopyTextButton'; +import { WarningPanel } from '../WarningPanel'; + +const useStyles = makeStyles(theme => ({ + text: { + fontFamily: 'monospace', + whiteSpace: 'pre', + overflowX: 'auto', + marginRight: theme.spacing(2), + }, + divider: { + margin: theme.spacing(2), + }, +})); + +type ErrorListProps = { + error: string; + message: string; + request?: string; + stack?: string; + json?: string; +}; + +const ErrorList = ({ + error, + message, + stack, + children, +}: PropsWithChildren) => { + const classes = useStyles(); + + return ( + + + + + + + + + + + + {stack && ( + + + + + )} + + {children} + + ); +}; + +export type ErrorPanelProps = { + error: Error; + defaultExpanded?: boolean; + title?: string; +}; + +/** + * Renders a warning panel as the effect of an error. + */ +export const ErrorPanel = ({ + title, + error, + defaultExpanded, + children, +}: PropsWithChildren) => { + return ( + + + + ); +}; diff --git a/packages/core/src/components/ErrorPanel/index.ts b/packages/core/src/components/ErrorPanel/index.ts new file mode 100644 index 0000000000..7103266eab --- /dev/null +++ b/packages/core/src/components/ErrorPanel/index.ts @@ -0,0 +1,18 @@ +/* + * Copyright 2021 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +export { ErrorPanel } from './ErrorPanel'; +export type { ErrorPanelProps } from './ErrorPanel'; diff --git a/packages/core/src/components/ResponseErrorPanel/ResponseErrorPanel.tsx b/packages/core/src/components/ResponseErrorPanel/ResponseErrorPanel.tsx index 42adf1980c..33da6261ec 100644 --- a/packages/core/src/components/ResponseErrorPanel/ResponseErrorPanel.tsx +++ b/packages/core/src/components/ResponseErrorPanel/ResponseErrorPanel.tsx @@ -15,17 +15,11 @@ */ import { ResponseError } from '@backstage/errors'; -import { - Divider, - List, - ListItem, - ListItemText, - makeStyles, -} from '@material-ui/core'; +import { Divider, ListItem, ListItemText, makeStyles } from '@material-ui/core'; import React from 'react'; import { CodeSnippet } from '../CodeSnippet'; import { CopyTextButton } from '../CopyTextButton'; -import { WarningPanel } from '../WarningPanel'; +import { ErrorPanel, ErrorPanelProps } from '../ErrorPanel'; const useStyles = makeStyles(theme => ({ text: { @@ -39,107 +33,25 @@ const useStyles = makeStyles(theme => ({ }, })); -type ResponseErrorListProps = { - error: string; - message: string; - request?: string; - stack?: string; - json?: string; - actions?: React.ReactNode; -}; - -const ResponseErrorList = ({ - error, - request, - message, - stack, - json, - actions, -}: ResponseErrorListProps) => { - const classes = useStyles(); - - return ( - - - - - - - - - - {request && ( - - - - - )} - {stack && ( - - - - - )} - {json && ( - <> - - - } - /> - - - - )} - {actions && ( - <> - - {actions} - - )} - - ); -}; - -type Props = { - error: Error; - defaultExpanded?: boolean; - title?: string; - actions?: React.ReactNode; -}; - /** - * Renders details about a failed server request. + * Renders a warning panel as the effect of a failed server request. * * Has special treatment for ResponseError errors, to display rich * server-provided information about what happened. */ -export const ResponseErrorDetails = ({ error, actions }: Props) => { +export const ResponseErrorPanel = ({ + title, + error, + defaultExpanded, +}: ErrorPanelProps) => { + const classes = useStyles(); + if (error.name !== 'ResponseError') { return ( - ); } @@ -154,34 +66,32 @@ export const ResponseErrorDetails = ({ error, actions }: Props) => { const jsonString = JSON.stringify(data, undefined, 2); return ( - - ); -}; - -/** - * Renders a warning panel as the effect of a failed server request. - * - * Has special treatment for ResponseError errors, to display rich - * server-provided information about what happened. - */ -export const ResponseErrorPanel = ({ - title, - error, - defaultExpanded, - actions, -}: Props) => { - return ( - - - + {requestString && ( + + + + + )} + <> + + + } + /> + + + + ); }; diff --git a/packages/core/src/components/ResponseErrorPanel/index.ts b/packages/core/src/components/ResponseErrorPanel/index.ts index ac62553fd5..1fc6221a44 100644 --- a/packages/core/src/components/ResponseErrorPanel/index.ts +++ b/packages/core/src/components/ResponseErrorPanel/index.ts @@ -14,4 +14,4 @@ * limitations under the License. */ -export { ResponseErrorDetails, ResponseErrorPanel } from './ResponseErrorPanel'; +export { ResponseErrorPanel } from './ResponseErrorPanel'; From f9a4fd82495938a2c6b44c0ffff0f78bc677820b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Mon, 7 Jun 2021 14:38:33 +0200 Subject: [PATCH 4/4] Move everything out of the old core libs, and fix up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- .changeset/few-cats-type.md | 15 +- packages/app/src/App.tsx | 4 - packages/core-api/src/app/App.test.tsx | 1 - packages/core-api/src/app/types.ts | 12 +- .../src/extensions/extensions.test.tsx | 49 ------ .../core-api/src/extensions/extensions.tsx | 28 +--- packages/core-api/src/routing/hooks.test.tsx | 148 ++++++++--------- packages/core-app-api/api-report.md | 8 + packages/core-app-api/src/app/App.test.tsx | 1 + packages/core-app-api/src/app/createApp.tsx | 40 ++++- packages/core-app-api/src/app/types.ts | 7 + .../src/routing/RoutingProvider.test.tsx | 107 ++++++------ .../src/components/ErrorPanel/ErrorPanel.tsx | 0 .../src/components/ErrorPanel/index.ts | 0 .../ResponseErrorPanel/ResponseErrorPanel.tsx | 152 +++++------------- .../components/ResponseErrorPanel/index.ts | 2 +- .../components/WarningPanel/WarningPanel.tsx | 10 +- .../core-components/src/components/index.ts | 1 + packages/core-plugin-api/api-report.md | 8 + packages/core-plugin-api/src/app/types.ts | 7 + .../src/extensions/PluginErrorBoundary.tsx | 30 ++-- .../src/extensions/extensions.test.tsx | 34 ++++ .../src/extensions/extensions.tsx | 18 ++- packages/core/src/api-wrappers/createApp.tsx | 2 - .../ErrorBoundaryFallback.tsx | 38 ----- .../components/ErrorBoundaryFallback/index.ts | 17 -- .../ResponseErrorPanel/ResponseErrorPanel.tsx | 152 +++++++++++++----- .../components/ResponseErrorPanel/index.ts | 2 +- .../components/WarningPanel/WarningPanel.tsx | 5 +- packages/core/src/components/index.ts | 1 - .../test-utils/src/testUtils/appWrappers.tsx | 1 - 31 files changed, 432 insertions(+), 468 deletions(-) rename packages/{core => core-components}/src/components/ErrorPanel/ErrorPanel.tsx (100%) rename packages/{core => core-components}/src/components/ErrorPanel/index.ts (100%) rename packages/{core-api => core-plugin-api}/src/extensions/PluginErrorBoundary.tsx (67%) delete mode 100644 packages/core/src/components/ErrorBoundaryFallback/ErrorBoundaryFallback.tsx delete mode 100644 packages/core/src/components/ErrorBoundaryFallback/index.ts diff --git a/.changeset/few-cats-type.md b/.changeset/few-cats-type.md index 7d4c0f69cb..45a59d5ada 100644 --- a/.changeset/few-cats-type.md +++ b/.changeset/few-cats-type.md @@ -1,11 +1,12 @@ --- -'@backstage/core': minor -'@backstage/core-api': patch +'@backstage/core-app-api': patch +'@backstage/core-components': patch +'@backstage/core-plugin-api': patch --- -This change adds error boundary on extensions. +This change adds automatic error boundaries around extensions. -This means all exposed parts of a plugin are wrapped in a general error boundary component, that is plugin aware. The default design for the error box is borrowed from @backstage/errors. To override the default "fallback" one must provide a component named `ErrorBoundaryFallback` to `createApp`, like so: +This means that all exposed parts of a plugin are wrapped in a general error boundary component, that is plugin aware. The default design for the error box is borrowed from `@backstage/errors`. To override the default "fallback", one must provide a component named `ErrorBoundaryFallback` to `createApp`, like so: ```ts const app = createApp({ @@ -28,6 +29,6 @@ const app = createApp({ The props here include: -- `error`. An Error object or something that inherits it that represents the error that was thrown from any inner component. -- `resetError`. A callback that will simply mount the children of the error boundary again. -- `plugin`. A BackstagePlugin that can be used to lookup info to be presented in the error message. For instance, you may want to keep a map of your internal plugins and team names or slack channels and present these when an error occurs. Typically, you'll do that by getting the plugin id with `plugin.getId()`. +- `error`. An `Error` object or something that inherits it that represents the error that was thrown from any inner component. +- `resetError`. A callback that will simply attempt to mount the children of the error boundary again. +- `plugin`. A `BackstagePlugin` that can be used to look up info to be presented in the error message. For instance, you may want to keep a map of your internal plugins and team names or slack channels and present these when an error occurs. Typically, you'll do that by getting the plugin ID with `plugin.getId()`. diff --git a/packages/app/src/App.tsx b/packages/app/src/App.tsx index 927e9b4327..5874465ef6 100644 --- a/packages/app/src/App.tsx +++ b/packages/app/src/App.tsx @@ -20,7 +20,6 @@ import { FlatRoutes, OAuthRequestDialog, SignInPage, - ErrorBoundaryFallback, } from '@backstage/core'; import { apiDocsPlugin, ApiExplorerPage } from '@backstage/plugin-api-docs'; import { @@ -67,9 +66,6 @@ const app = createApp({ alert: AlarmIcon, }, components: { - ErrorBoundaryFallback: props => { - return ; - }, SignInPage: props => { return ( { BootErrorPage: () => null, Progress: () => null, Router: BrowserRouter, - ErrorBoundaryFallback: () => null, }; it('runs happy paths', async () => { diff --git a/packages/core-api/src/app/types.ts b/packages/core-api/src/app/types.ts index 333eb06afd..df0f65f33f 100644 --- a/packages/core-api/src/app/types.ts +++ b/packages/core-api/src/app/types.ts @@ -14,14 +14,13 @@ * limitations under the License. */ -import { AppConfig } from '@backstage/config'; import { ComponentType } from 'react'; -import { AppTheme, ProfileInfo } from '../apis/definitions'; -import { AnyApiFactory } from '../apis/system'; -import type { ErrorBoundaryFallbackProps } from '../extensions/PluginErrorBoundary'; import { IconComponent, IconComponentMap, IconKey } from '../icons/types'; import { AnyExternalRoutes, BackstagePlugin } from '../plugin/types'; import { ExternalRouteRef, RouteRef, SubRouteRef } from '../routing/types'; +import { AnyApiFactory } from '../apis/system'; +import { AppTheme, ProfileInfo } from '../apis/definitions'; +import { AppConfig } from '@backstage/config'; export type BootErrorPageProps = { step: 'load-config' | 'load-chunk'; @@ -59,11 +58,6 @@ export type AppComponents = { BootErrorPage: ComponentType; Progress: ComponentType<{}>; Router: ComponentType<{}>; - ErrorBoundaryFallback: ComponentType< - ErrorBoundaryFallbackProps & { - plugin?: BackstagePlugin; - } - >; /** * An optional sign-in page that will be rendered instead of the AppRouter at startup. diff --git a/packages/core-api/src/extensions/extensions.test.tsx b/packages/core-api/src/extensions/extensions.test.tsx index 773dc32ab2..26755b3bcf 100644 --- a/packages/core-api/src/extensions/extensions.test.tsx +++ b/packages/core-api/src/extensions/extensions.test.tsx @@ -14,13 +14,7 @@ * limitations under the License. */ -import { errorApiRef } from '../apis/definitions/ErrorApi'; -import { ApiRegistry } from '../apis/system/ApiRegistry'; -import { ApiProvider } from '../apis/system/ApiProvider'; -import { withLogCollector } from '@backstage/test-utils-core'; -import { render, screen } from '@testing-library/react'; import React from 'react'; -import { AppContextProvider } from '../app/AppContext'; import { createPlugin } from '../plugin'; import { createRouteRef } from '../routing'; import { getComponentData } from './componentData'; @@ -29,9 +23,6 @@ import { createReactExtension, createRoutableExtension, } from './extensions'; -import { AppComponents } from '../app/types'; -import { AppContext } from '../app'; -import { MockErrorApi } from '@backstage/test-utils'; const plugin = createPlugin({ id: 'my-plugin', @@ -82,44 +73,4 @@ describe('extensions', () => { expect(getComponentData(element2, 'core.plugin')).toBe(plugin); expect(getComponentData(element2, 'core.mountPoint')).toBe(routeRef); }); - - it('should wrap extended component with error boundary', async () => { - const extension = createComponentExtension({ - component: { - sync: () => { - throw new Error('Test error'); - }, - }, - }); - - const BrokenComponent = plugin.provide(extension); - - const errorApi = new MockErrorApi(); - - const apis = ApiRegistry.from([[errorApiRef, errorApi]]); - - const MockFallback: AppComponents['ErrorBoundaryFallback'] = props => ( - <>Error in {props.plugin?.getId()} - ); - - const { error: errors } = await withLogCollector(['error'], async () => { - render( - - ({ - ErrorBoundaryFallback: MockFallback, - }), - } as unknown) as AppContext - } - > - - - , - ); - }); - screen.getByText('Error in my-plugin'); - expect(errors[0]).toMatch('Test error'); - }); }); diff --git a/packages/core-api/src/extensions/extensions.tsx b/packages/core-api/src/extensions/extensions.tsx index 686c467caf..9f6412f960 100644 --- a/packages/core-api/src/extensions/extensions.tsx +++ b/packages/core-api/src/extensions/extensions.tsx @@ -19,7 +19,6 @@ import { useApp } from '../app'; import { BackstagePlugin, Extension } from '../plugin/types'; import { RouteRef, useRouteRef } from '../routing'; import { attachComponentData } from './componentData'; -import { PluginErrorBoundary } from './PluginErrorBoundary'; type ComponentLoader = | { @@ -124,28 +123,11 @@ export function createReactExtension< return { expose(plugin: BackstagePlugin) { - const Result: any = (props: any) => { - const app = useApp(); - const { ErrorBoundaryFallback } = app.getComponents(); - - return ( - - { - return ( - - ); - }} - > - - - - ); - }; + const Result: any = (props: any) => ( + + + + ); attachComponentData(Result, 'core.plugin', plugin); for (const [key, value] of Object.entries(data)) { diff --git a/packages/core-api/src/routing/hooks.test.tsx b/packages/core-api/src/routing/hooks.test.tsx index a8fdfda5d0..8a9b70ed7d 100644 --- a/packages/core-api/src/routing/hooks.test.tsx +++ b/packages/core-api/src/routing/hooks.test.tsx @@ -14,7 +14,6 @@ * limitations under the License. */ -import { AppContext } from '../app'; import { render } from '@testing-library/react'; import { renderHook } from '@testing-library/react-hooks'; import React, { @@ -24,7 +23,6 @@ import React, { useContext, } from 'react'; import { MemoryRouter, Route, Routes } from 'react-router-dom'; -import { AppContextProvider } from '../app/AppContext'; import { createRoutableExtension } from '../extensions'; import { childDiscoverer, @@ -156,27 +154,23 @@ function withRoutingProvider( ); } -const appContext = ({ getComponents: () => ({}) } as unknown) as AppContext; - describe('discovery', () => { it('should handle simple routeRef path creation for routeRefs used in other parts of the app', async () => { const root = ( - - - - - - - - - - - - - - - - + + + + + + + + + + + + + + ); const rendered = render( @@ -211,25 +205,23 @@ describe('discovery', () => { it('should handle routeRefs with parameters', async () => { const root = ( - - - - - - - - - - + + + + + + + + ); const rendered = render(withRoutingProvider(root)); @@ -244,37 +236,8 @@ describe('discovery', () => { it('should handle relative routing within parameterized routePaths', async () => { const root = ( - - - - - - - - - - - - - - - ); - - const rendered = render(withRoutingProvider(root)); - - await expect( - rendered.findByText('Path at inside: /foo/blob/baz'), - ).resolves.toBeInTheDocument(); - }); - - it('should throw errors for routing to other routeRefs with unsupported parameters', () => { - const root = ( - - + + @@ -287,8 +250,33 @@ describe('discovery', () => { routeRef={ref3} params={{ id: 'blob' }} /> - - + + + ); + + const rendered = render(withRoutingProvider(root)); + + await expect( + rendered.findByText('Path at inside: /foo/blob/baz'), + ).resolves.toBeInTheDocument(); + }); + + it('should throw errors for routing to other routeRefs with unsupported parameters', () => { + const root = ( + + + + + + + + + + ); const rendered = render(withRoutingProvider(root)); @@ -307,15 +295,13 @@ describe('discovery', () => { it('should handle relative routing of parameterized routePaths with duplicate param names', () => { const root = ( - - - - - - - - - + + + + + + + ); const { routePaths, routeParents } = traverseElementTree({ diff --git a/packages/core-app-api/api-report.md b/packages/core-app-api/api-report.md index 36c4f7419b..c64f9db999 100644 --- a/packages/core-app-api/api-report.md +++ b/packages/core-app-api/api-report.md @@ -120,6 +120,7 @@ export type AppComponents = { BootErrorPage: ComponentType; Progress: ComponentType<{}>; Router: ComponentType<{}>; + ErrorBoundaryFallback: ComponentType; SignInPage?: ComponentType; }; @@ -220,6 +221,13 @@ export class ErrorApiForwarder implements ErrorApi { post(error: Error, context?: ErrorContext): void; } +// @public (undocumented) +export type ErrorBoundaryFallbackProps = { + plugin?: BackstagePlugin; + error: Error; + resetError: () => void; +}; + // @public (undocumented) export const FlatRoutes: (props: FlatRoutesProps) => JSX.Element | null; diff --git a/packages/core-app-api/src/app/App.test.tsx b/packages/core-app-api/src/app/App.test.tsx index cde3af9456..0539c2146c 100644 --- a/packages/core-app-api/src/app/App.test.tsx +++ b/packages/core-app-api/src/app/App.test.tsx @@ -159,6 +159,7 @@ describe('Integration Test', () => { BootErrorPage: () => null, Progress: () => null, Router: BrowserRouter, + ErrorBoundaryFallback: () => null, }; it('runs happy paths', async () => { diff --git a/packages/core-app-api/src/app/createApp.tsx b/packages/core-app-api/src/app/createApp.tsx index c11ac840f9..b93df63ff0 100644 --- a/packages/core-app-api/src/app/createApp.tsx +++ b/packages/core-app-api/src/app/createApp.tsx @@ -14,21 +14,27 @@ * limitations under the License. */ +import { AppConfig, JsonObject } from '@backstage/config'; +import { Button } from '@material-ui/core'; +import { ErrorPage, ErrorPanel, Progress } from '@backstage/core-components'; +import { darkTheme, lightTheme } from '@backstage/theme'; +import DarkIcon from '@material-ui/icons/Brightness2'; +import LightIcon from '@material-ui/icons/WbSunny'; import React, { PropsWithChildren } from 'react'; -import { AppOptions, BootErrorPageProps, AppConfigLoader } from './types'; -import { defaultAppIcons } from './icons'; import { BrowserRouter, MemoryRouter, useInRouterContext, } from 'react-router-dom'; -import LightIcon from '@material-ui/icons/WbSunny'; -import DarkIcon from '@material-ui/icons/Brightness2'; -import { ErrorPage, Progress } from '@backstage/core-components'; -import { defaultApis } from './defaultApis'; -import { lightTheme, darkTheme } from '@backstage/theme'; -import { AppConfig, JsonObject } from '@backstage/config'; import { PrivateAppImpl } from './App'; +import { defaultApis } from './defaultApis'; +import { defaultAppIcons } from './icons'; +import { + AppConfigLoader, + AppOptions, + BootErrorPageProps, + ErrorBoundaryFallbackProps, +} from './types'; /** * The default config loader, which expects that config is available at compile-time @@ -115,6 +121,23 @@ export function createApp(options?: AppOptions) { ); }; + const DefaultErrorBoundaryFallback = ({ + error, + resetError, + plugin, + }: ErrorBoundaryFallbackProps) => { + return ( + + + + ); + }; const apis = options?.apis ?? []; const icons = { ...defaultAppIcons, ...options?.icons }; @@ -124,6 +147,7 @@ export function createApp(options?: AppOptions) { BootErrorPage: DefaultBootErrorPage, Progress: Progress, Router: BrowserRouter, + ErrorBoundaryFallback: DefaultErrorBoundaryFallback, ...options?.components, }; const themes = options?.themes ?? [ diff --git a/packages/core-app-api/src/app/types.ts b/packages/core-app-api/src/app/types.ts index 85be0c5f53..b7a6754232 100644 --- a/packages/core-app-api/src/app/types.ts +++ b/packages/core-app-api/src/app/types.ts @@ -59,11 +59,18 @@ export type SignInPageProps = { onResult(result: SignInResult): void; }; +export type ErrorBoundaryFallbackProps = { + plugin?: BackstagePlugin; + error: Error; + resetError: () => void; +}; + export type AppComponents = { NotFoundErrorPage: ComponentType<{}>; BootErrorPage: ComponentType; Progress: ComponentType<{}>; Router: ComponentType<{}>; + ErrorBoundaryFallback: ComponentType; /** * An optional sign-in page that will be rendered instead of the AppRouter at startup. diff --git a/packages/core-app-api/src/routing/RoutingProvider.test.tsx b/packages/core-app-api/src/routing/RoutingProvider.test.tsx index 8f8ffc820b..b75e68e7f0 100644 --- a/packages/core-app-api/src/routing/RoutingProvider.test.tsx +++ b/packages/core-app-api/src/routing/RoutingProvider.test.tsx @@ -48,6 +48,7 @@ import { import { validateRoutes } from './validation'; import { RouteResolver } from './RouteResolver'; import { AnyRouteRef, RouteFunc } from './types'; +import { AppContextProvider } from '../app/AppContext'; const MockComponent = ({ children }: PropsWithChildren<{ path?: string }>) => ( <>{children} @@ -125,6 +126,12 @@ const Extension5 = plugin.provide( }), ); +const mockContext = { + getComponents: () => ({ Progress: () => null } as any), + getSystemIcon: jest.fn(), + getPlugins: jest.fn(), +}; + function withRoutingProvider( root: ReactElement, routeBindings: [ExternalRouteRef, RouteRef][] = [], @@ -154,20 +161,22 @@ function withRoutingProvider( describe('discovery', () => { it('should handle simple routeRef path creation for routeRefs used in other parts of the app', async () => { const root = ( - - - - - - - - - - - - - - + + + + + + + + + + + + + + + + ); const rendered = render( @@ -202,23 +211,25 @@ describe('discovery', () => { it('should handle routeRefs with parameters', async () => { const root = ( - - - - - - - - + + + + + + + + + + ); const rendered = render(withRoutingProvider(root)); @@ -233,22 +244,24 @@ describe('discovery', () => { it('should handle relative routing within parameterized routePaths', async () => { const root = ( - - - - - - - - - - - - + + + + + + + + + + + + + + ); const rendered = render(withRoutingProvider(root)); diff --git a/packages/core/src/components/ErrorPanel/ErrorPanel.tsx b/packages/core-components/src/components/ErrorPanel/ErrorPanel.tsx similarity index 100% rename from packages/core/src/components/ErrorPanel/ErrorPanel.tsx rename to packages/core-components/src/components/ErrorPanel/ErrorPanel.tsx diff --git a/packages/core/src/components/ErrorPanel/index.ts b/packages/core-components/src/components/ErrorPanel/index.ts similarity index 100% rename from packages/core/src/components/ErrorPanel/index.ts rename to packages/core-components/src/components/ErrorPanel/index.ts diff --git a/packages/core-components/src/components/ResponseErrorPanel/ResponseErrorPanel.tsx b/packages/core-components/src/components/ResponseErrorPanel/ResponseErrorPanel.tsx index 52f7ab03f9..33da6261ec 100644 --- a/packages/core-components/src/components/ResponseErrorPanel/ResponseErrorPanel.tsx +++ b/packages/core-components/src/components/ResponseErrorPanel/ResponseErrorPanel.tsx @@ -15,17 +15,11 @@ */ import { ResponseError } from '@backstage/errors'; -import { - Divider, - List, - ListItem, - ListItemText, - makeStyles, -} from '@material-ui/core'; +import { Divider, ListItem, ListItemText, makeStyles } from '@material-ui/core'; import React from 'react'; import { CodeSnippet } from '../CodeSnippet'; import { CopyTextButton } from '../CopyTextButton'; -import { WarningPanel } from '../WarningPanel'; +import { ErrorPanel, ErrorPanelProps } from '../ErrorPanel'; const useStyles = makeStyles(theme => ({ text: { @@ -39,95 +33,25 @@ const useStyles = makeStyles(theme => ({ }, })); -type ResponseErrorListProps = { - error: string; - message: string; - request?: string; - stack?: string; - json?: string; -}; - -const ResponseErrorList = ({ - error, - request, - message, - stack, - json, -}: ResponseErrorListProps) => { - const classes = useStyles(); - - return ( - - - - - - - - - - {request && ( - - - - - )} - {stack && ( - - - - - )} - {json && ( - <> - - - } - /> - - - - )} - - ); -}; - -type Props = { - error: Error; -}; - /** - * Renders details about a failed server request. + * Renders a warning panel as the effect of a failed server request. * * Has special treatment for ResponseError errors, to display rich * server-provided information about what happened. */ -export const ResponseErrorDetails = ({ error }: Props) => { +export const ResponseErrorPanel = ({ + title, + error, + defaultExpanded, +}: ErrorPanelProps) => { + const classes = useStyles(); + if (error.name !== 'ResponseError') { return ( - ); } @@ -142,26 +66,32 @@ export const ResponseErrorDetails = ({ error }: Props) => { const jsonString = JSON.stringify(data, undefined, 2); return ( - - ); -}; - -/** - * Renders a warning panel as the effect of a failed server request. - * - * Has special treatment for ResponseError errors, to display rich - * server-provided information about what happened. - */ -export const ResponseErrorPanel = ({ error }: Props) => { - return ( - - - + + {requestString && ( + + + + + )} + <> + + + } + /> + + + + ); }; diff --git a/packages/core-components/src/components/ResponseErrorPanel/index.ts b/packages/core-components/src/components/ResponseErrorPanel/index.ts index ac62553fd5..1fc6221a44 100644 --- a/packages/core-components/src/components/ResponseErrorPanel/index.ts +++ b/packages/core-components/src/components/ResponseErrorPanel/index.ts @@ -14,4 +14,4 @@ * limitations under the License. */ -export { ResponseErrorDetails, ResponseErrorPanel } from './ResponseErrorPanel'; +export { ResponseErrorPanel } from './ResponseErrorPanel'; diff --git a/packages/core-components/src/components/WarningPanel/WarningPanel.tsx b/packages/core-components/src/components/WarningPanel/WarningPanel.tsx index 1faf3bb142..3b280dea9d 100644 --- a/packages/core-components/src/components/WarningPanel/WarningPanel.tsx +++ b/packages/core-components/src/components/WarningPanel/WarningPanel.tsx @@ -77,6 +77,7 @@ type Props = { title?: string; severity?: 'warning' | 'error' | 'info'; message?: React.ReactNode; + defaultExpanded?: boolean; children?: React.ReactNode; }; @@ -92,18 +93,21 @@ const capitalize = (s: string) => { * @param {string} [title] A title for the warning. If not supplied, "Warning" will be used. * @param {Object} [message] Optional more detailed user-friendly message elaborating on the cause of the error. * @param {Object} [children] Objects to provide context, such as a stack trace or detailed error reporting. - * Will be available inside an unfolded accordion. + * Will be available inside an unfolded accordion. */ export const WarningPanel = (props: Props) => { const classes = useStyles(props); - const { severity, title, message, children } = props; + const { severity, title, message, children, defaultExpanded } = props; // If no severity or title provided, the heading will read simply "Warning" const subTitle = (severity ? capitalize(severity) : 'Warning') + (title ? `: ${title}` : ''); return ( - + } className={classes.summary} diff --git a/packages/core-components/src/components/index.ts b/packages/core-components/src/components/index.ts index c8797f576b..28b6f6637a 100644 --- a/packages/core-components/src/components/index.ts +++ b/packages/core-components/src/components/index.ts @@ -22,6 +22,7 @@ export * from './CopyTextButton'; export * from './DependencyGraph'; export * from './DismissableBanner'; export * from './EmptyState'; +export * from './ErrorPanel'; export * from './ResponseErrorPanel'; export * from './FeatureDiscovery'; export * from './HeaderIconLinkRow'; diff --git a/packages/core-plugin-api/api-report.md b/packages/core-plugin-api/api-report.md index 686d096b7f..a6f196847a 100644 --- a/packages/core-plugin-api/api-report.md +++ b/packages/core-plugin-api/api-report.md @@ -70,6 +70,7 @@ export type AppComponents = { BootErrorPage: ComponentType; Progress: ComponentType<{}>; Router: ComponentType<{}>; + ErrorBoundaryFallback: ComponentType; SignInPage?: ComponentType; }; @@ -238,6 +239,13 @@ export type ErrorApi = { // @public (undocumented) export const errorApiRef: ApiRef; +// @public (undocumented) +export type ErrorBoundaryFallbackProps = { + plugin?: BackstagePlugin; + error: Error; + resetError: () => void; +}; + // @public export type ErrorContext = { hidden?: boolean; diff --git a/packages/core-plugin-api/src/app/types.ts b/packages/core-plugin-api/src/app/types.ts index 66796fd5a4..26e264983c 100644 --- a/packages/core-plugin-api/src/app/types.ts +++ b/packages/core-plugin-api/src/app/types.ts @@ -50,11 +50,18 @@ export type SignInPageProps = { onResult(result: SignInResult): void; }; +export type ErrorBoundaryFallbackProps = { + plugin?: BackstagePlugin; + error: Error; + resetError: () => void; +}; + export type AppComponents = { NotFoundErrorPage: ComponentType<{}>; BootErrorPage: ComponentType; Progress: ComponentType<{}>; Router: ComponentType<{}>; + ErrorBoundaryFallback: ComponentType; /** * An optional sign-in page that will be rendered instead of the AppRouter at startup. diff --git a/packages/core-api/src/extensions/PluginErrorBoundary.tsx b/packages/core-plugin-api/src/extensions/PluginErrorBoundary.tsx similarity index 67% rename from packages/core-api/src/extensions/PluginErrorBoundary.tsx rename to packages/core-plugin-api/src/extensions/PluginErrorBoundary.tsx index 4cf772a638..e33d9dcb43 100644 --- a/packages/core-api/src/extensions/PluginErrorBoundary.tsx +++ b/packages/core-plugin-api/src/extensions/PluginErrorBoundary.tsx @@ -15,19 +15,15 @@ */ import React from 'react'; - -export type ErrorBoundaryFallbackProps = { - error: Error; - resetError: () => void; -}; - -type FallbackRender = React.FunctionComponent; +import { AppContext } from '../app/types'; +import { BackstagePlugin } from '../plugin'; type Props = { - fallbackRender: FallbackRender; + app: AppContext; + plugin: BackstagePlugin; }; -type State = { error?: Error }; +type State = { error: Error | undefined }; export class PluginErrorBoundary extends React.Component { static getDerivedStateFromError(error: Error) { @@ -36,19 +32,23 @@ export class PluginErrorBoundary extends React.Component { state: State = { error: undefined }; - resetError = () => { + handleErrorReset = () => { this.setState({ error: undefined }); }; render() { const { error } = this.state; - const { fallbackRender } = this.props; + const { app, plugin } = this.props; + const { ErrorBoundaryFallback } = app.getComponents(); if (error) { - return fallbackRender({ - error, - resetError: this.resetError, - }); + return ( + + ); } return this.props.children; diff --git a/packages/core-plugin-api/src/extensions/extensions.test.tsx b/packages/core-plugin-api/src/extensions/extensions.test.tsx index 29592574ec..798d3c8fc2 100644 --- a/packages/core-plugin-api/src/extensions/extensions.test.tsx +++ b/packages/core-plugin-api/src/extensions/extensions.test.tsx @@ -14,7 +14,10 @@ * limitations under the License. */ +import { withLogCollector } from '@backstage/test-utils-core'; +import { render, screen } from '@testing-library/react'; import React from 'react'; +import { useApp, ErrorBoundaryFallbackProps } from '../app'; import { createPlugin } from '../plugin'; import { createRouteRef } from '../routing'; import { getComponentData } from './componentData'; @@ -24,6 +27,10 @@ import { createRoutableExtension, } from './extensions'; +jest.mock('../app'); + +const mocked = (f: Function) => f as jest.Mock; + const plugin = createPlugin({ id: 'my-plugin', }); @@ -73,4 +80,31 @@ describe('extensions', () => { expect(getComponentData(element2, 'core.plugin')).toBe(plugin); expect(getComponentData(element2, 'core.mountPoint')).toBe(routeRef); }); + + it('should wrap extended component with error boundary', async () => { + const BrokenComponent = plugin.provide( + createComponentExtension({ + component: { + sync: () => { + throw new Error('Test error'); + }, + }, + }), + ); + + mocked(useApp).mockReturnValue({ + getComponents: () => ({ + Progress: () => null, + ErrorBoundaryFallback: (props: ErrorBoundaryFallbackProps) => ( + <>Error in {props.plugin?.getId()} + ), + }), + }); + + const { error: errors } = await withLogCollector(['error'], async () => { + render(); + }); + screen.getByText('Error in my-plugin'); + expect(errors[0]).toMatch('Test error'); + }); }); diff --git a/packages/core-plugin-api/src/extensions/extensions.tsx b/packages/core-plugin-api/src/extensions/extensions.tsx index d6597ba8a9..349f44726f 100644 --- a/packages/core-plugin-api/src/extensions/extensions.tsx +++ b/packages/core-plugin-api/src/extensions/extensions.tsx @@ -19,6 +19,7 @@ import { useApp } from '../app'; import { RouteRef, useRouteRef } from '../routing'; import { attachComponentData } from './componentData'; import { Extension, BackstagePlugin } from '../plugin/types'; +import { PluginErrorBoundary } from './PluginErrorBoundary'; type ComponentLoader = | { @@ -123,11 +124,18 @@ export function createReactExtension< return { expose(plugin: BackstagePlugin) { - const Result: any = (props: any) => ( - - - - ); + const Result: any = (props: any) => { + const app = useApp(); + const { Progress } = app.getComponents(); + + return ( + }> + + + + + ); + }; attachComponentData(Result, 'core.plugin', plugin); for (const [key, value] of Object.entries(data)) { diff --git a/packages/core/src/api-wrappers/createApp.tsx b/packages/core/src/api-wrappers/createApp.tsx index 9b8524c89c..e79cdf509f 100644 --- a/packages/core/src/api-wrappers/createApp.tsx +++ b/packages/core/src/api-wrappers/createApp.tsx @@ -30,7 +30,6 @@ import LightIcon from '@material-ui/icons/WbSunny'; import DarkIcon from '@material-ui/icons/Brightness2'; import { ErrorPage } from '../layout/ErrorPage'; import { Progress } from '../components/Progress'; -import { ErrorBoundaryFallback } from '../components/ErrorBoundaryFallback'; import { defaultApis } from './defaultApis'; import { lightTheme, darkTheme } from '@backstage/theme'; import { AppConfig, JsonObject } from '@backstage/config'; @@ -131,7 +130,6 @@ export function createApp(options?: AppOptions) { BootErrorPage: DefaultBootErrorPage, Progress: Progress, Router: BrowserRouter, - ErrorBoundaryFallback: ErrorBoundaryFallback, ...options?.components, }; const themes = options?.themes ?? [ diff --git a/packages/core/src/components/ErrorBoundaryFallback/ErrorBoundaryFallback.tsx b/packages/core/src/components/ErrorBoundaryFallback/ErrorBoundaryFallback.tsx deleted file mode 100644 index 8be43995f1..0000000000 --- a/packages/core/src/components/ErrorBoundaryFallback/ErrorBoundaryFallback.tsx +++ /dev/null @@ -1,38 +0,0 @@ -/* - * Copyright 2020 Spotify AB - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import React from 'react'; -import { AppComponents } from '../..'; -import { ErrorPanel } from '../ErrorPanel'; -import { Button } from '@material-ui/core'; - -export const ErrorBoundaryFallback: AppComponents['ErrorBoundaryFallback'] = ({ - error, - resetError, - plugin, -}) => { - return ( - - - - ); -}; diff --git a/packages/core/src/components/ErrorBoundaryFallback/index.ts b/packages/core/src/components/ErrorBoundaryFallback/index.ts deleted file mode 100644 index ff40974ed7..0000000000 --- a/packages/core/src/components/ErrorBoundaryFallback/index.ts +++ /dev/null @@ -1,17 +0,0 @@ -/* - * Copyright 2020 Spotify AB - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -export { ErrorBoundaryFallback } from './ErrorBoundaryFallback'; diff --git a/packages/core/src/components/ResponseErrorPanel/ResponseErrorPanel.tsx b/packages/core/src/components/ResponseErrorPanel/ResponseErrorPanel.tsx index 33da6261ec..52f7ab03f9 100644 --- a/packages/core/src/components/ResponseErrorPanel/ResponseErrorPanel.tsx +++ b/packages/core/src/components/ResponseErrorPanel/ResponseErrorPanel.tsx @@ -15,11 +15,17 @@ */ import { ResponseError } from '@backstage/errors'; -import { Divider, ListItem, ListItemText, makeStyles } from '@material-ui/core'; +import { + Divider, + List, + ListItem, + ListItemText, + makeStyles, +} from '@material-ui/core'; import React from 'react'; import { CodeSnippet } from '../CodeSnippet'; import { CopyTextButton } from '../CopyTextButton'; -import { ErrorPanel, ErrorPanelProps } from '../ErrorPanel'; +import { WarningPanel } from '../WarningPanel'; const useStyles = makeStyles(theme => ({ text: { @@ -33,25 +39,95 @@ const useStyles = makeStyles(theme => ({ }, })); +type ResponseErrorListProps = { + error: string; + message: string; + request?: string; + stack?: string; + json?: string; +}; + +const ResponseErrorList = ({ + error, + request, + message, + stack, + json, +}: ResponseErrorListProps) => { + const classes = useStyles(); + + return ( + + + + + + + + + + {request && ( + + + + + )} + {stack && ( + + + + + )} + {json && ( + <> + + + } + /> + + + + )} + + ); +}; + +type Props = { + error: Error; +}; + /** - * Renders a warning panel as the effect of a failed server request. + * Renders details about a failed server request. * * Has special treatment for ResponseError errors, to display rich * server-provided information about what happened. */ -export const ResponseErrorPanel = ({ - title, - error, - defaultExpanded, -}: ErrorPanelProps) => { - const classes = useStyles(); - +export const ResponseErrorDetails = ({ error }: Props) => { if (error.name !== 'ResponseError') { return ( - ); } @@ -66,32 +142,26 @@ export const ResponseErrorPanel = ({ const jsonString = JSON.stringify(data, undefined, 2); return ( - - {requestString && ( - - - - - )} - <> - - - } - /> - - - - + + ); +}; + +/** + * Renders a warning panel as the effect of a failed server request. + * + * Has special treatment for ResponseError errors, to display rich + * server-provided information about what happened. + */ +export const ResponseErrorPanel = ({ error }: Props) => { + return ( + + + ); }; diff --git a/packages/core/src/components/ResponseErrorPanel/index.ts b/packages/core/src/components/ResponseErrorPanel/index.ts index 1fc6221a44..ac62553fd5 100644 --- a/packages/core/src/components/ResponseErrorPanel/index.ts +++ b/packages/core/src/components/ResponseErrorPanel/index.ts @@ -14,4 +14,4 @@ * limitations under the License. */ -export { ResponseErrorPanel } from './ResponseErrorPanel'; +export { ResponseErrorDetails, ResponseErrorPanel } from './ResponseErrorPanel'; diff --git a/packages/core/src/components/WarningPanel/WarningPanel.tsx b/packages/core/src/components/WarningPanel/WarningPanel.tsx index 20c71ee106..1faf3bb142 100644 --- a/packages/core/src/components/WarningPanel/WarningPanel.tsx +++ b/packages/core/src/components/WarningPanel/WarningPanel.tsx @@ -77,7 +77,6 @@ type Props = { title?: string; severity?: 'warning' | 'error' | 'info'; message?: React.ReactNode; - defaultExpanded?: boolean; children?: React.ReactNode; }; @@ -97,14 +96,14 @@ const capitalize = (s: string) => { */ export const WarningPanel = (props: Props) => { const classes = useStyles(props); - const { severity, title, message, children, defaultExpanded } = props; + const { severity, title, message, children } = props; // If no severity or title provided, the heading will read simply "Warning" const subTitle = (severity ? capitalize(severity) : 'Warning') + (title ? `: ${title}` : ''); return ( - + } className={classes.summary} diff --git a/packages/core/src/components/index.ts b/packages/core/src/components/index.ts index c8f9803eae..14f7f3cac0 100644 --- a/packages/core/src/components/index.ts +++ b/packages/core/src/components/index.ts @@ -23,7 +23,6 @@ export * from './CopyTextButton'; export * from './DependencyGraph'; export * from './DismissableBanner'; export * from './EmptyState'; -export * from './ErrorBoundaryFallback'; export * from './ResponseErrorPanel'; export * from './FeatureDiscovery'; export * from './HeaderIconLinkRow'; diff --git a/packages/test-utils/src/testUtils/appWrappers.tsx b/packages/test-utils/src/testUtils/appWrappers.tsx index 0086bf3293..add7b4479c 100644 --- a/packages/test-utils/src/testUtils/appWrappers.tsx +++ b/packages/test-utils/src/testUtils/appWrappers.tsx @@ -96,7 +96,6 @@ export function wrapInTestApp( Router: ({ children }) => ( ), - ErrorBoundaryFallback: () => null, }, icons: defaultSystemIcons, plugins: [],