From 6d3f745b4b5526895df12c9cb42ad0bdd425b465 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 25 May 2020 18:10:34 +0200 Subject: [PATCH 1/5] packages/app: move AlertDisplay to core --- packages/app/package.json | 1 - packages/app/src/App.tsx | 3 +-- packages/core/package.json | 1 + .../src/components/AlertDisplay/AlertDisplay.tsx | 6 ++---- packages/{app => core}/src/components/AlertDisplay/index.ts | 2 +- packages/core/src/index.ts | 1 + 6 files changed, 6 insertions(+), 8 deletions(-) rename packages/{app => core}/src/components/AlertDisplay/AlertDisplay.tsx (93%) rename packages/{app => core}/src/components/AlertDisplay/index.ts (93%) 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..327bca48f8 100644 --- a/packages/app/src/App.tsx +++ b/packages/app/src/App.tsx @@ -14,11 +14,10 @@ * 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'; 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/app/src/components/AlertDisplay/AlertDisplay.tsx b/packages/core/src/components/AlertDisplay/AlertDisplay.tsx similarity index 93% rename from packages/app/src/components/AlertDisplay/AlertDisplay.tsx rename to packages/core/src/components/AlertDisplay/AlertDisplay.tsx index d9dd0b240b..23d1c7825f 100644 --- a/packages/app/src/components/AlertDisplay/AlertDisplay.tsx +++ b/packages/core/src/components/AlertDisplay/AlertDisplay.tsx @@ -19,14 +19,14 @@ 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 { AlertApiForwarder, AlertMessage } from '../../api'; type Props = { forwarder: AlertApiForwarder; }; // TODO: improve on this and promote to a shared component for use by all apps. -const AlertDisplay: FC = ({ forwarder }) => { +export const AlertDisplay: FC = ({ forwarder }) => { const [messages, setMessages] = useState>([]); useEffect(() => { @@ -73,5 +73,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/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'; From 9be7870a34459e4af77c4c84ff5e21af98154bf2 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 25 May 2020 21:51:06 +0200 Subject: [PATCH 2/5] packages/core: add consumer side to AlertApi and use for AlertDialog + friends --- packages/app/src/App.tsx | 4 +-- .../core/src/api/apis/definitions/AlertApi.ts | 6 +++++ .../apis/implementations/AlertApiForwarder.ts | 17 +++++------- .../components/AlertDisplay/AlertDisplay.tsx | 26 +++++++++---------- 4 files changed, 26 insertions(+), 27 deletions(-) diff --git a/packages/app/src/App.tsx b/packages/app/src/App.tsx index 327bca48f8..b4d01e067b 100644 --- a/packages/app/src/App.tsx +++ b/packages/app/src/App.tsx @@ -19,7 +19,7 @@ import React, { FC } from 'react'; import { BrowserRouter as Router } from 'react-router-dom'; import Root from './components/Root'; import * as plugins from './plugins'; -import apis, { alertApiForwarder } from './apis'; +import apis from './apis'; const app = createApp({ apis, @@ -31,7 +31,7 @@ const AppComponent = app.getRootComponent(); const App: FC<{}> = () => ( - + 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/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/components/AlertDisplay/AlertDisplay.tsx b/packages/core/src/components/AlertDisplay/AlertDisplay.tsx index 23d1c7825f..85b41f2e0a 100644 --- a/packages/core/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 '../../api'; +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. -export 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,7 +71,3 @@ export const AlertDisplay: FC = ({ forwarder }) => { ); }; - -AlertDisplay.propTypes = { - forwarder: PropTypes.instanceOf(AlertApiForwarder).isRequired, -}; From 4e7e0833cc352e852f257cbf14dfcf1630464155 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 25 May 2020 22:01:20 +0200 Subject: [PATCH 3/5] packages/core: add observable consumer to ErrorApi --- .../core/src/api/apis/definitions/ErrorApi.ts | 6 ++++ .../apis/implementations/ErrorApiForwarder.ts | 30 ++++++++----------- .../CopyTextButton/CopyTextButton.test.tsx | 1 + .../src/components/CreateAudit/index.test.tsx | 2 +- 4 files changed, 20 insertions(+), 19 deletions(-) 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/ErrorApiForwarder.ts b/packages/core/src/api/apis/implementations/ErrorApiForwarder.ts index 8d777ac18f..79397c0a67 100644 --- a/packages/core/src/api/apis/implementations/ErrorApiForwarder.ts +++ b/packages/core/src/api/apis/implementations/ErrorApiForwarder.ts @@ -14,32 +14,26 @@ * limitations under the License. */ import { ErrorApi, ErrorContext, AlertApi } from '../../../'; - -type SubscriberFunc = (error: Error) => void; -type Unsubscribe = () => void; +import { PublishSubject } from './lib'; +import { Observable } from '../../types'; export class ErrorApiForwarder implements ErrorApi { - private readonly subscribers = new Set(); - private alertApi: AlertApi; + private readonly subject = new PublishSubject<{ + error: Error; + context?: ErrorContext; + }>(); - constructor(alertApi: AlertApi) { - this.alertApi = alertApi; - } + constructor(private readonly alertApi: AlertApi) {} post(error: Error, context?: ErrorContext) { - if (context?.hidden) { - return; + if (!context?.hidden) { + this.alertApi.post({ message: error.message, severity: 'error' }); } - 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/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/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], From 5c7d263aa8ba113a3b37110002c3df329fcc3a94 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 25 May 2020 22:07:54 +0200 Subject: [PATCH 4/5] packages/core: split alerting functionality out from ErrorApiForwarder into ErrorAlerter --- docs/getting-started/utility-apis.md | 5 ++- packages/app/src/apis.ts | 7 ++-- .../api/apis/implementations/ErrorAlerter.ts | 39 +++++++++++++++++++ .../apis/implementations/ErrorApiForwarder.ts | 8 +--- .../src/api/apis/implementations/index.ts | 1 + 5 files changed, 47 insertions(+), 13 deletions(-) create mode 100644 packages/core/src/api/apis/implementations/ErrorAlerter.ts 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/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/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 79397c0a67..a61fdae5c1 100644 --- a/packages/core/src/api/apis/implementations/ErrorApiForwarder.ts +++ b/packages/core/src/api/apis/implementations/ErrorApiForwarder.ts @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { ErrorApi, ErrorContext, AlertApi } from '../../../'; +import { ErrorApi, ErrorContext } from '../../../'; import { PublishSubject } from './lib'; import { Observable } from '../../types'; @@ -23,13 +23,7 @@ export class ErrorApiForwarder implements ErrorApi { context?: ErrorContext; }>(); - constructor(private readonly alertApi: AlertApi) {} - post(error: Error, context?: ErrorContext) { - if (!context?.hidden) { - this.alertApi.post({ message: error.message, severity: 'error' }); - } - this.subject.next({ error, context }); } 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'; From fe08f81359f00ebb8294215f2151609bb543c77f Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 25 May 2020 22:15:02 +0200 Subject: [PATCH 5/5] packages/dev-utils: update to reflect changes in core and include proper alert display --- packages/dev-utils/src/devApp/apiFactories.test.ts | 6 ++++-- packages/dev-utils/src/devApp/apiFactories.ts | 14 +++++--------- packages/dev-utils/src/devApp/render.tsx | 2 ++ 3 files changed, 11 insertions(+), 11 deletions(-) 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}