Review feedback that I forgot to address previously.

ampproject · Nov 21, 2016 · be874ba · be874ba
1 parent 0191721
commit be874ba
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 32 deletions.
diff --git a/extensions/amp-analytics/0.1/amp-analytics.js b/extensions/amp-analytics/0.1/amp-analytics.js
@@ -193,7 +193,7 @@ export class AmpAnalytics extends AMP.BaseElement {
             return this.variableService_.expandTemplate(
                 trigger['selector'], trigger, this.config_,
                 /* opt_event */ undefined, /* opt_iterations */ undefined,
-                /* opt_encode */ false).then(selector => {
+                /* opt_no_encode */ true).then(selector => {
                   trigger['selector'] = selector;
                   this.instrumentation_.addListener(
                       trigger, this.handleEvent_.bind(this, trigger),

diff --git a/extensions/amp-analytics/0.1/test/test-variables.js b/extensions/amp-analytics/0.1/test/test-variables.js
@@ -46,6 +46,13 @@ describe('amp-analytics.VariableService', function() {
       );
     });
 
+    it('expands nested vars (no encode)', () => {
+      return variables.expandTemplate('${1}', vars, {}, {}, undefined, true)
+        .then(actual =>
+          expect(actual).to.equal('123${4}')
+        );
+    });
+
     it('limits the recursion to n', () => {
       return variables.expandTemplate('${1}', vars, {}, {}, 3).then(actual =>
         expect(actual).to.equal('1234%25252524%2525257B1%2525257D')

diff --git a/extensions/amp-analytics/0.1/variables.js b/extensions/amp-analytics/0.1/variables.js
@@ -28,9 +28,9 @@ let VariableFilterDef;
 let SyncVariableFilterDef;
 
 /**
- * @param  {!string} str
- * @param  {Number} s
- * @param  {Number} l
+ * @param {!string} str
+ * @param {Number} s
+ * @param {Number} l
  * @return {string}
  */
 function substrFilter(str, s, l) {
@@ -44,8 +44,8 @@ function substrFilter(str, s, l) {
 }
 
 /**
- * @param  {*} value
- * @param  {*} defaultValue
+ * @param {*} value
+ * @param {*} defaultValue
  * @return {*}
  */
 function defaultFilter(value, defaultValue) {
@@ -80,18 +80,16 @@ export class VariableService {
     this.registerSync_('base64', value => btoa(String(value)));
     this.register_('hash', this.hashFilter_.bind(this));
     this.register_('if', (value, thenValue, elseValue) =>
-        Promise.resolve(Boolean(value)
-          ? thenValue
-          : elseValue));
+        Promise.resolve(value ? thenValue : elseValue));
 
     cryptoFor(this.win_).then(crypto => {
       this.crypto_ = crypto;
     });
   }
 
   /**
-   * @param  {string} name
-   * @param  {VariableFilterDef} handler
+   * @param {string} name
+   * @param {VariableFilterDef} handler
    */
   register_(name, handler) {
     dev().assert(!this.filters_[name], 'Filter "' + name
@@ -101,8 +99,8 @@ export class VariableService {
   }
 
   /**
-   * @param  {string} name
-   * @param  {SyncVariableFilterDef} handler
+   * @param {string} name
+   * @param {SyncVariableFilterDef} handler
    */
   registerSync_(name, handler) {
     this.register_(name, (...args) =>
@@ -112,9 +110,8 @@ export class VariableService {
       }));
   }
 
-
   /**
-   * @param  {string} filterStr
+   * @param {string} filterStr
    * @return {!Object<string, *>}
    */
   parseFilter_(filterStr) {
@@ -132,19 +129,19 @@ export class VariableService {
       user().error(TAG, 'Invalid filter name: ' + tokens[0]);
       return {};
     }
-    return {fn, args: tokens.splice(1)};
+    return {fn, args: tokens.slice(1)};
   }
 
   /**
-   * @param  {string} value
-   * @param  {Array<string>} filters
+   * @param {string} value
+   * @param {Array<string>} filters
    * @return {string}
    */
   applyFilters_(value, filters) {
     for (let i = 0; i < filters.length; i++) {
       const parsedFilter = this.parseFilter_(filters[i].trim());
       if (parsedFilter) {
-        parsedFilter.args.splice(0, 0, value);
+        parsedFilter.args.unshift(value);
         value = parsedFilter.fn.apply(null, parsedFilter.args);
       }
     }
@@ -157,14 +154,13 @@ export class VariableService {
    * @param {!Object=} opt_event Object with details about the event.
    * @param {number=} opt_iterations Number of recursive expansions to perform.
    *    Defaults to 2 substitutions.
-   * @param {boolean=} opt_encode Used to determine if the vars should be
-   *    encoded or not. Defaults to true.
+   * @param {boolean=} opt_noEncode Used to determine if the vars should be
+   *    encoded or not. Defaults to false.
    * @return {!Promise<!string>} The expanded string.
    */
   expandTemplate(template, trigger, config, opt_event, opt_iterations,
-      opt_encode) {
+      opt_noEncode) {
     opt_iterations = opt_iterations === undefined ? 2 : opt_iterations;
-    opt_encode = opt_encode === undefined ? true : opt_encode;
     if (opt_iterations < 0) {
       user().error(TAG, 'Maximum depth reached while expanding variables. ' +
           'Please ensure that the variables are not recursive.');
@@ -192,7 +188,7 @@ export class VariableService {
       let p;
       if (typeof raw == 'string') {
         p = this.expandTemplate(raw, trigger, config,
-            opt_event, opt_iterations - 1);
+            opt_event, opt_iterations - 1, opt_noEncode);
       } else {
         // Values can also be arrays and objects. Don't expand them.
         p = Promise.resolve(raw);
@@ -203,9 +199,9 @@ export class VariableService {
           this.applyFilters_(expandedValue, tokens.slice(1)))
         .then(finalRawValue => {
           // Then encode the value
-          const val = opt_encode
-              ? this.encodeVars(finalRawValue, name)
-              : finalRawValue;
+          const val = opt_noEncode
+              ? finalRawValue
+              : this.encodeVars(finalRawValue, name);
           return val ? val + argList : val;
         })
         .then(encodedValue => {
@@ -244,9 +240,7 @@ export class VariableService {
       return {name: '', argList: ''};
     }
     const match = key.match(/([^(]*)(\([^)]*\))?/);
-    if (!match) {
-      user().error(TAG, 'Variable with invalid format found: ' + key);
-    }
+    user().assert(match, 'Variable with invalid format found: ' + key);
     return {name: match[1], argList: match[2] || ''};
   }
 
@@ -268,9 +262,8 @@ export class VariableService {
     return encodeURIComponent(name) + argList;
   }
 
-
   /**
-   * @param  {string} value
+   * @param {string} value
    * @return {!Promise<!string>}
    */
   hashFilter_(value) {