Feedback from @mikelehen and @schmidt-sebastian

Refactor to also log fatal issues

Refactor to not attempt to log if the level is invalid

Adding docs and more feedback

Author: jshcrowthe

packages/database/src/core/PersistentConnection.ts

@@ -936,7 +936,7 @@ export class PersistentConnection extends ServerActions {
       this.securityDebugCallback_(body);
     } else {
       if ('msg' in body) {
-        log('FIREBASE: ' + body['msg'].replace('\n', '\nFIREBASE: '));
+        console.log('FIREBASE: ' + body['msg'].replace('\n', '\nFIREBASE: '));
       }
     }
   }

packages/database/src/core/Repo.ts

@@ -653,7 +653,7 @@ export class Repo {
     forEach(stats, (stat: string, value: any) => {
       // pad stat names to be the same length (plus 2 extra spaces).
       for (let i = stat.length; i < longestName + 2; i++) stat += ' ';
-      log(stat + value);
+      console.log(stat + value);
     });
   }
 

packages/database/src/core/util/util.ts

@@ -29,7 +29,7 @@ import { SessionStorage } from '../storage/storage';
 import { isNodeSdk } from '@firebase/util';
 import { Logger } from '@firebase/logger';
 
-const logClient = new Logger();
+const logClient = new Logger('@firebase/database');
 
 /**
  * Returns a locally-unique ID (generated by just incrementing up from 0 each time its called).
@@ -159,8 +159,9 @@ export const error = function(...var_args: string[]) {
  * @param {...string} var_args
  */
 export const fatal = function(...var_args: string[]) {
-  const message = buildLogMessage_(...var_args);
-  throw new Error('FIREBASE FATAL ERROR: ' + message);
+  const message = `FIREBASE FATAL ERROR: ${buildLogMessage_(...var_args)}`;
+  logClient.error(message);
+  throw new Error(message);
 };
 
 /**

packages/firestore/src/util/log.ts

@@ -19,9 +19,9 @@
 import { SDK_VERSION } from '../core/version';
 import { AnyJs } from './misc';
 import { PlatformSupport } from '../platform/platform';
-import { Logger, LogLevel as _LogLevel } from '@firebase/logger';
+import { Logger, LogLevel as FirebaseLogLevel } from '@firebase/logger';
 
-const client = new Logger();
+const logClient = new Logger('@firebase/firestore');
 
 export enum LogLevel {
   DEBUG,
@@ -31,30 +31,36 @@ export enum LogLevel {
 
 // Helper methods are needed because variables can't be exported as read/write
 export function getLogLevel(): LogLevel {
-  if (client.logLevel === _LogLevel.DEBUG) {
+  if (logClient.logLevel === FirebaseLogLevel.DEBUG) {
     return LogLevel.DEBUG;
-  }
-
-  if (client.logLevel === _LogLevel.SILENT) {
+  } else if (logClient.logLevel === FirebaseLogLevel.SILENT) {
     return LogLevel.SILENT;
+  } else {
+    return LogLevel.ERROR;
   }
-
-  return LogLevel.ERROR;
 }
 export function setLogLevel(newLevel: LogLevel): void {
-  client.logLevel = newLevel;
+  /**
+   * Since Firestore's LogLevel enum is a pure subset of Firebase's LogLevel, we
+   * can return it directly.
+   */
+  logClient.logLevel = newLevel;
 }
 
 export function debug(tag: string, msg: string, ...obj: AnyJs[]): void {
-  const time = new Date().toISOString();
-  const args = obj.map(argToString);
-  client.log(`Firestore (${SDK_VERSION}) ${time} [${tag}]: ${msg}`, ...args);
+  if (logClient.logLevel <= FirebaseLogLevel.DEBUG) {
+    const time = new Date().toISOString();
+    const args = obj.map(argToString);
+    logClient.log(`Firestore (${SDK_VERSION}) ${time} [${tag}]: ${msg}`, ...args);
+  }
 }
 
 export function error(msg: string, ...obj: AnyJs[]): void {
-  const time = new Date().toISOString();
-  const args = obj.map(argToString);
-  client.error(`Firestore (${SDK_VERSION}) ${time}: ${msg}`, ...args);
+  if (logClient.logLevel <= FirebaseLogLevel.ERROR) {
+    const time = new Date().toISOString();
+    const args = obj.map(argToString);
+    logClient.error(`Firestore (${SDK_VERSION}) ${time}: ${msg}`, ...args);
+  }
 }
 
 /**

packages/logger/README.md

@@ -1,13 +1,39 @@
-# @firebase/template
-
-This package can be used as a template for anyone creating new packages in the
-Firebase JS SDK. It will give you a couple things OOTB:
-
-- **Typescript Support:** Your code should be written in TS to be consistent
-  with the rest of the SDK.
-- **Isomorphic Testing/Coverage:** Your tests will be run in both Node.js and
-  Browser environments and coverage from both, collected.
-- **Links to all of the other packages:** Should your new package need to take
-  a dependency on any of the other packages in this monorepo (e.g.
-  `@firebase/app`, `@firebase/util`, etc), all those dependencies are already
-  set up, you can just remove the ones you don't need.
+# @firebase/logger
+
+This package serves as the base of all logging in the JS SDK. Any logging that
+is intended to be for firebase end developers, should go through this module.
+
+## Basic Usage
+
+Firebase components should import the `Logger` class and instantiate a new
+instance by passing a component name (e.g. `@firebase/<COMPONENT>`) to the
+constructor.
+
+_e.g._
+
+```typescript
+import { Logger } from '@firebase/logger';
+
+const logClient = new Logger(`@firebase/<COMPONENT>`);
+```
+
+Each `Logger` instance supports 5 log functions each to be used in a specific
+instance:
+
+- `debug`: Internal logs; use this to allow developers to send us their debug
+  logs for us to be able to diagnose an issue.
+- `log`: Use to inform your user about things they may need to know.
+- `info`: Use if you have to inform the user about something that they need to
+  take a concrete action on. Once they take that action, the log should go away.
+- `warn`: Use when a product feature may stop functioning correctly; unexpected
+  scenario. Majority of user-facing logs.
+- `error`: Only use when user App would stop functioning correctly - super rare!
+
+## Log Format
+
+Each log will be formatted in the following manner:
+
+```typescript
+`[${new Date()}]  ${this.name}: ${...args}`
+```
+

packages/logger/gulpfile.js

@@ -23,7 +23,7 @@ const buildModule = gulp.parallel([
 ]);
 
 const setupWatcher = () => {
-  gulp.watch('src/**/*', buildModule);
+  gulp.watch(['index.ts', 'src/**/*'], buildModule);
 };
 
 gulp.task('build', buildModule);

