Review improvements

Signed-off-by: Sami Tiilikainen <97873007+samitiilikainen@users.noreply.github.com>

packages/core/src/main/logger/close-ipc-logging-listener.injectable.ts

@@ -11,10 +11,11 @@ import {
 const closeIpcFileLoggingListenerInjectable = getMessageChannelListenerInjectable({
   id: "close-ipc-file-logging",
   channel: closeIpcFileLoggerChannel,
-  handler: (di) => (fileId) =>
+  handler: (di) => {
-    di
+    const ipcFileLogger = di.inject(ipcFileLoggerInjectable);
-      .inject(ipcFileLoggerInjectable)
+
-      .close(fileId),
+    return (fileId) => ipcFileLogger.close(fileId);
+  },
 });
 
 export default closeIpcFileLoggingListenerInjectable;

packages/core/src/main/logger/create-ipc-file-transport.global-override-for-injectable.ts

@@ -0,0 +1,18 @@
+/**
+ * Copyright (c) OpenLens Authors. All rights reserved.
+ * Licensed under MIT License. See LICENSE in root directory for more information.
+ */
+
+import type { transports } from "winston";
+import { getGlobalOverride } from "@k8slens/test-utils";
+import { noop } from "@k8slens/utilities";
+import createIpcFileLoggerTranportInjectable from "./create-ipc-file-transport.injectable";
+
+export default getGlobalOverride(
+  createIpcFileLoggerTranportInjectable,
+  () => () =>
+    ({
+      log: noop,
+      close: noop,
+    } as typeof transports.File),
+);

packages/core/src/main/logger/create-ipc-file-transport.injectable.ts

@@ -0,0 +1,28 @@
+/**
+ * Copyright (c) OpenLens Authors. All rights reserved.
+ * Licensed under MIT License. See LICENSE in root directory for more information.
+ */
+import { getInjectable } from "@ogre-tools/injectable";
+import { transports } from "winston";
+import directoryForLogsInjectable from "../../common/app-paths/directory-for-logs.injectable";
+
+const createIpcFileLoggerTranportInjectable = getInjectable({
+  id: "create-ipc-file-logger-transport",
+  instantiate: (di) => {
+    const options = {
+      dirname: di.inject(directoryForLogsInjectable),
+      maxsize: 1024 * 1024,
+      maxFiles: 2,
+      tailable: true,
+    };
+
+    return (fileId: string) =>
+      new transports.File({
+        ...options,
+        filename: `lens-${fileId}.log`,
+      });
+  },
+  causesSideEffects: true,
+});
+
+export default createIpcFileLoggerTranportInjectable;

packages/core/src/main/logger/file-transport.injectable.ts

@@ -8,7 +8,7 @@ import directoryForLogsInjectable from "../../common/app-paths/directory-for-log
 import { loggerTransportInjectionToken } from "../../common/logger/transports";
 
 const fileLoggerTranportInjectable = getInjectable({
-  id: "file-logger-tranport",
+  id: "file-logger-transport",
   instantiate: (di) => new transports.File({
     handleExceptions: false,
     level: "debug",

packages/core/src/main/logger/ipc-file-logger.injectable.ts

@@ -3,23 +3,53 @@
  * Licensed under MIT License. See LICENSE in root directory for more information.
  */
 import { getInjectable } from "@ogre-tools/injectable";
-import { transports } from "winston";
+import { getOrInsertWith } from "@k8slens/utilities";
-import directoryForLogsInjectable from "../../common/app-paths/directory-for-logs.injectable";
+import type { LogEntry, transports } from "winston";
-import IpcFileLogger from "./ipc-file-logger";
+import createIpcFileLoggerTranportInjectable from "./create-ipc-file-transport.injectable";
+
+export interface IpcFileLogger {
+  log: (fileLog: { fileId: string; entry: LogEntry }) => void;
+  close: (fileId: string) => void;
+  closeAll: () => void;
+}
 
 const ipcFileLoggerInjectable = getInjectable({
   id: "ipc-file-logger",
-  instantiate: (di) =>
+  instantiate: (di): IpcFileLogger => {
-    new IpcFileLogger(
+    const createIpcFileTransport = di.inject(createIpcFileLoggerTranportInjectable);
-      {
+    const fileTransports = new Map<string, transports.FileTransportInstance>();
-        dirname: di.inject(directoryForLogsInjectable),
+
-        maxsize: 1024 * 1024,
+    function log({ fileId, entry }: { fileId: string; entry: LogEntry }) {
-        maxFiles: 2,
+      const transport = getOrInsertWith(
-        tailable: true,
+        fileTransports,
+        fileId,
+        () => createIpcFileTransport(fileId),
+      );
+
+      transport?.log?.(entry, () => {});
+    }
+
+    function close(fileId: string) {
+      const transport = fileTransports.get(fileId);
+
+      if (transport) {
+        transport.close?.();
+        fileTransports.delete(fileId);
+      }
+    }
+
+    function closeAll() {
+      for (const fileId of fileTransports.keys()) {
+        close(fileId);
+      }
+    }
+
+    return {
+      log,
+      close,
+      closeAll,
+    };
   },
-      (options: transports.FileTransportOptions) => new transports.File(options),
-    ),
-  causesSideEffects: true,
 });
 
 export default ipcFileLoggerInjectable;

packages/core/src/main/logger/ipc-file-logger.test.ts

@@ -2,7 +2,10 @@
  * Copyright (c) OpenLens Authors. All rights reserved.
  * Licensed under MIT License. See LICENSE in root directory for more information.
  */
-import IpcFileLogger from "./ipc-file-logger";
+import { getDiForUnitTesting } from "../getDiForUnitTesting";
+import createIpcFileLoggerTranportInjectable from "./create-ipc-file-transport.injectable";
+import type { IpcFileLogger } from "./ipc-file-logger.injectable";
+import ipcFileLoggerInjectable from "./ipc-file-logger.injectable";
 
 describe("ipc file logger in main", () => {
   let logMock: jest.Mock;
@@ -17,14 +20,11 @@ describe("ipc file logger in main", () => {
       log: logMock,
       close: closeMock,
     }));
-    logger = new IpcFileLogger(
+
-      {
+    const di = getDiForUnitTesting();
-        dirname: "some-logs-dir",
+
-        maxFiles: 1,
+    di.override(createIpcFileLoggerTranportInjectable, () => createFileTransportMock);
-        tailable: true,
+    logger = di.inject(ipcFileLoggerInjectable);
-      },
-      createFileTransportMock,
-    );
   });
 
   it("creates a transport for new log file", () => {
@@ -33,12 +33,7 @@ describe("ipc file logger in main", () => {
       entry: { level: "irrelevant", message: "irrelevant" },
     });
 
-    expect(createFileTransportMock).toHaveBeenCalledWith({
+    expect(createFileTransportMock).toHaveBeenCalledWith("some-log-file");
-      dirname: "some-logs-dir",
-      filename: "lens-some-log-file.log",
-      maxFiles: 1,
-      tailable: true,
-    });
   });
 
   it("uses existing transport for log file", () => {
@@ -59,12 +54,7 @@ describe("ipc file logger in main", () => {
 
     expect(createFileTransportMock).toHaveBeenCalledTimes(1);
 
-    expect(createFileTransportMock).toHaveBeenCalledWith({
+    expect(createFileTransportMock).toHaveBeenCalledWith("some-log-file");
-      dirname: "some-logs-dir",
-      filename: "lens-some-log-file.log",
-      maxFiles: 1,
-      tailable: true,
-    });
   });
 
   it("creates separate transport for each log file", () => {
@@ -85,26 +75,11 @@ describe("ipc file logger in main", () => {
 
     expect(createFileTransportMock).toHaveBeenCalledTimes(3);
 
-    expect(createFileTransportMock).toHaveBeenCalledWith({
+    expect(createFileTransportMock).toHaveBeenCalledWith("some-log-file");
-      dirname: "some-logs-dir",
-      filename: "lens-some-log-file.log",
-      maxFiles: 1,
-      tailable: true,
-    });
 
-    expect(createFileTransportMock).toHaveBeenCalledWith({
+    expect(createFileTransportMock).toHaveBeenCalledWith("some-other-log-file");
-      dirname: "some-logs-dir",
-      filename: "lens-some-other-log-file.log",
-      maxFiles: 1,
-      tailable: true,
-    });
 
-    expect(createFileTransportMock).toHaveBeenCalledWith({
+    expect(createFileTransportMock).toHaveBeenCalledWith("some-yet-another-log-file");
-      dirname: "some-logs-dir",
-      filename: "lens-some-yet-another-log-file.log",
-      maxFiles: 1,
-      tailable: true,
-    });
   });
 
   it("logs using file transport", () => {
@@ -122,12 +97,12 @@ describe("ipc file logger in main", () => {
     const someLogMock = jest.fn();
     const someOthertLogMock = jest.fn();
 
-    createFileTransportMock.mockImplementation((options) => {
+    createFileTransportMock.mockImplementation((fileId: string) => {
-      if (options.filename === "lens-some-log-file.log") {
+      if (fileId === "some-log-file") {
         return { log: someLogMock };
       }
 
-      if (options.filename === "lens-some-other-log-file.log") {
+      if (fileId === "some-other-log-file") {
         return { log: someOthertLogMock };
       }
 

packages/core/src/main/logger/ipc-file-logger.ts

@@ -1,56 +0,0 @@
-/**
- * Copyright (c) OpenLens Authors. All rights reserved.
- * Licensed under MIT License. See LICENSE in root directory for more information.
- */
-import type { LogEntry, transports } from "winston";
-
-type IpcFileLoggerOptions = Omit<transports.FileTransportOptions, "filename">;
-
-class IpcFileLogger {
-  private fileTransports = new Map<string, transports.FileTransportInstance>();
-
-  constructor(
-    private options: IpcFileLoggerOptions,
-    private createNewFileTransport: (
-      options: transports.FileTransportOptions
-    ) => transports.FileTransportInstance,
-  ) {}
-
-  log({ fileId, entry }: { fileId: string; entry: LogEntry }) {
-    const transport = this.ensureTransportForFile(fileId);
-
-    transport?.log?.(entry, () => {});
-  }
-
-  close(fileId: string) {
-    const transport = this.fileTransports.get(fileId);
-
-    if (transport) {
-      transport.close?.();
-      this.fileTransports.delete(fileId);
-    }
-  }
-
-  closeAll() {
-    [...this.fileTransports.keys()].forEach((fileId) => {
-      this.close(fileId);
-    });
-  }
-
-  private ensureTransportForFile(fileId: string) {
-    if (this.fileTransports.has(fileId)) {
-      return this.fileTransports.get(fileId);
-    }
-
-    const fileTransport = this.createNewFileTransport({
-      ...this.options,
-      filename: `lens-${fileId}.log`,
-    });
-
-    this.fileTransports.set(fileId, fileTransport);
-
-    return fileTransport;
-  }
-}
-
-export default IpcFileLogger;

packages/core/src/main/logger/ipc-logging-listener.injectable.ts

@@ -4,11 +4,8 @@
  */
 import ipcFileLoggerInjectable from "./ipc-file-logger.injectable";
 import { getMessageChannelListenerInjectable } from "../../common/utils/channel/message-channel-listener-injection-token";
-import type {
+import type { IpcFileLogObject } from "../../common/logger/ipc-file-logger-channel";
-  IpcFileLogObject } from "../../common/logger/ipc-file-logger-channel";
+import { ipcFileLoggerChannel } from "../../common/logger/ipc-file-logger-channel";
-import {
-  ipcFileLoggerChannel,
-} from "../../common/logger/ipc-file-logger-channel";
 import { MESSAGE } from "triple-beam";
 
 /**
@@ -31,10 +28,12 @@ export function deserializeLogFromIpc(ipcFileLogObject: IpcFileLogObject) {
 const ipcFileLoggingListenerInjectable = getMessageChannelListenerInjectable({
   id: "ipc-file-logging",
   channel: ipcFileLoggerChannel,
-  handler: (di) => (ipcFileLogObject) =>
+  handler: (di) => {
-    di
+    const logger = di.inject(ipcFileLoggerInjectable);
-      .inject(ipcFileLoggerInjectable)
+
-      .log(deserializeLogFromIpc(ipcFileLogObject)),
+    return (ipcFileLogObject) =>
+      logger.log(deserializeLogFromIpc(ipcFileLogObject));
+  },
 });
 
 export default ipcFileLoggingListenerInjectable;

packages/core/src/renderer/logger/renderer-log-file-id.injectable.ts

@@ -17,7 +17,7 @@ const rendererLogFileIdInjectable = getInjectable({
       const { host } = di.inject(windowLocationInjectable);
       const clusterId = getClusterIdFromHost(host);
 
-      frameId = clusterId ? `cluster-${clusterId}` : "cluster";
+      frameId = `cluster-${clusterId}`;
     } else {
       frameId = "main";
     }