diff --git a/build-system/tasks/presubmit-checks.js b/build-system/tasks/presubmit-checks.js index 6ab98275a05e..20d4295eba82 100644 --- a/build-system/tasks/presubmit-checks.js +++ b/build-system/tasks/presubmit-checks.js @@ -144,8 +144,8 @@ const forbiddenTerms = { message: 'This is only available in vendor config for ' + 'temporary workarounds.', whitelist: [ - 'extensions/amp-analytics/0.1/amp-analytics.js', 'extensions/amp-analytics/0.1/config.js', + 'extensions/amp-analytics/0.1/requests.js', ], }, // Service factories that should only be installed once. diff --git a/extensions/amp-analytics/0.1/amp-analytics.js b/extensions/amp-analytics/0.1/amp-analytics.js index d55a17f6df7e..e7b594f58903 100644 --- a/extensions/amp-analytics/0.1/amp-analytics.js +++ b/extensions/amp-analytics/0.1/amp-analytics.js @@ -95,6 +95,7 @@ export class AmpAnalytics extends AMP.BaseElement { /** @private {?Promise} */ this.iniPromise_ = null; + /** @private {./transport.Transport} */ this.transport_ = null; /** @private {boolean} */ @@ -451,7 +452,7 @@ export class AmpAnalytics extends AMP.BaseElement { const request = this.config_['requests'][k]; requests[k] = new RequestHandler( this.element, request, this.preconnect, - this.sendRequest_.bind(this), + this.transport_, this.isSandbox_); } } @@ -671,26 +672,6 @@ export class AmpAnalytics extends AMP.BaseElement { this.element).expandUrlAsync(key)); } - /** - * @param {string} request The full request string to send. - * @param {!JsonObject} trigger - * @private - */ - sendRequest_(request, trigger) { - if (!request) { - const TAG = this.getName_(); - this.user().error(TAG, 'Request not sent. Contents empty.'); - return; - } - if (trigger['iframePing']) { - user().assert(trigger['on'] == 'visible', - 'iframePing is only available on page view requests.'); - this.transport_.sendRequestUsingIframe(request); - } else { - this.transport_.sendRequest(request); - } - } - /** * @return {string} Returns a string to identify this tag. May not be unique * if the element id is not unique. diff --git a/extensions/amp-analytics/0.1/requests.js b/extensions/amp-analytics/0.1/requests.js index ed8df2f76de2..341ae485bcf1 100644 --- a/extensions/amp-analytics/0.1/requests.js +++ b/extensions/amp-analytics/0.1/requests.js @@ -30,7 +30,7 @@ import {dict, map} from '../../../src/utils/object'; import {filterSplice} from '../../../src/utils/array'; import {isArray, isFiniteNumber} from '../../../src/types'; -const TAG = 'AMP-ANALYTICS'; +const TAG = 'AMP-ANALYTICS/requests'; const BATCH_INTERVAL_MIN = 200; @@ -39,10 +39,10 @@ export class RequestHandler { * @param {!Element} ampAnalyticsElement * @param {!JsonObject} request * @param {!../../../src/preconnect.Preconnect} preconnect - * @param {function(string, !JsonObject)} handler + * @param {./transport.Transport} transport * @param {boolean} isSandbox */ - constructor(ampAnalyticsElement, request, preconnect, handler, isSandbox) { + constructor(ampAnalyticsElement, request, preconnect, transport, isSandbox) { /** @const {!Window} */ this.win = ampAnalyticsElement.getAmpDoc().win; @@ -93,8 +93,8 @@ export class RequestHandler { /** @private {!../../../src/preconnect.Preconnect} */ this.preconnect_ = preconnect; - /** @private {function(string, !JsonObject)} */ - this.handler_ = handler; + /** @private {./transport.Transport} */ + this.transport_ = transport; /** @const @private {!Object|undefined} */ this.whiteList_ = isSandbox ? SANDBOX_AVAILABLE_VARS : undefined; @@ -230,7 +230,7 @@ export class RequestHandler { baseUrlPromise_: baseUrlPromise, batchSegmentPromises_: batchSegmentsPromise, } = this; - const lastTrigger = /** @type {!JsonObject} */ (this.lastTrigger_); + const trigger = /** @type {!JsonObject} */ (this.lastTrigger_); this.reset_(); baseUrlTemplatePromise.then(preUrl => { @@ -244,8 +244,18 @@ export class RequestHandler { requestUrlPromise = constructExtraUrlParamStrs(baseUrl, extraUrlParamsPromise); } - requestUrlPromise.then(requestUrl => { - this.handler_(requestUrl, lastTrigger); + requestUrlPromise.then(request => { + if (!request) { + user().error(TAG, 'Request not sent. Contents empty.'); + return; + } + if (trigger['iframePing']) { + user().assert(trigger['on'] == 'visible', + 'iframePing is only available on page view requests.'); + this.transport_.sendRequestUsingIframe(request); + } else { + this.transport_.sendRequest(request); + } }); }); }); diff --git a/extensions/amp-analytics/0.1/test/test-amp-analytics.js b/extensions/amp-analytics/0.1/test/test-amp-analytics.js index 98acda7c6867..b1d3a0025779 100644 --- a/extensions/amp-analytics/0.1/test/test-amp-analytics.js +++ b/extensions/amp-analytics/0.1/test/test-amp-analytics.js @@ -154,7 +154,7 @@ describes.realWin('amp-analytics', { const analytics = new AmpAnalytics(el); analytics.createdCallback(); analytics.buildCallback(); - sendRequestSpy = sandbox.stub(analytics, 'sendRequest_'); + sendRequestSpy = sandbox.stub(Transport.prototype, 'sendRequest'); postMessageSpy = sandbox.spy(analytics.win.parent, 'postMessage'); return analytics; } @@ -385,7 +385,7 @@ describes.realWin('amp-analytics', { analytics.buildCallback(); // Initialization has not started. expect(analytics.iniPromise_).to.be.null; - sendRequestSpy = sandbox.spy(analytics, 'sendRequest_'); + sendRequestSpy = sandbox.spy(Transport.prototype, 'sendRequest'); return waitForNoSendRequest(analytics).then(() => { expect(sendRequestSpy).to.have.not.been.called; @@ -441,7 +441,7 @@ describes.realWin('amp-analytics', { el.connectedCallback(); analytics.createdCallback(); analytics.buildCallback(); - sendRequestSpy = sandbox.spy(analytics, 'sendRequest_'); + sendRequestSpy = sandbox.spy(Transport.prototype, 'sendRequest'); return waitForNoSendRequest(analytics).then(() => { expect(sendRequestSpy).to.have.not.been.called; @@ -462,7 +462,7 @@ describes.realWin('amp-analytics', { el.connectedCallback(); analytics.createdCallback(); analytics.buildCallback(); - sendRequestSpy = sandbox.spy(analytics, 'sendRequest_'); + sendRequestSpy = sandbox.spy(Transport.prototype, 'sendRequest'); return waitForNoSendRequest(analytics).then(() => { expect(sendRequestSpy).to.have.not.been.called; diff --git a/extensions/amp-analytics/0.1/test/test-requests.js b/extensions/amp-analytics/0.1/test/test-requests.js index 90be3e6db031..62d923a23ec0 100644 --- a/extensions/amp-analytics/0.1/test/test-requests.js +++ b/extensions/amp-analytics/0.1/test/test-requests.js @@ -54,7 +54,7 @@ describes.realWin('Requests', {amp: 1}, env => { const spy = sandbox.spy(); const r = {'baseUrl': 'r2', 'batchInterval': 1}; const handler = new RequestHandler( - analyticsMock, r, preconnect, spy, false); + analyticsMock, r, preconnect, {sendRequest: spy}, false); const expansionOptions = new ExpansionOptions({}); handler.send({}, {}, expansionOptions, {}); handler.send({}, {}, expansionOptions, {}); @@ -69,7 +69,7 @@ describes.realWin('Requests', {amp: 1}, env => { const spy = sandbox.spy(); const r = {'baseUrl': 'r1'}; const handler = new RequestHandler( - analyticsMock, r, preconnect, spy, false); + analyticsMock, r, preconnect, {sendRequest: spy}, false); const expansionOptions = new ExpansionOptions({}); handler.send({}, {}, expansionOptions, {}); handler.send({}, {}, expansionOptions, {}); @@ -81,7 +81,7 @@ describes.realWin('Requests', {amp: 1}, env => { it('should preconnect', function* () { const r = {'baseUrl': 'r2?cid=CLIENT_ID(scope)&var=${test}'}; const handler = new RequestHandler( - analyticsMock, r, preconnect, sandbox.spy(), false); + analyticsMock, r, preconnect, {sendRequest: sandbox.spy()}, false); const expansionOptions = new ExpansionOptions({'test': 'expanded'}); handler.send({}, {}, expansionOptions, {}); yield macroTask(); @@ -92,14 +92,16 @@ describes.realWin('Requests', {amp: 1}, env => { describe('batch with batchInterval', () => { let spy; + let transport; beforeEach(() => { spy = sandbox.spy(); + transport = {sendRequest: spy}; }); it('should support number', () => { const r = {'baseUrl': 'r1', 'batchInterval': 5}; const handler = new RequestHandler( - analyticsMock, r, preconnect, spy, false); + analyticsMock, r, preconnect, transport, false); expect(handler.batchIntervalPointer_).to.not.be.null; expect(handler.batchInterval_).to.deep.equal([5000]); }); @@ -107,7 +109,7 @@ describes.realWin('Requests', {amp: 1}, env => { it('should support array', () => { const r = {'baseUrl': 'r1', 'batchInterval': [1, 2, 3]}; const handler = new RequestHandler( - analyticsMock, r, preconnect, spy, false); + analyticsMock, r, preconnect, transport, false); expect(handler.batchIntervalPointer_).to.not.be.null; expect(handler.batchInterval_).to.deep.equal([1000, 2000, 3000]); }); @@ -117,13 +119,13 @@ describes.realWin('Requests', {amp: 1}, env => { const r1 = {'baseUrl': 'r', 'batchInterval': 'invalid'}; const r2 = {'baseUrl': 'r', 'batchInterval': ['invalid']}; try { - new RequestHandler(analyticsMock, r1, preconnect, spy, false); + new RequestHandler(analyticsMock, r1, preconnect, transport, false); throw new Error('should never happen'); } catch (e) { expect(e).to.match(/Invalid batchInterval value/); } try { - new RequestHandler(analyticsMock, r2, preconnect, spy, false); + new RequestHandler(analyticsMock, r2, preconnect, transport, false); throw new Error('should never happen'); } catch (e) { expect(e).to.match(/Invalid batchInterval value/); @@ -134,19 +136,19 @@ describes.realWin('Requests', {amp: 1}, env => { const r4 = {'baseUrl': 'r', 'batchInterval': [-1, 5]}; const r5 = {'baseUrl': 'r', 'batchInterval': [1, 0.01]}; try { - new RequestHandler(analyticsMock, r3, preconnect, spy, false); + new RequestHandler(analyticsMock, r3, preconnect, transport, false); throw new Error('should never happen'); } catch (e) { expect(e).to.match(/Invalid batchInterval value/); } try { - new RequestHandler(analyticsMock, r4, preconnect, spy, false); + new RequestHandler(analyticsMock, r4, preconnect, transport, false); throw new Error('should never happen'); } catch (e) { expect(e).to.match(/Invalid batchInterval value/); } try { - new RequestHandler(analyticsMock, r5, preconnect, spy, false); + new RequestHandler(analyticsMock, r5, preconnect, transport, false); throw new Error('should never happen'); } catch (e) { expect(e).to.match(/Invalid batchInterval value/); @@ -156,7 +158,7 @@ describes.realWin('Requests', {amp: 1}, env => { it('should schedule send request with interval array', function* () { const r = {'baseUrl': 'r', 'batchInterval': [1, 2]}; const handler = new RequestHandler( - analyticsMock, r, preconnect, spy, false); + analyticsMock, r, preconnect, transport, false); const expansionOptions = new ExpansionOptions({}); clock.tick(998); handler.send({}, {}, expansionOptions, {}); @@ -184,7 +186,7 @@ describes.realWin('Requests', {amp: 1}, env => { it('should not schedule send request w/o trigger', function* () { const r = {'baseUrl': 'r', 'batchInterval': [1]}; - new RequestHandler(analyticsMock, r, preconnect, spy, false); + new RequestHandler(analyticsMock, r, preconnect, transport, false); clock.tick(1000); yield macroTask(); expect(spy).to.not.be.called; @@ -193,7 +195,7 @@ describes.realWin('Requests', {amp: 1}, env => { it('should schedule send independent of trigger immediate', function* () { const r = {'baseUrl': 'r', 'batchInterval': [1, 2]}; const handler = new RequestHandler( - analyticsMock, r, preconnect, spy, false); + analyticsMock, r, preconnect, transport, false); const expansionOptions = new ExpansionOptions({}); handler.send({}, {}, expansionOptions, {}); clock.tick(999); @@ -210,20 +212,22 @@ describes.realWin('Requests', {amp: 1}, env => { describe('reportWindow', () => { let spy; + let transport; beforeEach(() => { spy = sandbox.spy(); + transport = {sendRequest: spy}; }); it('should accept reportWindow with number', () => { const r = {'baseUrl': 'r', 'reportWindow': 1}; const handler = new RequestHandler( - analyticsMock, r, preconnect, spy, false); + analyticsMock, r, preconnect, transport, false); const r2 = {'baseUrl': 'r', 'reportWindow': '2'}; const handler2 = new RequestHandler( - analyticsMock, r2, preconnect, spy, false); + analyticsMock, r2, preconnect, transport, false); const r3 = {'baseUrl': 'r', 'reportWindow': 'invalid'}; const handler3 = new RequestHandler( - analyticsMock, r3, preconnect, spy, false); + analyticsMock, r3, preconnect, transport, false); expect(handler.reportWindow_).to.equal(1); expect(handler2.reportWindow_).to.equal(2); expect(handler3.reportWindow_).to.be.null; @@ -232,7 +236,7 @@ describes.realWin('Requests', {amp: 1}, env => { it('should stop bathInterval outside batch report window', function* () { const r = {'baseUrl': 'r', 'batchInterval': 0.5, 'reportWindow': 1}; const handler = new RequestHandler( - analyticsMock, r, preconnect, spy, false); + analyticsMock, r, preconnect, transport, false); const expansionOptions = new ExpansionOptions({}); handler.send({}, {}, expansionOptions, {}); clock.tick(500); @@ -250,7 +254,7 @@ describes.realWin('Requests', {amp: 1}, env => { it('should stop send request outside batch report window', function* () { const r = {'baseUrl': 'r', 'reportWindow': 1}; const handler = new RequestHandler( - analyticsMock, r, preconnect, spy, false); + analyticsMock, r, preconnect, transport, false); const expansionOptions = new ExpansionOptions({}); handler.send({}, {}, expansionOptions, {}); yield macroTask(); @@ -265,7 +269,7 @@ describes.realWin('Requests', {amp: 1}, env => { it('should flush batch queue after batch report window', function* () { const r = {'baseUrl': 'r', 'batchInterval': 5, 'reportWindow': 1}; const handler = new RequestHandler( - analyticsMock, r, preconnect, spy, false); + analyticsMock, r, preconnect, transport, false); const expansionOptions = new ExpansionOptions({}); handler.send({}, {}, expansionOptions, {}); clock.tick(1000); @@ -276,7 +280,7 @@ describes.realWin('Requests', {amp: 1}, env => { it('should respect immediate trigger', function* () { const r = {'baseUrl': 'r', 'batchInterval': 0.2, 'reportWindow': 0.5}; const handler = new RequestHandler( - analyticsMock, r, preconnect, spy, false); + analyticsMock, r, preconnect, transport, false); const expansionOptions = new ExpansionOptions({}); clock.tick(500); yield macroTask(); @@ -292,21 +296,20 @@ describes.realWin('Requests', {amp: 1}, env => { const spy = sandbox.spy(); const r = {'baseUrl': 'r1', 'batchInterval': 1}; const handler = new RequestHandler( - analyticsMock, r, preconnect, spy, false); + analyticsMock, r, preconnect, {sendRequest: spy}, false); const expansionOptions = new ExpansionOptions({}); handler.send({'e1': 'e1'}, {}, expansionOptions, {}); handler.send({'e1': 'e1'}, {}, expansionOptions, {}); clock.tick(1000); yield macroTask(); - expect(spy).to.be.calledOnce; - expect(spy.args[0][0]).to.equal('r1?e1=e1&e1=e1'); + expect(spy).to.be.calledWith('r1?e1=e1&e1=e1'); }); it('should respect trigger extraUrlParam', function* () { const spy = sandbox.spy(); const r = {'baseUrl': 'r1', 'batchInterval': 1}; const handler = new RequestHandler( - analyticsMock, r, preconnect, spy, false); + analyticsMock, r, preconnect, {sendRequest: spy}, false); const expansionOptions = new ExpansionOptions({'v2': 'δΈ­'}); handler.send({}, { 'extraUrlParams': { @@ -318,15 +321,14 @@ describes.realWin('Requests', {amp: 1}, env => { {}, {'extraUrlParams': {'e1': 'e1'}}, expansionOptions, {}); clock.tick(1000); yield macroTask(); - expect(spy).to.be.calledOnce; - expect(spy.args[0][0]).to.equal('r1?e1=e1&e2=%E4%B8%AD&e1=e1'); + expect(spy).to.be.calledWith('r1?e1=e1&e2=%E4%B8%AD&e1=e1'); }); it('should replace extraUrlParam', function* () { const spy = sandbox.spy(); const r = {'baseUrl': 'r1&${extraUrlParams}&r2', 'batchInterval': 1}; const handler = new RequestHandler( - analyticsMock, r, preconnect, spy, false); + analyticsMock, r, preconnect, {sendRequest: spy}, false); const expansionOptions = new ExpansionOptions({}); handler.send( {}, {'extraUrlParams': {'e1': 'e1'}}, expansionOptions, {}); @@ -334,8 +336,7 @@ describes.realWin('Requests', {amp: 1}, env => { {}, {'extraUrlParams': {'e2': 'e2'}}, expansionOptions, {}); clock.tick(1000); yield macroTask(); - expect(spy).to.be.calledOnce; - expect(spy.args[0][0]).to.equal('r1&e1=e1&e2=e2&r2'); + expect(spy).to.be.calledWith('r1&e1=e1&e2=e2&r2'); }); }); @@ -344,7 +345,8 @@ describes.realWin('Requests', {amp: 1}, env => { const spy = sandbox.spy(); const r = {'baseUrl': 'r', 'batchPlugin': '_ping_'}; try { - new RequestHandler(analyticsMock, r, preconnect, spy, false); + new RequestHandler( + analyticsMock, r, preconnect, {sendRequest: spy}, false); } catch (e) { expect(e).to.match( /batchPlugin cannot be set on non-batched request/); @@ -356,7 +358,8 @@ describes.realWin('Requests', {amp: 1}, env => { const r = {'baseUrl': 'r', 'batchInterval': 1, 'batchPlugin': 'invalid'}; try { - new RequestHandler(analyticsMock, r, preconnect, spy, false); + new RequestHandler( + analyticsMock, r, preconnect, {sendRequest: spy}, false); } catch (e) { expect(e).to.match(/unsupported batch plugin/); } @@ -366,7 +369,7 @@ describes.realWin('Requests', {amp: 1}, env => { const spy = sandbox.spy(); const r = {'baseUrl': 'r', 'batchInterval': 1, 'batchPlugin': '_ping_'}; const handler = new RequestHandler( - analyticsMock, r, preconnect, spy, false); + analyticsMock, r, preconnect, {sendRequest: spy}, false); // Overwrite batchPlugin function handler.batchingPlugin_ = () => {throw new Error('test');}; expectAsyncConsoleError(/test/); @@ -374,15 +377,14 @@ describes.realWin('Requests', {amp: 1}, env => { handler.send({}, {'extraUrlParams': {'e1': 'e1'}}, expansionOptions); clock.tick(1000); yield macroTask(); - expect(spy).to.be.calledOnce; - expect(spy.args[0][0]).to.equal(''); + expect(spy).to.be.not.called; }); it('should pass in correct batchSegments', function* () { const spy = sandbox.spy(); const r = {'baseUrl': 'r', 'batchInterval': 1, 'batchPlugin': '_ping_'}; const handler = new RequestHandler( - analyticsMock, r, preconnect, spy, false); + analyticsMock, r, preconnect, {sendRequest: spy}, false); // Overwrite batchPlugin function const batchPluginSpy = sandbox.spy(handler, 'batchingPlugin_'); const expansionOptions = new ExpansionOptions({}); @@ -427,7 +429,7 @@ describes.realWin('Requests', {amp: 1}, env => { const spy = sandbox.spy(); const r = {'baseUrl': 'r1&${extraUrlParams}&BASE_VALUE'}; const handler = new RequestHandler( - analyticsMock, r, preconnect, spy, false); + analyticsMock, r, preconnect, {sendRequest: spy}, false); const expansionOptions = new ExpansionOptions({ 'param1': 'PARAM_1', 'param2': 'PARAM_2', @@ -448,8 +450,7 @@ describes.realWin('Requests', {amp: 1}, env => { }; handler.send({}, params, expansionOptions, bindings); yield macroTask(); - expect(spy).to.be.calledOnce; - expect(spy.args[0][0]).to.equal( + expect(spy).to.be.calledWith( 'r1&key1=val1&key2=val2&key3=val3&val_base'); }); @@ -461,7 +462,7 @@ describes.realWin('Requests', {amp: 1}, env => { 'baseUrl': 'r1&${extraUrlParams}&BASE_VALUE&foo=${foo}', }; const handler = new RequestHandler( - analyticsMock, r, preconnect, spy, false); + analyticsMock, r, preconnect, {sendRequest: spy}, false); const expansionOptions = new ExpansionOptions({ 'param1': 'PARAM_1', 'param2': 'PARAM_2', @@ -484,8 +485,7 @@ describes.realWin('Requests', {amp: 1}, env => { }; handler.send({}, params, expansionOptions, bindings); yield macroTask(); - expect(spy).to.be.calledOnce; - expect(spy.args[0][0]).to.equal( + expect(spy).to.be.calledWith( 'r1&key1=val1&key2=val2&key3=val3&val_base&foo=ZM9V'); toggleExperiment(env.win, 'url-replacement-v2'); });