packages/logger/index.ts

@@ -21,13 +21,12 @@
  * just use index.ts
  */
 
-import { instances, setDefaultLogLevel, LogLevel } from './src/logger';
+import { instances, LogLevel } from './src/logger';
 
 export function setLogLevel(level: LogLevel) {
   instances.forEach(inst => {
     inst.logLevel = level;
   });
-  setDefaultLogLevel(level);
 }
 
 export { Logger, LogLevel, LogHandler } from './src/logger';

packages/logger/src/logger.ts

@@ -14,13 +14,6 @@
  * limitations under the License.
  */
 
-/**
- * This is the file that people using Node.js will actually import. You should
- * only include this file if you have something specific about your
- * implementation that mandates having a separate entrypoint. Otherwise you can
- * just use index.ts
- */
-
 /**
  * A container for all of the Logger instances
  */
@@ -31,15 +24,15 @@ export const instances: Logger[] = [];
  * silence the logs altogether.
  *
  * The order is a follows:
- * DEBUG < VERBOSE < INFO < WARN < ERROR < SILENT
+ * DEBUG < LOG < INFO < WARN < ERROR < SILENT
  *
  * All of the log types above the current log level will be captured (i.e. if
  * I set the log level to `INFO`, errors will still be logged, but `DEBUG` and
- * `VERBOSE` logs will not)
+ * `LOG` logs will not)
  */
 export enum LogLevel {
   DEBUG,
-  VERBOSE,
+  LOG,
   INFO,
   WARN,
   ERROR,
@@ -49,71 +42,67 @@ export enum LogLevel {
 /**
  * A container for the default log level
  */
-let defaultLogLevel: LogLevel = LogLevel.WARN;
-
-/**
- * A function to set the default log level externally
- */
-export function setDefaultLogLevel(val: LogLevel) {
-  if (!(val in LogLevel)) {
-    throw new TypeError('Attempted to Invalid value assigned to `logLevel`');
-  }
-  defaultLogLevel = val;
-}
+const defaultLogLevel: LogLevel = LogLevel.WARN;
 
 /**
  * We allow users the ability to pass their own log handler. We will pass the
  * type of log, the current log level, and any other arguments passed (i.e. the
  * messages that the user wants to log) to this function.
  */
 export type LogHandler = (
+  loggerInstance: Logger,
   logType: LogLevel,
-  currentLogLevel: LogLevel,
   ...args: any[]
 ) => void;
 
 /**
- * The default log handler will forward DEBUG, VERBOSE, INFO, WARN, and ERROR
+ * The default log handler will forward DEBUG, LOG, INFO, WARN, and ERROR
  * messages on to their corresponding console counterparts (if the log method
  * is supported by the current log level)
  */
 const defaultLogHandler: LogHandler = (
-  logType: LogLevel,
-  currentLevel: LogLevel,
-  ...args: any[]
+  instance,
+  logType,
+  ...args
 ) => {
-  if (logType < currentLevel) return;
+  if (logType < instance.logLevel) return;
+  const now = new Date();
   switch (logType) {
     case LogLevel.SILENT:
       return;
-    case LogLevel.VERBOSE:
-      console.log(...args);
+    case LogLevel.LOG:
+      console.log(`[${now}]  ${instance.name}:`, ...args);
       break;
     case LogLevel.INFO:
-      console.info(...args);
+      console.info(`[${now}]  ${instance.name}:`, ...args);
       break;
     case LogLevel.WARN:
-      console.warn(...args);
+      console.warn(`[${now}]  ${instance.name}:`, ...args);
       break;
     case LogLevel.ERROR:
-      console.error(...args);
+      console.error(`[${now}]  ${instance.name}:`, ...args);
       break;
     default:
-      console.debug(...args);
+      console.log(`[${now}]  ${instance.name}:`, ...args);
   }
 };
 
 export class Logger {
-  constructor() {
+  /**
+   * Gives you an instance of a Logger to capture messages according to
+   * Firebase's logging scheme.
+   * 
+   * @param name The name that the logs will be associated with
+   */
+  constructor(public name: string) {
     /**
      * Capture the current instance for later use
      */
     instances.push(this);
   }
 
   /**
-   * The log level of the given logger. Though all of the log levels can be
-   * centrally set, each logger can be set individually if it desires.
+   * The log level of the given Logger instance.
    */
   private _logLevel = defaultLogLevel;
   get logLevel() {
@@ -127,9 +116,7 @@ export class Logger {
   }
 
   /**
-   * The log handler for the current logger instance. This can be set to any
-   * function value, though this should not be needed the vast majority of the
-   * time
+   * The log handler for the Logger instance.
    */
   private _logHandler: LogHandler = defaultLogHandler;
   get logHandler() {
@@ -147,18 +134,18 @@ export class Logger {
    */
 
   debug(...args) {
-    this._logHandler(LogLevel.DEBUG, this._logLevel, ...args);
+    this._logHandler(this, LogLevel.DEBUG, ...args);
   }
   log(...args) {
-    this._logHandler(LogLevel.VERBOSE, this._logLevel, ...args);
+    this._logHandler(this, LogLevel.LOG, ...args);
   }
   info(...args) {
-    this._logHandler(LogLevel.INFO, this._logLevel, ...args);
+    this._logHandler(this, LogLevel.INFO, ...args);
   }
   warn(...args) {
-    this._logHandler(LogLevel.WARN, this._logLevel, ...args);
+    this._logHandler(this, LogLevel.WARN, ...args);
   }
   error(...args) {
-    this._logHandler(LogLevel.ERROR, this._logLevel, ...args);
+    this._logHandler(this, LogLevel.ERROR, ...args);
   }
 }

packages/logger/test/logger.test.ts

@@ -31,31 +31,31 @@ describe('@firebase/logger', () => {
     errorSpy: null
   };
   /**
-   * Before each test, instantiate a new instance of Logger and set the log
-   * level so that all
-   *
-   * Also spy on all of the console methods so we can assert against them as
-   * needed
+   * Before each test, instantiate a new instance of Logger and establish spies
+   * on all of the console methods so we can assert against them as needed
    */
   beforeEach(() => {
-    client = new Logger();
+    client = new Logger('@firebase/test-logger');
 
-    spies.debugSpy = Spy(console, 'debug');
     spies.logSpy = Spy(console, 'log');
     spies.infoSpy = Spy(console, 'info');
     spies.warnSpy = Spy(console, 'warn');
     spies.errorSpy = Spy(console, 'error');
   });
 
   afterEach(() => {
-    spies.debugSpy.restore();
     spies.logSpy.restore();
     spies.infoSpy.restore();
     spies.warnSpy.restore();
     spies.errorSpy.restore();
   });
 
   function testLog(message, channel, shouldLog) {
+    /**
+     * `debug` logs also go through log channel
+     */
+    channel = channel === 'debug' ? 'log' : channel;
+
     it(`Should ${shouldLog ? '' : 'not'} call \`console.${channel}\` if \`.${
       channel
     }\` is called`, () => {
@@ -71,7 +71,7 @@ describe('@firebase/logger', () => {
     /**
      * Allow all logs to be exported for this block of tests
      */
-    before(() => {
+    beforeEach(() => {
       setLogLevel(LogLevel.DEBUG);
     });
     testLog(message, 'debug', true);
@@ -82,7 +82,7 @@ describe('@firebase/logger', () => {
   });
 
   describe('Defaults', () => {
-    before(() => {
+    beforeEach(() => {
       setLogLevel(LogLevel.WARN);
     });
     testLog(message, 'debug', false);