Bug 1452200 - 1b. Add template literal support to Log.jsm; r?markh draft
authorJim Chen <nchen@mozilla.com>
Sun, 15 Apr 2018 14:53:28 -0400
changeset 782397 cbdbdc5f5607401fe402f7e51103130050a8b5e7
parent 782396 86c45f30ee0fc1495a7af15f1081a1b2c7960f4b
child 782398 1c04b6c7f87c3450253a1805f87a9b1b2917d826
push id106522
push userbmo:nchen@mozilla.com
push dateSun, 15 Apr 2018 18:53:59 +0000
reviewersmarkh
bugs1452200
milestone61.0a1
Bug 1452200 - 1b. Add template literal support to Log.jsm; r?markh Make Log.jsm functions support tagged template literals. For example, instead of |logger.debug("foo " + bar)| or |logger.debug(`foo ${bar}`)|, you can now use |logger.debug `foo ${bar}`| (without parentheses). Using tagged template literals has the benefit of less verbosity compared to regular string concatenation, with the added benefit of lazily-stringified parameters -- the parameters are only stringified when logging is enabled, possibly saving from an expensive stringify operation. This patch also fixes a bug in BasicFormatter where consecutive tokens are not formatted correctly (e.g. "${a}${b}"). MozReview-Commit-ID: 9kjLvpZF5ch
toolkit/modules/Log.jsm
toolkit/modules/tests/xpcshell/test_Log.js
--- a/toolkit/modules/Log.jsm
+++ b/toolkit/modules/Log.jsm
@@ -413,54 +413,85 @@ Logger.prototype = {
     } else {
       level = this.level;
     }
 
     params.action = action;
     this.log(level, params._message, params);
   },
 
+  _unpackTemplateLiteral(string, params) {
+    if (!Array.isArray(params)) {
+      // Regular log() call.
+      return [string, params];
+    }
+
+    if (!Array.isArray(string)) {
+      // Not using template literal. However params was packed into an array by
+      // the this.[level] call, so we need to unpack it here.
+      return [string, params[0]];
+    }
+
+    // We're using template literal format (logger.warn `foo ${bar}`). Turn the
+    // template strings into one string containing "${0}"..."${n}" tokens, and
+    // feed it to the basic formatter. The formatter will treat the numbers as
+    // indices into the params array, and convert the tokens to the params.
+
+    if (!params.length) {
+      // No params; we need to set params to undefined, so the formatter
+      // doesn't try to output the params array.
+      return [string[0], undefined];
+    }
+
+    let concat = string[0];
+    for (let i = 0; i < params.length; i++) {
+      concat += `\${${i}}${string[i + 1]}`;
+    }
+    return [concat, params];
+  },
+
   log(level, string, params) {
     if (this.level > level)
       return;
 
     // Hold off on creating the message object until we actually have
     // an appender that's responsible.
     let message;
     let appenders = this.appenders;
     for (let appender of appenders) {
       if (appender.level > level) {
         continue;
       }
       if (!message) {
+        [string, params] = this._unpackTemplateLiteral(string, params);
         message = new LogMessage(this._name, level, string, params);
       }
       appender.append(message);
     }
   },
 
