diff --git a/docs/getting-started/utility-apis.md b/docs/getting-started/utility-apis.md index 09dc27ce7a..dba48355c7 100644 --- a/docs/getting-started/utility-apis.md +++ b/docs/getting-started/utility-apis.md @@ -55,15 +55,16 @@ import { errorApiRef, AlertApiForwarder, ErrorApiForwarder, + ErrorAlerter, } from '@backstage/core'; const builder = ApiRegistry.builder(); // The alert API is a self-contained implementation that shows alerts to the user. -builder.add(alertApiRef, new AlertApiForwarder()); +const alertApi = builder.add(alertApiRef, new AlertApiForwarder()); // The error API uses the alert API to send error notifications to the user. -builder.add(errorApiRef, new ErrorApiForwarder(alertApiForwarder)); +builder.add(errorApiRef, new ErrorAlerter(alertApi, new ErrorApiForwarder())); const app = createApp({ apis: apiBuilder.build(), diff --git a/packages/app/package.json b/packages/app/package.json index 0a3d68603b..a366dcce1a 100644 --- a/packages/app/package.json +++ b/packages/app/package.json @@ -18,7 +18,6 @@ "@backstage/plugin-sentry": "^0.1.1-alpha.6", "@material-ui/core": "^4.9.1", "@material-ui/icons": "^4.9.1", - "@material-ui/lab": "4.0.0-alpha.45", "prop-types": "^15.7.2", "react": "^16.12.0", "react-dom": "^16.12.0", diff --git a/packages/app/src/App.tsx b/packages/app/src/App.tsx index e29fac8865..b4d01e067b 100644 --- a/packages/app/src/App.tsx +++ b/packages/app/src/App.tsx @@ -14,13 +14,12 @@ * limitations under the License. */ -import { createApp, OAuthRequestDialog } from '@backstage/core'; +import { createApp, AlertDisplay, OAuthRequestDialog } from '@backstage/core'; import React, { FC } from 'react'; import { BrowserRouter as Router } from 'react-router-dom'; import Root from './components/Root'; -import AlertDisplay from './components/AlertDisplay'; import * as plugins from './plugins'; -import apis, { alertApiForwarder } from './apis'; +import apis from './apis'; const app = createApp({ apis, @@ -32,7 +31,7 @@ const AppComponent = app.getRootComponent(); const App: FC<{}> = () => ( - + diff --git a/packages/app/src/apis.ts b/packages/app/src/apis.ts index c11ea24aca..8d76c75d91 100644 --- a/packages/app/src/apis.ts +++ b/packages/app/src/apis.ts @@ -21,6 +21,7 @@ import { errorApiRef, AlertApiForwarder, ErrorApiForwarder, + ErrorAlerter, featureFlagsApiRef, FeatureFlags, GoogleAuth, @@ -40,11 +41,9 @@ import { CircleCIApi, circleCIApiRef } from '@backstage/plugin-circleci'; const builder = ApiRegistry.builder(); -export const alertApiForwarder = new AlertApiForwarder(); -builder.add(alertApiRef, alertApiForwarder); +const alertApi = builder.add(alertApiRef, new AlertApiForwarder()); -export const errorApiForwarder = new ErrorApiForwarder(alertApiForwarder); -builder.add(errorApiRef, errorApiForwarder); +builder.add(errorApiRef, new ErrorAlerter(alertApi, new ErrorApiForwarder())); builder.add(circleCIApiRef, new CircleCIApi()); builder.add(featureFlagsApiRef, new FeatureFlags()); diff --git a/packages/core/package.json b/packages/core/package.json index e4521530b1..b1800f5db3 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -31,6 +31,7 @@ "@backstage/theme": "^0.1.1-alpha.6", "@material-ui/core": "^4.9.1", "@material-ui/icons": "^4.9.1", + "@material-ui/lab": "4.0.0-alpha.45", "@types/classnames": "^2.2.9", "@types/google-protobuf": "^3.7.2", "@types/jest": "^25.2.2", diff --git a/packages/core/src/api/apis/definitions/AlertApi.ts b/packages/core/src/api/apis/definitions/AlertApi.ts index 9776a96d84..123cd47651 100644 --- a/packages/core/src/api/apis/definitions/AlertApi.ts +++ b/packages/core/src/api/apis/definitions/AlertApi.ts @@ -14,6 +14,7 @@ * limitations under the License. */ import { createApiRef } from '../ApiRef'; +import { Observable } from '../../types'; export type AlertMessage = { message: string; @@ -30,6 +31,11 @@ export type AlertApi = { * Post an alert for handling by the application. */ post(alert: AlertMessage): void; + + /** + * Observe alerts posted by other parts of the application. + */ + alert$(): Observable; }; export const alertApiRef = createApiRef({ diff --git a/packages/core/src/api/apis/definitions/ErrorApi.ts b/packages/core/src/api/apis/definitions/ErrorApi.ts index c481547459..7f9676ea5a 100644 --- a/packages/core/src/api/apis/definitions/ErrorApi.ts +++ b/packages/core/src/api/apis/definitions/ErrorApi.ts @@ -15,6 +15,7 @@ */ import { createApiRef } from '../ApiRef'; +import { Observable } from '../../types'; /** * Mirrors the javascript Error class, for the purpose of @@ -54,6 +55,11 @@ export type ErrorApi = { * Post an error for handling by the application. */ post(error: Error, context?: ErrorContext): void; + + /** + * Observe errors posted by other parts of the application. + */ + error$(): Observable<{ error: Error; context?: ErrorContext }>; }; export const errorApiRef = createApiRef({ diff --git a/packages/core/src/api/apis/implementations/AlertApiForwarder.ts b/packages/core/src/api/apis/implementations/AlertApiForwarder.ts index 349512c0f9..c95408283c 100644 --- a/packages/core/src/api/apis/implementations/AlertApiForwarder.ts +++ b/packages/core/src/api/apis/implementations/AlertApiForwarder.ts @@ -14,22 +14,17 @@ * limitations under the License. */ import { AlertApi, AlertMessage } from '../../../'; - -type SubscriberFunc = (message: AlertMessage) => void; -type Unsubscribe = () => void; +import { PublishSubject } from './lib'; +import { Observable } from '../../types'; export class AlertApiForwarder implements AlertApi { - private readonly subscribers = new Set(); + private readonly subject = new PublishSubject(); post(alert: AlertMessage) { - this.subscribers.forEach(subscriber => subscriber(alert)); + this.subject.next(alert); } - subscribe(func: SubscriberFunc): Unsubscribe { - this.subscribers.add(func); - - return () => { - this.subscribers.delete(func); - }; + alert$(): Observable { + return this.subject; } } diff --git a/packages/core/src/api/apis/implementations/ErrorAlerter.ts b/packages/core/src/api/apis/implementations/ErrorAlerter.ts new file mode 100644 index 0000000000..81d0cc8fb8 --- /dev/null +++ b/packages/core/src/api/apis/implementations/ErrorAlerter.ts @@ -0,0 +1,39 @@ +/* + * 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 { ErrorApi, ErrorContext, AlertApi } from '../../../'; + +/** + * Decorates an ErrorApi by also forwarding error messages + * to the alertApi with an 'error' severity. + */ +export class ErrorAlerter implements ErrorApi { + constructor( + private readonly alertApi: AlertApi, + private readonly errorApi: ErrorApi, + ) {} + + post(error: Error, context?: ErrorContext) { + if (!context?.hidden) { + this.alertApi.post({ message: error.message, severity: 'error' }); + } + + return this.errorApi.post(error, context); + } + + error$() { + return this.errorApi.error$(); + } +} diff --git a/packages/core/src/api/apis/implementations/ErrorApiForwarder.ts b/packages/core/src/api/apis/implementations/ErrorApiForwarder.ts index 8d777ac18f..a61fdae5c1 100644 --- a/packages/core/src/api/apis/implementations/ErrorApiForwarder.ts +++ b/packages/core/src/api/apis/implementations/ErrorApiForwarder.ts @@ -13,33 +13,21 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { ErrorApi, ErrorContext, AlertApi } from '../../../'; - -type SubscriberFunc = (error: Error) => void; -type Unsubscribe = () => void; +import { ErrorApi, ErrorContext } from '../../../'; +import { PublishSubject } from './lib'; +import { Observable } from '../../types'; export class ErrorApiForwarder implements ErrorApi { - private readonly subscribers = new Set(); - private alertApi: AlertApi; - - constructor(alertApi: AlertApi) { - this.alertApi = alertApi; - } + private readonly subject = new PublishSubject<{ + error: Error; + context?: ErrorContext; + }>(); post(error: Error, context?: ErrorContext) { - if (context?.hidden) { - return; - } - - this.alertApi.post({ message: error.message, severity: 'error' }); - this.subscribers.forEach(subscriber => subscriber(error)); + this.subject.next({ error, context }); } - subscribe(func: SubscriberFunc): Unsubscribe { - this.subscribers.add(func); - - return () => { - this.subscribers.delete(func); - }; + error$(): Observable<{ error: Error; context?: ErrorContext }> { + return this.subject; } } diff --git a/packages/core/src/api/apis/implementations/index.ts b/packages/core/src/api/apis/implementations/index.ts index 0bf5cbbb24..1f2b91c16d 100644 --- a/packages/core/src/api/apis/implementations/index.ts +++ b/packages/core/src/api/apis/implementations/index.ts @@ -21,5 +21,6 @@ export * from './auth'; export * from './AppThemeSelector'; export * from './AlertApiForwarder'; +export * from './ErrorAlerter'; export * from './ErrorApiForwarder'; export * from './OAuthRequestManager'; diff --git a/packages/app/src/components/AlertDisplay/AlertDisplay.tsx b/packages/core/src/components/AlertDisplay/AlertDisplay.tsx similarity index 78% rename from packages/app/src/components/AlertDisplay/AlertDisplay.tsx rename to packages/core/src/components/AlertDisplay/AlertDisplay.tsx index d9dd0b240b..85b41f2e0a 100644 --- a/packages/app/src/components/AlertDisplay/AlertDisplay.tsx +++ b/packages/core/src/components/AlertDisplay/AlertDisplay.tsx @@ -15,25 +15,27 @@ */ import React, { FC, useEffect, useState } from 'react'; -import PropTypes from 'prop-types'; import { Snackbar, IconButton } from '@material-ui/core'; import CloseIcon from '@material-ui/icons/Close'; import { Alert } from '@material-ui/lab'; -import { AlertApiForwarder, AlertMessage } from '@backstage/core'; +import { AlertMessage, useApi, alertApiRef } from '../../api'; -type Props = { - forwarder: AlertApiForwarder; -}; +type Props = {}; // TODO: improve on this and promote to a shared component for use by all apps. -const AlertDisplay: FC = ({ forwarder }) => { +export const AlertDisplay: FC = () => { const [messages, setMessages] = useState>([]); + const alertApi = useApi(alertApiRef); useEffect(() => { - return forwarder.subscribe((message: AlertMessage) => - setMessages(msgs => msgs.concat(message)), - ); - }, [forwarder]); + const subscription = alertApi + .alert$() + .subscribe(message => setMessages(msgs => msgs.concat(message))); + + return () => { + subscription.unsubscribe(); + }; + }, [alertApi]); if (messages.length === 0) { return null; @@ -69,9 +71,3 @@ const AlertDisplay: FC = ({ forwarder }) => { ); }; - -AlertDisplay.propTypes = { - forwarder: PropTypes.instanceOf(AlertApiForwarder).isRequired, -}; - -export default AlertDisplay; diff --git a/packages/app/src/components/AlertDisplay/index.ts b/packages/core/src/components/AlertDisplay/index.ts similarity index 93% rename from packages/app/src/components/AlertDisplay/index.ts rename to packages/core/src/components/AlertDisplay/index.ts index b60bc1776b..7bfac0c981 100644 --- a/packages/app/src/components/AlertDisplay/index.ts +++ b/packages/core/src/components/AlertDisplay/index.ts @@ -14,4 +14,4 @@ * limitations under the License. */ -export { default } from './AlertDisplay'; +export * from './AlertDisplay'; diff --git a/packages/core/src/components/CopyTextButton/CopyTextButton.test.tsx b/packages/core/src/components/CopyTextButton/CopyTextButton.test.tsx index 695a700738..7bc59d1f48 100644 --- a/packages/core/src/components/CopyTextButton/CopyTextButton.test.tsx +++ b/packages/core/src/components/CopyTextButton/CopyTextButton.test.tsx @@ -44,6 +44,7 @@ const apiRegistry = ApiRegistry.from([ post(error) { throw error; }, + error$: jest.fn(), } as ErrorApi, ], ]); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 415f167067..013c80e2fa 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -28,6 +28,7 @@ export { default as InfoCard } from './layout/InfoCard'; export { CardTab, TabbedCard } from './layout/TabbedCard'; export { default as ErrorBoundary } from './layout/ErrorBoundary'; export * from './layout/Sidebar'; +export { AlertDisplay } from './components/AlertDisplay'; export { default as HorizontalScrollGrid } from './components/HorizontalScrollGrid'; export { default as ProgressCard } from './components/ProgressBars/ProgressCard'; export { default as CircleProgress } from './components/ProgressBars/CircleProgress'; diff --git a/packages/dev-utils/src/devApp/apiFactories.test.ts b/packages/dev-utils/src/devApp/apiFactories.test.ts index 55455346b9..be3ae2cdbe 100644 --- a/packages/dev-utils/src/devApp/apiFactories.test.ts +++ b/packages/dev-utils/src/devApp/apiFactories.test.ts @@ -15,12 +15,14 @@ */ import * as apiFactories from './apiFactories'; -import { ApiTestRegistry } from '@backstage/core'; +import { ApiTestRegistry, ApiFactory } from '@backstage/core'; describe('apiFactories', () => { it('should be possible to get an instance of each API', () => { const registry = new ApiTestRegistry(); - const factories = Object.values(apiFactories); + const factories: ApiFactory[] = Object.values( + apiFactories, + ); for (const factory of factories) { registry.register(factory); diff --git a/packages/dev-utils/src/devApp/apiFactories.ts b/packages/dev-utils/src/devApp/apiFactories.ts index a03faf0033..162375cf1c 100644 --- a/packages/dev-utils/src/devApp/apiFactories.ts +++ b/packages/dev-utils/src/devApp/apiFactories.ts @@ -20,6 +20,8 @@ import { ErrorApiForwarder, AlertApi, createApiFactory, + ErrorAlerter, + AlertApiForwarder, } from '@backstage/core'; // TODO(rugvip): We should likely figure out how to reuse all of these between apps @@ -30,18 +32,12 @@ import { export const alertApiFactory = createApiFactory({ implements: alertApiRef, deps: {}, - factory: (): AlertApi => ({ - // TODO: Figure out how to ship a nicer implementation without having - // to export any external references. - post(alertConfig) { - // eslint-disable-next-line no-alert - alert(`Alert[${alertConfig.severity}]: ${alertConfig.message}`); - }, - }), + factory: (): AlertApi => new AlertApiForwarder(), }); export const errorApiFactory = createApiFactory({ implements: errorApiRef, deps: { alertApi: alertApiRef }, - factory: ({ alertApi }) => new ErrorApiForwarder(alertApi), + factory: ({ alertApi }) => + new ErrorAlerter(alertApi, new ErrorApiForwarder()), }); diff --git a/packages/dev-utils/src/devApp/render.tsx b/packages/dev-utils/src/devApp/render.tsx index 1922e056a3..72c9581a84 100644 --- a/packages/dev-utils/src/devApp/render.tsx +++ b/packages/dev-utils/src/devApp/render.tsx @@ -29,6 +29,7 @@ import { createPlugin, ApiTestRegistry, ApiHolder, + AlertDisplay, } from '@backstage/core'; import * as defaultApiFactories from './apiFactories'; @@ -77,6 +78,7 @@ class DevAppBuilder { const DevApp: FC<{}> = () => { return ( + {sidebar} diff --git a/plugins/lighthouse/src/components/CreateAudit/index.test.tsx b/plugins/lighthouse/src/components/CreateAudit/index.test.tsx index f47dc3d475..b337698485 100644 --- a/plugins/lighthouse/src/components/CreateAudit/index.test.tsx +++ b/plugins/lighthouse/src/components/CreateAudit/index.test.tsx @@ -53,7 +53,7 @@ describe('CreateAudit', () => { let errorApi: ErrorApi; beforeEach(() => { - errorApi = { post: jest.fn() }; + errorApi = { post: jest.fn(), error$: jest.fn() }; apis = ApiRegistry.from([ [lighthouseApiRef, new LighthouseRestApi('http://lighthouse')], [errorApiRef, errorApi],