Fabiangosebrink/adding abstract services

[FabianGosebrink]

#1416

[FabianGosebrink]

@timdeschryver if you have the time to review, feel free :)

[github-actions]

Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-hill-002425310-1422.centralus.azurestaticapps.net

[timdeschryver]

@FabianGosebrink I'll make some time for this next week.

[timdeschryver]

I think that a couple of changes already were pushed to main (ad4b090).

This PR looks good to me and I also like the changes to the storage, this makes things consistent and flexible.
But I had something else in mind for the logger.

import { Injectable } from '@angular/core';
import { OpenIdConfiguration } from '../config/openid-configuration';
import { AbstractLoggerService } from './abstract-logger.service';
import { LogLevel } from './log-level';

@Injectable()
export class LoggerService  {

+  constructor(private log: AbstractLoggerService) {}

  logError(configuration: OpenIdConfiguration, message: any, ...args: any[]): void {
    if (this.loggingIsTurnedOff(configuration)) {
      return;
    }

    const { configId } = configuration;

    if (!!args && !!args.length) {
-      console.error(`[ERROR] ${configId} - ${message}`, ...args);
+      this.log.logError(`[ERROR] ${configId} - ${message}`, ...args);
    } else {
-      console.error(`[ERROR] ${configId} - ${message}`);
+      this.log.logError(`[ERROR] ${configId} - ${message}`);
    }
  }

  logWarning(configuration: OpenIdConfiguration, message: any, ...args: any[]): void {
    if (!this.logLevelIsSet(configuration)) {
      return;
    }

    if (this.loggingIsTurnedOff(configuration)) {
      return;
    }

    if (!this.currentLogLevelIsEqualOrSmallerThan(configuration, LogLevel.Warn)) {
      return;
    }

    const { configId } = configuration;

    if (!!args && !!args.length) {
-      console.warn(`[WARN] ${configId} - ${message}`, ...args);
+      this.log.logWarning(`[WARN] ${configId} - ${message}`, ...args);
    } else {
-      console.warn(`[WARN] ${configId} - ${message}`);
+      this.log.logWarning(`[WARN] ${configId} - ${message}`);
    }
  }

  logDebug(configuration: OpenIdConfiguration, message: any, ...args: any[]): void {
    if (!this.logLevelIsSet(configuration)) {
      return;
    }

    if (this.loggingIsTurnedOff(configuration)) {
      return;
    }

    if (!this.currentLogLevelIsEqualOrSmallerThan(configuration, LogLevel.Debug)) {
      return;
    }

    const { configId } = configuration;

    if (!!args && !!args.length) {
-      console.log(`[DEBUG] ${configId} - ${message}`, ...args);
+      this.log.logDebug(`[DEBUG] ${configId} - ${message}`, ...args);
    } else {
-      console.log(`[DEBUG] ${configId} - ${message}`);
+      this.log.logDebug(`[DEBUG] ${configId} - ${message}`);
    }
  }

  private currentLogLevelIsEqualOrSmallerThan(configuration: OpenIdConfiguration, logLevelToCompare: LogLevel): boolean {
    const { logLevel } = configuration || {};

    return logLevel <= logLevelToCompare;
  }

  private logLevelIsSet(configuration: OpenIdConfiguration): boolean {
    const { logLevel } = configuration || {};

    if (logLevel === null) {
      return false;
    }

    if (logLevel === undefined) {
      return false;
    }

    return true;
  }

  private loggingIsTurnedOff(configuration: OpenIdConfiguration): boolean {
    const { logLevel } = configuration || {};

    return logLevel === LogLevel.None;
  }
}

The reasoning behind this, is that the logger service already contains some useful checks whether that it should log or not, and these are fine. From a consumers view, I just want to use another logger implementation than console.log.

With the current implementation we would have to rewrite that logic, which is fine if that's intended.
Thoughts?

[FabianGosebrink]

I like that idea. That of course makes sense. With this, the user can use the checks which are already there. Will implement this and push. But with this we would need another logger service, like an ConsoleLoggerService only having console.logs in it which implements the AbstractLoggerService. Or am I mixing something here? @timdeschryver . IDK why some things are already on main tbh...

[github-actions]

Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-hill-002425310-1422.centralus.azurestaticapps.net

[github-actions]

Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-hill-002425310-1422.centralus.azurestaticapps.net

[timdeschryver]

@FabianGosebrink Exactly, this is perfect! 🤩
Thanks for implementing this so quickly.

[FabianGosebrink]

Thank you for providing such a valuable and good feedback so quickly! Thanks :)