fix: address review comments

Signed-off-by: Heikki Hellgren <heikki.hellgren@op.fi>

plugins/signals/api-report.md

@@ -23,9 +23,12 @@ export class SignalClient implements SignalApi {
   // (undocumented)
   static readonly DEFAULT_RECONNECT_TIMEOUT_MS: number;
   // (undocumented)
-  subscribe(topic: string, onMessage: (message: JsonObject) => void): string;
-  // (undocumented)
-  unsubscribe(subscription: string): void;
+  subscribe(
+    topic: string,
+    onMessage: (message: JsonObject) => void,
+  ): {
+    unsubscribe: () => void;
+  };
 }
 
 // @public (undocumented)

plugins/signals/package.json

@@ -28,7 +28,7 @@
     "@backstage/plugin-signals-react": "workspace:^",
     "@backstage/theme": "workspace:^",
     "@backstage/types": "workspace:^",
-    "@material-ui/core": "^4.9.13",
+    "@material-ui/core": "^4.12.4",
     "@material-ui/icons": "^4.9.1",
     "@material-ui/lab": "^4.0.0-alpha.61",
     "react-use": "^17.2.4",
@@ -42,8 +42,8 @@
     "@backstage/core-app-api": "workspace:^",
     "@backstage/dev-utils": "workspace:^",
     "@backstage/test-utils": "workspace:^",
-    "@testing-library/jest-dom": "^5.10.1",
-    "@testing-library/react": "^12.1.3",
+    "@testing-library/jest-dom": "^6.0.0",
+    "@testing-library/react": "^14.0.0",
     "@testing-library/user-event": "^14.0.0",
     "jest-websocket-mock": "^2.5.0",
     "msw": "^1.0.0"

plugins/signals/src/api/SignalClient.ts

@@ -62,7 +62,10 @@ export class SignalClient implements SignalApi {
     private reconnectTimeout: number,
   ) {}
 
-  subscribe(topic: string, onMessage: (message: JsonObject) => void): string {
+  subscribe(
+    topic: string,
+    onMessage: (message: JsonObject) => void,
+  ): { unsubscribe: () => void } {
     const subscriptionId = uuid();
     const exists = [...this.subscriptions.values()].find(
       sub => sub.topic === topic,
@@ -79,30 +82,30 @@ export class SignalClient implements SignalApi {
       .catch(() => {
         this.reconnect();
       });
-    return subscriptionId;
-  }
 
-  unsubscribe(subscription: string): void {
-    const sub = this.subscriptions.get(subscription);
-    if (!sub) {
-      return;
-    }
-    const topic = sub.topic;
-    this.subscriptions.delete(subscription);
-    const exists = [...this.subscriptions.values()].find(
-      s => s.topic === topic,
-    );
-    // If there are subscriptions still listening to this topic, do not
-    // unsubscribe from the server
-    if (!exists) {
-      this.send({ action: 'unsubscribe', topic: sub.topic });
-    }
+    const unsubscribe = () => {
+      const sub = this.subscriptions.get(subscriptionId);
+      if (!sub) {
+        return;
+      }
+      this.subscriptions.delete(subscriptionId);
+      const multipleExists = [...this.subscriptions.values()].find(
+        s => s.topic === topic,
+      );
+      // If there are subscriptions still listening to this topic, do not
+      // unsubscribe from the server
+      if (!multipleExists) {
+        this.send({ action: 'unsubscribe', topic: sub.topic });
+      }
 
-    // If there are no subscriptions, close the connection
-    if (this.subscriptions.size === 0) {
-      this.ws?.close(WS_CLOSE_NORMAL);
-      this.ws = null;
-    }
+      // If there are no subscriptions, close the connection
+      if (this.subscriptions.size === 0) {
+        this.ws?.close(WS_CLOSE_NORMAL);
+        this.ws = null;
+      }
+    };
+
+    return { unsubscribe };
   }
 
   private send(data?: JsonObject): void {

plugins/signals/src/api/SignalsClient.test.ts

@@ -44,7 +44,7 @@ describe('SignalsClient', () => {
   it('should handle single subscription correctly', async () => {
     const messageMock = jest.fn();
     const client = SignalClient.create({ discoveryApi, identity });
-    const sub = client.subscribe('topic', messageMock);
+    const { unsubscribe } = client.subscribe('topic', messageMock);
     await server.connected;
 
     await expect(server).toReceiveMessage({
@@ -54,7 +54,8 @@ describe('SignalsClient', () => {
     server.send({ topic: 'topic', message: { hello: 'world' } });
     expect(messageMock).toHaveBeenCalledWith({ hello: 'world' });
 
-    client.unsubscribe(sub);
+    await unsubscribe();
+
     await expect(server).toReceiveMessage({
       action: 'unsubscribe',
       topic: 'topic',
@@ -66,8 +67,14 @@ describe('SignalsClient', () => {
     const messageMock2 = jest.fn();
     const client1 = SignalClient.create({ discoveryApi, identity });
     const client2 = SignalClient.create({ discoveryApi, identity });
-    const sub1 = client1.subscribe('topic', messageMock1);
-    const sub2 = client2.subscribe('topic', messageMock2);
+    const { unsubscribe: unsubscribe1 } = client1.subscribe(
+      'topic',
+      messageMock1,
+    );
+    const { unsubscribe: unsubscribe2 } = client2.subscribe(
+      'topic',
+      messageMock2,
+    );
 
     await server.connected;
 
@@ -79,13 +86,13 @@ describe('SignalsClient', () => {
     expect(messageMock1).toHaveBeenCalledWith({ hello: 'world' });
     expect(messageMock2).toHaveBeenCalledWith({ hello: 'world' });
 
-    client1.unsubscribe(sub1);
+    await unsubscribe1();
     await expect(server).not.toReceiveMessage({
       action: 'unsubscribe',
       topic: 'topic',
     });
 
-    client2.unsubscribe(sub2);
+    await unsubscribe2();
     await expect(server).toReceiveMessage({
       action: 'unsubscribe',
       topic: 'topic',

plugins/signals/src/plugin.ts

@@ -22,8 +22,6 @@ import {
 import { signalApiRef } from '@backstage/plugin-signals-react';
 import { SignalClient } from './api/SignalClient';
 
-let clientInstance: SignalClient | undefined = undefined;
-
 /** @public */
 export const signalsPlugin = createPlugin({
   id: 'signals',
@@ -35,13 +33,10 @@ export const signalsPlugin = createPlugin({
         discoveryApi: discoveryApiRef,
       },
       factory: ({ identity, discoveryApi }) => {
-        if (!clientInstance) {
-          clientInstance = SignalClient.create({
-            identity,
-            discoveryApi,
-          });
-        }
-        return clientInstance;
+        return SignalClient.create({
+          identity,
+          discoveryApi,
+        });
       },
     }),
   ],