Merge pull request #1003 from spotify/rugvip/apimove

core: refactor ErrorApi and AlertApi

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(),

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",

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<{}> = () => (
   <AppProvider>
-    <AlertDisplay forwarder={alertApiForwarder} />
+    <AlertDisplay />
     <OAuthRequestDialog />
     <Router>
       <Root>

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());
 

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",

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<AlertMessage>;
 };
 
 export const alertApiRef = createApiRef<AlertApi>({

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<ErrorApi>({

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<SubscriberFunc>();
+  private readonly subject = new PublishSubject<AlertMessage>();
 
   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<AlertMessage> {
+    return this.subject;
   }
 }

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$();
+  }
+}

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<SubscriberFunc>();
-  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;
   }
 }

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';

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<Props> = ({ forwarder }) => {
+export const AlertDisplay: FC<Props> = () => {
   const [messages, setMessages] = useState<Array<AlertMessage>>([]);
+  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<Props> = ({ forwarder }) => {
     </Snackbar>
   );
 };
-
-AlertDisplay.propTypes = {
-  forwarder: PropTypes.instanceOf(AlertApiForwarder).isRequired,
-};
-
-export default AlertDisplay;

packages/core/src/components/AlertDisplay/index.ts

@@ -14,4 +14,4 @@
  * limitations under the License.
  */
 
-export { default } from './AlertDisplay';
+export * from './AlertDisplay';

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,
   ],
 ]);

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';

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<unknown, unknown, unknown>[] = Object.values(
+      apiFactories,
+    );
 
     for (const factory of factories) {
       registry.register(factory);

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()),
 });

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 (
         <AppProvider>
+          <AlertDisplay />
           <BrowserRouter>
             <SidebarPage>
               {sidebar}

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],