From be874baa1c1c2ab58f543010b68c3c28ed75b530 Mon Sep 17 00:00:00 2001 From: Avi Mehta Date: Mon, 21 Nov 2016 12:29:00 -0800 Subject: [PATCH] Review feedback that I forgot to address previously. --- extensions/amp-analytics/0.1/amp-analytics.js | 2 +- .../amp-analytics/0.1/test/test-variables.js | 7 +++ extensions/amp-analytics/0.1/variables.js | 55 ++++++++----------- 3 files changed, 32 insertions(+), 32 deletions(-) diff --git a/extensions/amp-analytics/0.1/amp-analytics.js b/extensions/amp-analytics/0.1/amp-analytics.js index 4644311133efd..c2b64b1ab1d0e 100644 --- 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 index 707c5059ae234..3e833e771b73b 100644 --- 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 index 425daba5c7264..18c30ef25f48f 100644 --- 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,9 +80,7 @@ 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; @@ -90,8 +88,8 @@ export class VariableService { } /** - * @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} */ 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} filters + * @param {string} value + * @param {Array} 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} 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} */ hashFilter_(value) {