-  fatal(string, params) {
+  fatal(string, ...params) {
     this.log(Log.Level.Fatal, string, params);
   },
-  error(string, params) {
+  error(string, ...params) {
     this.log(Log.Level.Error, string, params);
   },
-  warn(string, params) {
+  warn(string, ...params) {
     this.log(Log.Level.Warn, string, params);
   },
-  info(string, params) {
+  info(string, ...params) {
     this.log(Log.Level.Info, string, params);
   },
-  config(string, params) {
+  config(string, ...params) {
     this.log(Log.Level.Config, string, params);
   },
-  debug(string, params) {
+  debug(string, ...params) {
     this.log(Log.Level.Debug, string, params);
   },
-  trace(string, params) {
+  trace(string, ...params) {
     this.log(Log.Level.Trace, string, params);
   }
 };
 
 /*
  * LoggerRepository
  * Implements a hierarchy of Loggers
  */
@@ -543,17 +574,26 @@ LoggerRepository.prototype = {
    *        (string) The Logger to retrieve.
    * @param prefix
    *        (string) The string to prefix each logged message with.
    */
   getLoggerWithMessagePrefix(name, prefix) {
     let log = this.getLogger(name);
 
     let proxy = Object.create(log);
-    proxy.log = (level, string, params) => log.log(level, prefix + string, params);
+    proxy.log = (level, string, params) => {
+      if (Array.isArray(string) && Array.isArray(params)) {
+        // Template literal.
+        // We cannot change the original array, so create a new one.
+        string = [prefix + string[0]].concat(string.slice(1));
+      } else {
+        string = prefix + string; // Regular string.
+      }
+      return log.log(level, string, params);
+    };
     return proxy;
   },
 };
 
 /*
  * Formatters
  * These massage a LogMessage into whatever output is desired.
  * BasicFormatter and StructuredFormatter are implemented here.
@@ -592,17 +632,17 @@ BasicFormatter.prototype = {
     // We could add a special case for NSRESULT values here...
     let pIsObject = (typeof(params) == "object" || typeof(params) == "function");
 
     // if we have params, try and find substitutions.
     if (this.parameterFormatter) {
       // have we successfully substituted any parameters into the message?
       // in the log message
       let subDone = false;
-      let regex = /\$\{(\S*)\}/g;
+      let regex = /\$\{(\S*?)\}/g;
       let textParts = [];
       if (message.message) {
         textParts.push(message.message.replace(regex, (_, sub) => {
           // ${foo} means use the params['foo']
           if (sub) {
             if (pIsObject && sub in message.params) {
               subDone = true;
               return this.parameterFormatter.format(message.params[sub]);
--- a/toolkit/modules/tests/xpcshell/test_Log.js
+++ b/toolkit/modules/tests/xpcshell/test_Log.js
@@ -69,21 +69,23 @@ add_test(function test_LoggerWithMessage
   let appender = new MockAppender(new Log.MessageOnlyFormatter());
   log.addAppender(appender);
 
   let prefixed = Log.repository.getLoggerWithMessagePrefix(
     "test.logger.prefix", "prefix: ");
 
   log.warn("no prefix");
   prefixed.warn("with prefix");
+  prefixed.warn `with prefix`;
 
-  Assert.equal(appender.messages.length, 2, "2 messages were logged.");
+  Assert.equal(appender.messages.length, 3, "3 messages were logged.");
   Assert.deepEqual(appender.messages, [
     "no prefix",
     "prefix: with prefix",
+    "prefix: with prefix",
   ], "Prefix logger works.");
 
   run_next_test();
 });
 
 /*
  * A utility method for checking object equivalence.
  * Fields with a reqular expression value in expected will be tested
@@ -410,16 +412,20 @@ add_task(async function log_message_with
   Assert.equal(formatMessage("Null ${n} undefined ${u}", {n: null, u: undefined}),
                "Null null undefined undefined");
 
   // Format params with number, bool, and String type.
   Assert.equal(formatMessage("number ${n} boolean ${b} boxed Boolean ${bx} String ${s}",
                              {n: 45, b: false, bx: Boolean(true), s: String("whatevs")}),
                "number 45 boolean false boxed Boolean true String whatevs");
 
+  // Format params with consecutive tokens.
+  Assert.equal(formatMessage("${a}${b}${c}", {a: "foo", b: "bar", c: "baz"}),
+               "foobarbaz");
+
   /*
    * Check that errors get special formatting if they're formatted directly as
    * a named param or they're the only param, but not if they're a field in a
    * larger structure.
    */
   let err = Components.Exception("test exception", Cr.NS_ERROR_FAILURE);
   let str = formatMessage("Exception is ${}", err);
   Assert.ok(str.includes('Exception is [Exception... "test exception"'));
@@ -550,16 +556,37 @@ add_task(async function log_message_with
   Assert.equal(appender.messages.length, 7);
   for (let msg of appender.messages) {
     Assert.ok(msg.params === testParams);
     Assert.ok(msg.message.startsWith("Test "));
   }
 });
 
 /*
+ * Test that all the basic logger methods support tagged template literal format.
+ */
+add_task(async function log_template_literal_message() {
+  let log = Log.repository.getLogger("error.logger");
+  let appender = new MockAppender(new Log.BasicFormatter());
+  log.addAppender(appender);
+
+  log.fatal `Test ${"foo"} ${42}`;
+  log.error `Test ${"foo"} 42`;
+  log.warn `Test foo 42`;
+  log.info `Test ${"foo " + 42}`;
+  log.config `${"Test"} foo ${42}`;
+  log.debug `Test ${"f"}${"o"}${"o"} 42`;
+  log.trace `${"Test foo 42"}`;
+  Assert.equal(appender.messages.length, 7);
+  for (let msg of appender.messages) {
+    Assert.equal(msg.split("\t")[3], "Test foo 42");
+  }
+});
+
+/*
  * Check that we format JS Errors reasonably.
  * This needs to stay a generator to exercise Task.jsm's stack rewriting.
  */
 add_task(function* format_errors() {
   let pFormat = new Log.ParameterFormatter();
 
   // Test that subclasses of Error are recognized as errors.
   let err = new ReferenceError("Ref Error", "ERROR_FILE", 28);