Address some PR comments

Author: hsubox76

packages/app/src/errors.ts

@@ -22,7 +22,8 @@ export const enum AppError {
   BAD_APP_NAME = 'bad-app-name',
   DUPLICATE_APP = 'duplicate-app',
   APP_DELETED = 'app-deleted',
-  INVALID_APP_ARGUMENT = 'invalid-app-argument'
+  INVALID_APP_ARGUMENT = 'invalid-app-argument',
+  INVALID_LOG_ARGUMENT = 'invalid-log-argument'
 }
 
 const ERRORS: ErrorMap<AppError> = {
@@ -34,7 +35,8 @@ const ERRORS: ErrorMap<AppError> = {
   [AppError.APP_DELETED]: "Firebase App named '{$appName}' already deleted",
   [AppError.INVALID_APP_ARGUMENT]:
     'firebase.{$appName}() takes either no argument or a ' +
-    'Firebase App instance.'
+    'Firebase App instance.',
+  [AppError.INVALID_LOG_ARGUMENT]: 'First argument to `onLog` must be null or a function.'
 };
 
 type ErrorParams = { [key in AppError]: { appName: string } };

packages/app/src/firebaseNamespaceCore.ts

@@ -34,7 +34,7 @@ import { FirebaseAppLiteImpl } from './lite/firebaseAppLite';
 import { DEFAULT_ENTRY_NAME, PLATFORM_LOG_STRING } from './constants';
 import { version } from '../../firebase/package.json';
 import { logger } from './logger';
-import { setUserLogHandler, setLogLevel } from '@firebase/logger';
+import { setUserLogHandler, setLogLevel, LogCallback, LogOptions } from '@firebase/logger';
 import { Component, ComponentType, Name } from '@firebase/component';
 
 /**
@@ -62,7 +62,14 @@ export function createFirebaseNamespaceCore(
     app,
     registerVersion,
     setLogLevel,
-    onLog: setUserLogHandler,
+    onLog: (
+      logCallback: LogCallback | null,
+      options?: LogOptions) => {
+        if (logCallback !== null && typeof logCallback !== 'function') {
+          throw ERROR_FACTORY.create(AppError.INVALID_LOG_ARGUMENT, { appName: name });
+        }
+        setUserLogHandler(logCallback, options);
+      },
     // @ts-ignore
     apps: null,
     SDK_VERSION: version,

packages/app/test/clientLogger.test.ts

@@ -78,6 +78,7 @@ describe('User Log Methods', () => {
     });
 
     it(`correctly triggers callback given to firebase.onLog()`, () => {
+      // Note: default log level is INFO.
       firebase.initializeApp({});
       registerCoreComponents(firebase);
       (firebase as _FirebaseNamespace).INTERNAL.registerComponent(
@@ -91,6 +92,8 @@ describe('User Log Methods', () => {
             logger.info('hi');
             expect(result.level).to.equal('info');
             expect(result.message).to.equal('hi');
+            expect(result.args).to.deep.equal(['hi']);
+            expect(result.type).to.equal('@firebase/logger-test');
             expect(infoSpy.called).to.be.true;
           },
           ComponentType.PUBLIC

packages/logger/src/logger.ts

@@ -105,7 +105,7 @@ const ConsoleMethod = {
  * messages on to their corresponding console counterparts (if the log method
  * is supported by the current log level)
  */
-const defaultLogHandler: LogHandler = (instance, logType, message): void => {
+const defaultLogHandler: LogHandler = (instance, logType, ...args): void => {
   if (logType < instance.logLevel) {
     return;
   }
@@ -114,7 +114,7 @@ const defaultLogHandler: LogHandler = (instance, logType, message): void => {
   if (method) {
     console[method as 'log' | 'info' | 'warn' | 'error'](
       `[${now}]  ${instance.name}:`,
-      message
+      ...args
     );
   } else {
     throw new Error(
@@ -215,11 +215,6 @@ export function setUserLogHandler(
   logCallback: LogCallback | null,
   options?: LogOptions
 ): void {
-  if (logCallback !== null && typeof logCallback !== 'function') {
-    throw new TypeError(
-      'First argument to `onLog` must be null or a function.'
-    );
-  }
   for (const instance of instances) {
     let threshhold = instance.logLevel;
     if (options && options.level) {

packages/logger/test/custom-logger.test.ts

@@ -51,6 +51,7 @@ describe(`Custom log handler`, () => {
     });
 
     it('calls custom callback with correct data for calling instance', () => {
+      // Default log level is INFO.
       client1.info('info message!');
       expect(result.message).to.equal('info message!');
       expect(result.args[0]).to.equal('info message!');
@@ -67,6 +68,7 @@ describe(`Custom log handler`, () => {
     });
 
     it('parses multiple arguments correctly', () => {
+      // Default log level is INFO.
       client1.info('info message!', ['hello'], 1, { a: 3 });
       expect(result.message).to.equal('info message! ["hello"] 1 {"a":3}');
       expect(result.args).to.deep.equal(['info message!', ['hello'], 1, { a: 3 }]);
@@ -75,7 +77,8 @@ describe(`Custom log handler`, () => {
     });
 
     it('calls custom callback when log call is above set log level', () => {
-      // Info was already tested above.
+      // Default log level is INFO.
+      // INFO was already tested above.
       client1.warn('warning message!');
       expect(result.message).to.equal('warning message!');
       expect(result.level).to.equal('warn');
@@ -87,6 +90,7 @@ describe(`Custom log handler`, () => {
     });
 
     it('does not call custom callback when log call is not above set log level', () => {
+      // Default log level is INFO.
       client1.log('message you should not see');
       expect(result).to.be.null;
       expect(spies.logSpy.called).to.be.false;