Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move sendRequest_ logic from amp-analytics.js to requests.js #18263

Merged
merged 2 commits into from Sep 21, 2018
Merged

Move sendRequest_ logic from amp-analytics.js to requests.js #18263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e911cb5
Move sendRequest_ logic from amp-analytics.js to requests.js
lannka Sep 20, 2018
8a60d64
fix presubmit
lannka Sep 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion build-system/tasks/presubmit-checks.js
View file
Open in desktop
Expand Up @@ -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.
Expand Down
23 changes: 2 additions & 21 deletions extensions/amp-analytics/0.1/amp-analytics.js
View file
Open in desktop
Expand Up @@ -95,6 +95,7 @@ export class AmpAnalytics extends AMP.BaseElement {
/** @private {?Promise} */
this.iniPromise_ = null;

/** @private {./transport.Transport} */
this.transport_ = null;

/** @private {boolean} */
Expand Down Expand Up @@ -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_);
}
}
Expand Down Expand Up @@ -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.
Expand Down
26 changes: 18 additions & 8 deletions extensions/amp-analytics/0.1/requests.js
View file
Open in desktop
Expand Up @@ -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';
zhouyx marked this conversation as resolved.
Show resolved Hide resolved

const BATCH_INTERVAL_MIN = 200;

Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 => {
Expand All @@ -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);
}
});
});
});
Expand Down
8 changes: 4 additions & 4 deletions extensions/amp-analytics/0.1/test/test-amp-analytics.js
View file
Open in desktop
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down