Address @mikelehen feedback

Author: jshcrowthe

packages/firestore/src/util/log.ts

@@ -41,28 +41,37 @@ export function getLogLevel(): LogLevel {
 }
 export function setLogLevel(newLevel: LogLevel): void {
   /**
-   * Since Firestore's LogLevel enum is a pure subset of Firebase's LogLevel, we
-   * can return it directly.
+   * Map the new log level to the associated Firebase Log Level
    */
-  logClient.logLevel = newLevel;
+  switch (newLevel) {
+    case LogLevel.DEBUG:
+      logClient.logLevel = FirebaseLogLevel.DEBUG;
+      break;
+    case LogLevel.ERROR:
+      logClient.logLevel = FirebaseLogLevel.ERROR;
+      break;
+    case LogLevel.SILENT:
+      logClient.logLevel = FirebaseLogLevel.SILENT;
+      break;
+    default:
+      logClient.error(`Firestore (${SDK_VERSION}): Invalid value passed to \`setLogLevel\``);
+  }
 }
 
 export function debug(tag: string, msg: string, ...obj: AnyJs[]): void {
   if (logClient.logLevel <= FirebaseLogLevel.DEBUG) {
-    const time = new Date().toISOString();
     const args = obj.map(argToString);
     logClient.log(
-      `Firestore (${SDK_VERSION}) ${time} [${tag}]: ${msg}`,
+      `Firestore (${SDK_VERSION}) [${tag}]: ${msg}`,
       ...args
     );
   }
 }
 
 export function error(msg: string, ...obj: AnyJs[]): void {
   if (logClient.logLevel <= FirebaseLogLevel.ERROR) {
-    const time = new Date().toISOString();
     const args = obj.map(argToString);
-    logClient.error(`Firestore (${SDK_VERSION}) ${time}: ${msg}`, ...args);
+    logClient.error(`Firestore (${SDK_VERSION}): ${msg}`, ...args);
   }
 }
 

packages/logger/README.md

@@ -34,6 +34,6 @@ instance:
 Each log will be formatted in the following manner:
 
 ```typescript
-`[${new Date()}]  ${this.name}: ${...args}`
+`[${new Date()}]  ${COMPONENT_NAME}: ${...args}`
 ```
 

packages/logger/index.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
- */
-
 import { instances, LogLevel } from './src/logger';
 
 export function setLogLevel(level: LogLevel) {

packages/logger/src/logger.ts

@@ -40,7 +40,7 @@ export enum LogLevel {
 }
 
 /**
- * A container for the default log level
+ * The sefault log level
  */
 const defaultLogLevel: LogLevel = LogLevel.WARN;
 
@@ -62,10 +62,25 @@ export type LogHandler = (
  */
 const defaultLogHandler: LogHandler = (instance, logType, ...args) => {
   if (logType < instance.logLevel) return;
-  const now = new Date();
+  const now = new Date().toISOString();
   switch (logType) {
+    /**
+     * The default log handler doesn't do anything with LogLevel silent, so we
+     * are early returning here. To allow custom log handlers to handle this
+     * behavior differently, we are still going to pass through logging calls
+     * to their handlers when the log level is set to `SILENT`.
+     */
     case LogLevel.SILENT:
       return;
+    /**
+     * By default, `console.debug` is not displayed in the developer console (in
+     * chrome). To avoid forcing users to have to opt-in to these logs twice
+     * (i.e. once for firebase, and once in the console), we are sending `DEBUG`
+     * logs to the `console.log` function.
+     */
+    case LogLevel.DEBUG:
+      console.log(`[${now}]  ${instance.name}:`, ...args);
+      break;
     case LogLevel.LOG:
       console.log(`[${now}]  ${instance.name}:`, ...args);
       break;
@@ -79,7 +94,7 @@ const defaultLogHandler: LogHandler = (instance, logType, ...args) => {
       console.error(`[${now}]  ${instance.name}:`, ...args);
       break;
     default:
-      console.log(`[${now}]  ${instance.name}:`, ...args);
+      throw new Error(`Attempted to log a message with an invalid logType (value: ${logType})`);
   }
 };
 

packages/logger/test/logger.test.ts

@@ -24,7 +24,6 @@ describe('@firebase/logger', () => {
   const message = 'Hello there!';
   let client: Logger;
   const spies = {
-    debugSpy: null,
     logSpy: null,
     infoSpy: null,
     warnSpy: null,
@@ -52,7 +51,8 @@ describe('@firebase/logger', () => {
 
   function testLog(message, channel, shouldLog) {
     /**
-     * `debug` logs also go through log channel
+     * Ensure that `debug` logs assert against the `console.log` function. The
+     * rationale here is explained in `logger.ts`.
      */
     channel = channel === 'debug' ? 'log' : channel;
 
@@ -81,10 +81,7 @@ describe('@firebase/logger', () => {
     testLog(message, 'error', true);
   });
 
-  describe('Defaults', () => {
-    beforeEach(() => {
-      setLogLevel(LogLevel.WARN);
-    });
+  describe('Defaults to LogLevel.WARN', () => {
     testLog(message, 'debug', false);
     testLog(message, 'log', false);
     testLog(message, 'info', false);

packages/template/index.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
- */
-
 import { testFxn } from './src';
 
 testFxn();