diff --git a/build-system/tasks/presubmit-checks.js b/build-system/tasks/presubmit-checks.js index 4350db3a7153..29d384e149b9 100644 --- a/build-system/tasks/presubmit-checks.js +++ b/build-system/tasks/presubmit-checks.js @@ -29,6 +29,10 @@ var requiresReviewPrivacy = 'being privacy sensitive. Please file an issue asking for permission' + ' to use if you have not yet done so.'; +var privateServiceFactory = 'This service should only be installed in ' + + 'the whitelisted files. Other modules should use a public function ' + + 'typically called serviceNameFor.'; + // Terms that must not appear in our source files. var forbiddenTerms = { 'DO NOT SUBMIT': '', @@ -44,11 +48,24 @@ var forbiddenTerms = { 'validator/validator-in-browser.js', ] }, + // Service factories that should only be installed once. + 'installCidService': { + message: privateServiceFactory, + whitelist: [ + 'src/service/cid-impl.js', + 'extensions/amp-analytics/0.1/amp-analytics.js', + 'extensions/amp-analytics/0.1/test/test-amp-analytics.js', + 'test/functional/test-cid.js', + 'test/functional/test-url-replacements.js' + ], + }, + // Privacy sensitive 'cidFor': { message: requiresReviewPrivacy, whitelist: [ 'src/cid.js', 'src/service/cid-impl.js', + 'src/url-replacements.js', 'test/functional/test-cid.js', ], }, @@ -78,6 +95,7 @@ var forbiddenTerms = { 'src/cookies.js', 'src/experiments.js', 'test/functional/test-cookies.js', + 'test/functional/test-url-replacements.js', 'tools/experiments/experiments.js', ] }, @@ -85,6 +103,7 @@ var forbiddenTerms = { 'localStorage': { message: requiresReviewPrivacy, whitelist: [ + 'extensions/amp-analytics/0.1/test/test-amp-analytics.js', 'test/_init_tests.js', 'src/service/cid-impl.js', 'test/functional/test-cid.js', diff --git a/builtins/amp-pixel.js b/builtins/amp-pixel.js index 58edb83eb0fa..639ead8cc9a6 100644 --- a/builtins/amp-pixel.js +++ b/builtins/amp-pixel.js @@ -44,16 +44,17 @@ export function installPixel(win) { /** @override */ layoutCallback() { - let src = this.element.getAttribute('src'); - src = urlReplacementsFor(this.getWin()).expand(this.assertSource(src)); - const image = new Image(); - image.src = src; - image.width = 1; - image.height = 1; - // Make it take zero space - this.element.style.width = 0; - this.element.appendChild(image); - return Promise.resolve(); + const src = this.element.getAttribute('src'); + return urlReplacementsFor(this.getWin()).expand(this.assertSource(src)) + .then(src => { + const image = new Image(); + image.src = src; + image.width = 1; + image.height = 1; + // Make it take zero space + this.element.style.width = 0; + this.element.appendChild(image); + }); } assertSource(src) { diff --git a/extensions/amp-analytics/0.1/amp-analytics.js b/extensions/amp-analytics/0.1/amp-analytics.js index ae60b73a60a7..03ce039cf9ef 100644 --- a/extensions/amp-analytics/0.1/amp-analytics.js +++ b/extensions/amp-analytics/0.1/amp-analytics.js @@ -15,6 +15,7 @@ */ import {isExperimentOn} from '../../../src/experiments'; +import {installCidService} from '../../../src/service/cid-impl'; import {Layout} from '../../../src/layout'; import {log} from '../../../src/log'; import {loadPromise} from '../../../src/event-helper'; @@ -25,6 +26,9 @@ import {addListener} from './instrumentation'; import {ANALYTICS_CONFIG} from './vendors'; +installCidService(AMP.win); + + /** @const */ const EXPERIMENT = 'amp-analytics'; @@ -59,9 +63,9 @@ export class AmpAnalytics extends AMP.BaseElement { } /** @override */ - buildCallback() { + layoutCallback() { if (!this.isExperimentOn_()) { - return; + return Promise.resolve(); } /** @@ -85,7 +89,7 @@ export class AmpAnalytics extends AMP.BaseElement { if (this.hasOptedOut_()) { // Nothing to do when the user has opted out. log.fine(this.getName_(), 'User has opted out. No hits will be sent.'); - return; + return Promise.resolve(); } this.generateRequests_(); @@ -93,7 +97,7 @@ export class AmpAnalytics extends AMP.BaseElement { if (!Array.isArray(this.config_['triggers'])) { log.error(this.getName_(), 'No triggers were found in the config. No ' + 'analytics data will be sent.'); - return; + return Promise.resolve(); } // Trigger callback can be synchronous. Do the registration at the end. @@ -107,6 +111,7 @@ export class AmpAnalytics extends AMP.BaseElement { addListener(this.getWin(), trigger['on'], this.handleEvent_.bind(this, trigger)); } + return Promise.resolve(); } /** @@ -227,12 +232,12 @@ export class AmpAnalytics extends AMP.BaseElement { }); // For consistentcy with amp-pixel we also expand any url replacements. - request = urlReplacementsFor(this.getWin()).expand(request); - - // TODO(btownsend, #1061): Add support for sendBeacon. - if (host && request) { - this.sendRequest_('https://' + host + request); - } + urlReplacementsFor(this.getWin()).expand(request).then(request => { + // TODO(btownsend, #1061): Add support for sendBeacon. + if (host && request) { + this.sendRequest_('https://' + host + 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 fba3ea4b01ae..9882fe13248b 100644 --- a/extensions/amp-analytics/0.1/test/test-amp-analytics.js +++ b/extensions/amp-analytics/0.1/test/test-amp-analytics.js @@ -16,6 +16,8 @@ import {AmpAnalytics} from '../../../../build/all/v0/amp-analytics-0.1.max'; import {adopt} from '../../../../src/runtime'; +import {markElementScheduledForTesting} from '../../../../src/service'; +import {installCidService} from '../../../../src/service/cid-impl'; import * as sinon from 'sinon'; adopt(window); @@ -27,6 +29,7 @@ describe('amp-analytics', function() { let sendRequestSpy; beforeEach(() => { + markElementScheduledForTesting(window, 'amp-analytics'); sandbox = sinon.sandbox.create(); const WindowApi = function() {}; windowApi = new WindowApi(); @@ -37,6 +40,8 @@ describe('amp-analytics', function() { referrer: 'https://www.google.com/' }; windowApi.Object = window.Object; + markElementScheduledForTesting(windowApi, 'amp-analytics'); + installCidService(windowApi); }); afterEach(() => { @@ -72,8 +77,9 @@ describe('amp-analytics', function() { }); analytics.isExperimentOn_ = () => false; - analytics.buildCallback(); - expect(sendRequestSpy.callCount).to.equal(0); + return analytics.layoutCallback().then(() => { + expect(sendRequestSpy.callCount).to.equal(0); + }); }); it('sends a basic hit', function() { @@ -83,8 +89,10 @@ describe('amp-analytics', function() { 'triggers': [{'on': 'visible', 'request': 'foo'}] }); - analytics.buildCallback(); - expect(sendRequestSpy.withArgs('https://example.com/bar').calledOnce).to.be.true; + return analytics.layoutCallback().then(() => { + expect(sendRequestSpy.withArgs('https://example.com/bar').calledOnce) + .to.be.true; + }); }); it('does not send a hit when config is not in a script tag', function() { @@ -101,8 +109,9 @@ describe('amp-analytics', function() { analytics.createdCallback(); sendRequestSpy = sandbox.spy(analytics, 'sendRequest_'); - analytics.buildCallback(); - expect(sendRequestSpy.callCount).to.equal(0); + return analytics.layoutCallback().then(() => { + expect(sendRequestSpy.callCount).to.equal(0); + }); }); it('does not send a hit when multiple child tags exist', function() { @@ -114,8 +123,9 @@ describe('amp-analytics', function() { const script2 = document.createElement('script'); script2.setAttribute('type', 'application/json'); analytics.element.appendChild(script2); - analytics.buildCallback(); - expect(sendRequestSpy.callCount).to.equal(0); + return analytics.layoutCallback().then(() => { + expect(sendRequestSpy.callCount).to.equal(0); + }); }); it('does not send a hit when script tag does not have a type attribute', @@ -134,8 +144,9 @@ describe('amp-analytics', function() { analytics.createdCallback(); sendRequestSpy = sandbox.spy(analytics, 'sendRequest_'); - analytics.buildCallback(); - expect(sendRequestSpy.callCount).to.equal(0); + return analytics.layoutCallback().then(() => { + expect(sendRequestSpy.callCount).to.equal(0); + }); }); it('does not send a hit when host is not provided', function() { @@ -144,8 +155,9 @@ describe('amp-analytics', function() { 'triggers': [{'on': 'visible', 'request': 'foo'}] }); - analytics.buildCallback(); - expect(sendRequestSpy.callCount).to.equal(0); + return analytics.layoutCallback().then(() => { + expect(sendRequestSpy.callCount).to.equal(0); + }); }); it('does not send a hit when request is not provided', function() { @@ -155,8 +167,9 @@ describe('amp-analytics', function() { 'triggers': [{'on': 'visible'}] }); - analytics.buildCallback(); - expect(sendRequestSpy.callCount).to.equal(0); + return analytics.layoutCallback().then(() => { + expect(sendRequestSpy.callCount).to.equal(0); + }); }); it('does not send a hit when request type is not defined', function() { @@ -165,8 +178,9 @@ describe('amp-analytics', function() { 'triggers': [{'on': 'visible', 'request': 'foo'}] }); - analytics.buildCallback(); - expect(sendRequestSpy.callCount).to.equal(0); + return analytics.layoutCallback().then(() => { + expect(sendRequestSpy.callCount).to.equal(0); + }); }); it('expands nested requests', function() { @@ -176,10 +190,11 @@ describe('amp-analytics', function() { 'triggers': [{'on': 'visible', 'request': 'foo'}] }); - analytics.buildCallback(); - expect(sendRequestSpy.calledOnce).to.be.true; - expect(sendRequestSpy.args[0][0]) + return analytics.layoutCallback().then(() => { + expect(sendRequestSpy.calledOnce).to.be.true; + expect(sendRequestSpy.args[0][0]) .to.equal('https://example.com/bar&f1&baz'); + }); }); it('expands nested requests', function() { @@ -189,9 +204,10 @@ describe('amp-analytics', function() { 'triggers': [{'on': 'visible', 'request': 'foo'}] }); - analytics.buildCallback(); - expect(sendRequestSpy.calledOnce).to.be.true; - expect(sendRequestSpy.args[0][0]).to.equal('https://example.com/bar&b1'); + return analytics.layoutCallback().then(() => { + expect(sendRequestSpy.calledOnce).to.be.true; + expect(sendRequestSpy.args[0][0]).to.equal('https://example.com/bar&b1'); + }); }); it('expands recursive requests', function() { @@ -201,10 +217,11 @@ describe('amp-analytics', function() { 'triggers': [{'on': 'visible', 'request': 'foo'}] }); - analytics.buildCallback(); - expect(sendRequestSpy.calledOnce).to.be.true; - expect(sendRequestSpy.args[0][0]) - .to.equal('https://example.com/bar&/bar&/bar&&baz&baz&baz'); + return analytics.layoutCallback().then(() => { + expect(sendRequestSpy.calledOnce).to.be.true; + expect(sendRequestSpy.args[0][0]) + .to.equal('https://example.com/bar&/bar&/bar&&baz&baz&baz'); + }); }); it('fills in the platform variables', function() { @@ -214,9 +231,34 @@ describe('amp-analytics', function() { 'triggers': [{'on': 'visible', 'request': 'foo'}] }); - analytics.buildCallback(); - expect(sendRequestSpy.calledOnce).to.be.true; - expect(sendRequestSpy.args[0][0]).to.not.match(/AMPDOC_URL/); + return analytics.layoutCallback().then(() => { + expect(sendRequestSpy.calledOnce).to.be.true; + expect(sendRequestSpy.args[0][0]).to.not.match(/AMPDOC_URL/); + }); + }); + + it('fills cid for proxy host', function() { + windowApi.localStorage = { + getItem: function(name) { + return JSON.stringify({ + time: new Date().getTime(), + cid: 'base' + }); + }, + }; + windowApi.location.href = '/c/www.test.com/abc'; + const analytics = getAnalyticsTag({ + 'host': 'example.com', + 'requests': {'foo': 'cid=CLIENT_ID(analytics-abc)'}, + 'triggers': [{'on': 'visible', 'request': 'foo'}] + }); + + analytics.layoutCallback().then(() => { + expect(sendRequestSpy.calledOnce).to.be.true; + expect(sendRequestSpy.args[0][0]).to.equal( + 'https://example.comcid=uQVAtQyO978OPCNBZXWOKRDcxSORw9GQfB' + + 'x2CyJSF0MnkIPeeX9ruacSFPgQ0HSD'); + }); }); it('merges host correctly', function() { @@ -230,9 +272,10 @@ describe('amp-analytics', function() { 'host': 'example.com' } }; - analytics.buildCallback(); - expect(sendRequestSpy.calledOnce).to.be.true; - expect(sendRequestSpy.args[0][0]).to.equal('https://example.com/bar'); + return analytics.layoutCallback().then(() => { + expect(sendRequestSpy.calledOnce).to.be.true; + expect(sendRequestSpy.args[0][0]).to.equal('https://example.com/bar'); + }); }); it('merges requests correctly', function() { @@ -247,9 +290,10 @@ describe('amp-analytics', function() { 'requests': {'foo': '/bar', 'bar': 'foobar'} } }; - analytics.buildCallback(); - expect(sendRequestSpy.calledOnce).to.be.true; - expect(sendRequestSpy.args[0][0]).to.equal('https://example.com/foobar'); + return analytics.layoutCallback().then(() => { + expect(sendRequestSpy.calledOnce).to.be.true; + expect(sendRequestSpy.args[0][0]).to.equal('https://example.com/foobar'); + }); }); it('merges objects correctly', function() { @@ -259,33 +303,34 @@ describe('amp-analytics', function() { 'triggers': [{'on': 'visible', 'request': 'foo'}] }); - analytics.buildCallback(); - expect(analytics.mergeObjects_({}, {})).to.deep.equal({}); - expect(analytics.mergeObjects_({'foo': 1}, {'1': 1})) - .to.deep.equal({'foo': 1, '1': 1}); - expect(analytics.mergeObjects_({'1': 1}, {'bar': 'bar'})) - .to.deep.equal({'1': 1, 'bar': 'bar'}); - expect(analytics.mergeObjects_( - {'foo': [1, 2, 3, 4]}, - {'bar': [4, 5, 6, 7]})) - .to.deep.equal( - {'foo': [1,2, 3, 4], 'bar': [4, 5, 6, 7]}); - expect(analytics.mergeObjects_( - null, - {'foo': 'bar', 'baz': {'foobar': ['abc', 'def']}})) - .to.deep.equal({'foo': 'bar', 'baz': {'foobar': ['abc', 'def']}}); - expect(analytics.mergeObjects_( - undefined, - {'foo': 'bar', 'baz': {'foobar': ['abc', 'def']}})) - .to.deep.equal({'foo': 'bar', 'baz': {'foobar': ['abc', 'def']}}); - expect(analytics.mergeObjects_( - {'baz': 'bar', 'foobar': {'foobar': ['abc', 'def']}}, - {'foo': 'bar', 'baz': {'foobar': ['abc', 'def']}})) - .to.deep.equal({ - 'foo': 'bar', - 'baz': 'bar', - 'foobar': {'foobar': ['abc', 'def']} - }); + return analytics.layoutCallback().then(() => { + expect(analytics.mergeObjects_({}, {})).to.deep.equal({}); + expect(analytics.mergeObjects_({'foo': 1}, {'1': 1})) + .to.deep.equal({'foo': 1, '1': 1}); + expect(analytics.mergeObjects_({'1': 1}, {'bar': 'bar'})) + .to.deep.equal({'1': 1, 'bar': 'bar'}); + expect(analytics.mergeObjects_( + {'foo': [1, 2, 3, 4]}, + {'bar': [4, 5, 6, 7]})) + .to.deep.equal( + {'foo': [1,2, 3, 4], 'bar': [4, 5, 6, 7]}); + expect(analytics.mergeObjects_( + null, + {'foo': 'bar', 'baz': {'foobar': ['abc', 'def']}})) + .to.deep.equal({'foo': 'bar', 'baz': {'foobar': ['abc', 'def']}}); + expect(analytics.mergeObjects_( + undefined, + {'foo': 'bar', 'baz': {'foobar': ['abc', 'def']}})) + .to.deep.equal({'foo': 'bar', 'baz': {'foobar': ['abc', 'def']}}); + expect(analytics.mergeObjects_( + {'baz': 'bar', 'foobar': {'foobar': ['abc', 'def']}}, + {'foo': 'bar', 'baz': {'foobar': ['abc', 'def']}})) + .to.deep.equal({ + 'foo': 'bar', + 'baz': 'bar', + 'foobar': {'foobar': ['abc', 'def']} + }); + }); }); it('expands trigger vars', () => { @@ -300,10 +345,11 @@ describe('amp-analytics', function() { 'var2': 'test2' } }]}); - analytics.buildCallback(); - expect(sendRequestSpy.calledOnce).to.be.true; - expect(sendRequestSpy.args[0][0]).to.equal( - 'https://example.com/test1=x&test2=test2'); + return analytics.layoutCallback().then(() => { + expect(sendRequestSpy.calledOnce).to.be.true; + expect(sendRequestSpy.args[0][0]).to.equal( + 'https://example.com/test1=x&test2=test2'); + }); }); it('expands config vars', () => { @@ -315,10 +361,11 @@ describe('amp-analytics', function() { }, 'requests': {'pageview': '/test1=${var1}&test2=${var2}'}, 'triggers': [{'on': 'visible', 'request': 'pageview'}]}); - analytics.buildCallback(); - expect(sendRequestSpy.calledOnce).to.be.true; - expect(sendRequestSpy.args[0][0]).to.equal( - 'https://example.com/test1=x&test2=test2'); + return analytics.layoutCallback().then(() => { + expect(sendRequestSpy.calledOnce).to.be.true; + expect(sendRequestSpy.args[0][0]).to.equal( + 'https://example.com/test1=x&test2=test2'); + }); }); it('expands trigger vars with higher precedence than config vars', () => { @@ -335,10 +382,11 @@ describe('amp-analytics', function() { 'vars': { 'var1': 'trigger1' }}]}); - analytics.buildCallback(); - expect(sendRequestSpy.calledOnce).to.be.true; - expect(sendRequestSpy.args[0][0]).to.equal( - 'https://example.com/test1=trigger1&test2=config2'); + return analytics.layoutCallback().then(() => { + expect(sendRequestSpy.calledOnce).to.be.true; + expect(sendRequestSpy.args[0][0]).to.equal( + 'https://example.com/test1=trigger1&test2=config2'); + }); }); it('does not expand nested vars', () => { @@ -352,10 +400,11 @@ describe('amp-analytics', function() { 'var1': '${var2}', 'var2': 't2' }}]}); - analytics.buildCallback(); - expect(sendRequestSpy.calledOnce).to.be.true; - expect(sendRequestSpy.args[0][0]).to.equal( - 'https://example.com/test=%24%7Bvar2%7D'); + return analytics.layoutCallback().then(() => { + expect(sendRequestSpy.calledOnce).to.be.true; + expect(sendRequestSpy.args[0][0]).to.equal( + 'https://example.com/test=%24%7Bvar2%7D'); + }); }); it('expands and encodes requests, config vars, and trigger vars', () => { @@ -376,11 +425,12 @@ describe('amp-analytics', function() { 't1': 'trigger=1', 't2': 'trigger?2' }}]}); - analytics.buildCallback(); - expect(sendRequestSpy.calledOnce).to.be.true; - expect(sendRequestSpy.args[0][0]).to.equal( - 'https://example.com/test?c1=config%201&t1=trigger%3D1&' + - 'c2=config%262&t2=trigger%3F2'); + return analytics.layoutCallback().then(() => { + expect(sendRequestSpy.calledOnce).to.be.true; + expect(sendRequestSpy.args[0][0]).to.equal( + 'https://example.com/test?c1=config%201&t1=trigger%3D1&' + + 'c2=config%262&t2=trigger%3F2'); + }); }); it('expands url-replacements vars', () => { @@ -395,11 +445,12 @@ describe('amp-analytics', function() { 'var2': 'DOCUMENT_REFERRER' } }]}); - analytics.buildCallback(); - expect(sendRequestSpy.calledOnce).to.be.true; - expect(sendRequestSpy.args[0][0]).to.equal( - 'https://example.com/test1=x&test2=https%3A%2F%2Fwww.google.com%2F' + - '&title=Test%20Title'); + return analytics.layoutCallback().then(() => { + expect(sendRequestSpy.calledOnce).to.be.true; + expect(sendRequestSpy.args[0][0]).to.equal( + 'https://example.com/test1=x&test2=https%3A%2F%2Fwww.google.com%2F' + + '&title=Test%20Title'); + }); }); it('respects optout', function() { @@ -410,21 +461,22 @@ describe('amp-analytics', function() { 'optout': 'foo.bar' }; let analytics = getAnalyticsTag(config); - analytics.buildCallback(); - expect(sendRequestSpy.withArgs('https://example.com/bar').calledOnce) - .to.be.true; - - sendRequestSpy.reset(); - windowApi['foo'] = {'bar': function() { return true; }}; - analytics = getAnalyticsTag(config); - analytics.buildCallback(); - expect(sendRequestSpy.callCount).to.be.equal(0); - - sendRequestSpy.reset(); - windowApi['foo'] = {'bar': function() { return false; }}; - analytics = getAnalyticsTag(config); - analytics.buildCallback(); - expect(sendRequestSpy.withArgs('https://example.com/bar').calledOnce) - .to.be.true; + return analytics.layoutCallback().then(() => { + expect(sendRequestSpy.withArgs('https://example.com/bar').calledOnce) + .to.be.true; + sendRequestSpy.reset(); + windowApi['foo'] = {'bar': function() { return true; }}; + analytics = getAnalyticsTag(config); + analytics.layoutCallback().then(() => { + expect(sendRequestSpy.callCount).to.be.equal(0); + sendRequestSpy.reset(); + windowApi['foo'] = {'bar': function() { return false; }}; + analytics = getAnalyticsTag(config); + analytics.layoutCallback().then(() => { + expect(sendRequestSpy.withArgs('https://example.com/bar').calledOnce) + .to.be.true; + }); + }); + }); }); }); diff --git a/extensions/amp-list/0.1/amp-list.js b/extensions/amp-list/0.1/amp-list.js index a3cf689fe64e..09d7d2956826 100644 --- a/extensions/amp-list/0.1/amp-list.js +++ b/extensions/amp-list/0.1/amp-list.js @@ -56,19 +56,20 @@ export class AmpList extends AMP.BaseElement { /** @override */ layoutCallback() { - const src = this.urlReplacements_.expand(assertHttpsUrl( - this.element.getAttribute('src'), this.element)); - const opts = { - credentials: this.element.getAttribute('credentials') - }; - return xhrFor(this.getWin()).fetchJson(src, opts).then(data => { - assert(typeof data == 'object' && Array.isArray(data['items']), - 'Response must be {items: []} object %s %s', - this.element, data); - const items = data['items']; - return templatesFor(this.getWin()).findAndRenderTemplateArray( - this.element, items).then(this.rendered_.bind(this)); - }); + return this.urlReplacements_.expand(assertHttpsUrl( + this.element.getAttribute('src'), this.element)).then(src => { + const opts = { + credentials: this.element.getAttribute('credentials') + }; + return xhrFor(this.getWin()).fetchJson(src, opts); + }).then(data => { + assert(typeof data == 'object' && Array.isArray(data['items']), + 'Response must be {items: []} object %s %s', + this.element, data); + const items = data['items']; + return templatesFor(this.getWin()).findAndRenderTemplateArray( + this.element, items).then(this.rendered_.bind(this)); + }); } /** diff --git a/src/runtime.js b/src/runtime.js index db0c71f53b0a..00e7ad679f0c 100644 --- a/src/runtime.js +++ b/src/runtime.js @@ -47,7 +47,9 @@ export function adopt(global) { // of functions const preregisteredElements = global.AMP || []; - global.AMP = {}; + global.AMP = { + win: global + }; /** * Registers an extended element and installs its styles. diff --git a/src/service.js b/src/service.js index 5afaaec7e9f3..209d358f72f0 100644 --- a/src/service.js +++ b/src/service.js @@ -75,8 +75,9 @@ export function getService(win, id, opt_factory) { export function getElementService(win, id, providedByElement) { assert(isElementScheduled(win, providedByElement), 'Service %s was requested to be provided through %s, ' + - 'but %s is not loaded in the current page.', - id, providedByElement, providedByElement); + 'but %s is not loaded in the current page. To fix this ' + + 'problem load the JavaScript file for %s in this page.', + id, providedByElement, providedByElement, providedByElement); const services = getServices(win, id); const s = services[id]; if (s) { @@ -115,6 +116,20 @@ function isElementScheduled(win, elementName) { return !!win.ampExtendedElements[elementName]; } +/** + * In order to provide better error messages we only allow to retrieve + * services from other elements if those elements are loaded in the page. + * This makes it possible to mark an element as loaded in a test. + * @param {!Window} win + * @param {string} elementName Name of an extended custom element. + */ +export function markElementScheduledForTesting(win, elementName) { + if (!win.ampExtendedElements) { + win.ampExtendedElements = {}; + } + win.ampExtendedElements[elementName] = true; +} + /** * Returns the object that holds the services registered in a window. * @param {!Window} win diff --git a/src/service/cid-impl.js b/src/service/cid-impl.js index 0af3430c6d5d..028724d10b7d 100644 --- a/src/service/cid-impl.js +++ b/src/service/cid-impl.js @@ -84,6 +84,9 @@ class Cid { * given. */ get(externalCidScope, consent, opt_persistenceConsent) { + assert(/^[a-zA-Z0-9-_]+$/.test(externalCidScope), + 'The client id name must match only use the characters ' + + '[a-zA-Z0-9-_]+\nInstead found %s', externalCidScope); return consent.then(() => { return getExternalCid(this, externalCidScope, opt_persistenceConsent || consent); diff --git a/src/url-replacements.js b/src/url-replacements.js index 70b179c15b48..48f47721890a 100644 --- a/src/url-replacements.js +++ b/src/url-replacements.js @@ -15,8 +15,9 @@ */ import {assert} from './asserts'; +import {cidFor} from './cid'; import {documentInfoFor} from './document-info'; -import {getService} from './service'; +import {getService, getElementService} from './service'; import {parseUrl, removeFragment} from './url'; @@ -82,7 +83,16 @@ class UrlReplacements { // single page view. It should have sufficient entropy to be unique for // all the page views a single user is making at a time. this.set_('PAGE_VIEW_ID', () => { - documentInfoFor(this.win_).pageViewId; + return documentInfoFor(this.win_).pageViewId; + }); + + this.set_('CLIENT_ID', (data, name) => { + return cidFor(this.win_).then(cid => { + return cid.get(name, + // TODO(@cramforce): Hook up mechanism to get consent to id + // generation. + Promise.resolve()); + }); }); } @@ -95,8 +105,6 @@ class UrlReplacements { * @private */ set_(varName, resolver) { - assert(varName == varName.toUpperCase(), - 'Variable name must be in upper case: %s', varName); this.replacements_[varName] = resolver; this.replacementExpr_ = undefined; return this; @@ -107,19 +115,42 @@ class UrlReplacements { * resolved values. * @param {string} url * @param {*} opt_data - * @return {string} + * @return {!Promise} */ expand(url, opt_data) { const expr = this.getExpr_(); - return url.replace(expr, (match, name) => { - let val = this.replacements_[name](opt_data); + let replacementPromise; + const encodeValue = val => { // Value 0 is specialcased because the numeric 0 is a valid substitution // value. if (!val && val !== 0) { val = ''; } return encodeURIComponent(val); + }; + url = url.replace(expr, (match, name, arg) => { + const val = this.replacements_[name](opt_data, arg); + // In case the produced value is a promise, we don't actually + // replace anything here, but do it again when the promise resolves. + if (val && val.then) { + const p = val.then(v => { + url = url.replace(match, encodeValue(v)); + }); + if (replacementPromise) { + replacementPromise = replacementPromise.then(() => p); + } else { + replacementPromise = p; + } + return match; + } + return encodeValue(val); }); + + if (replacementPromise) { + replacementPromise = replacementPromise.then(() => url); + } + + return replacementPromise || Promise.resolve(url); } /** @@ -132,7 +163,13 @@ class UrlReplacements { for (const k in this.replacements_) { all += (all.length > 0 ? '|' : '') + k; } - this.replacementExpr_ = new RegExp('\\$?(' + all + ')', 'g'); + this.replacementExpr_ = + // Match the given replacement patterns, as well as optionally + // arguments to the replacement behind it in parantheses. + // Example string that match + // FOO_BAR + // FOO_BAR(arg1) + new RegExp('\\$?(' + all + ')(?:\\(([a-zA-Z-_]+)\\))?', 'g'); } return this.replacementExpr_; } diff --git a/test/_init_tests.js b/test/_init_tests.js index 4d997e187df7..f75f36a88d2b 100644 --- a/test/_init_tests.js +++ b/test/_init_tests.js @@ -61,6 +61,7 @@ afterEach(() => { } } window.localStorage.clear(); + window.ampExtendedElements = {}; }); chai.Assertion.addMethod('attribute', function(attr) { diff --git a/test/functional/test-cid.js b/test/functional/test-cid.js index 5a2da23b94bd..211d1395a439 100644 --- a/test/functional/test-cid.js +++ b/test/functional/test-cid.js @@ -235,7 +235,7 @@ describe('cid', () => { const consent = timer.promise(100).then(() => { nonce = 'timer fired'; }); - const p = cid.get('', consent).then(c => { + const p = cid.get('test', consent).then(c => { expect(nonce).to.equal('timer fired'); }); clock.tick(100); @@ -246,6 +246,12 @@ describe('cid', () => { return expect(cid.get('abc', Promise.reject())).to.be.rejected; }); + it('should fail on invalid scope', () => { + expect(() => { + cid.get('$$$', Promise.resolve()); + }).to.throw(/\$\$\$/); + }); + it('should not store until persistence promise resolves', () => { let resolve; const persistencePromise = new Promise(r => { diff --git a/test/functional/test-url-replacements.js b/test/functional/test-url-replacements.js index b135c6a6c629..16de22a10a4e 100644 --- a/test/functional/test-url-replacements.js +++ b/test/functional/test-url-replacements.js @@ -16,17 +16,25 @@ import {createIframePromise} from '../../testing/iframe'; import {urlReplacementsFor} from '../../src/url-replacements'; +import {markElementScheduledForTesting} from '../../src/service'; +import {installCidService} from '../../src/service/cid-impl'; +import {setCookie} from '../../src/cookies'; describe('UrlReplacements', () => { - function expand(url) { + function expand(url, withCid) { return createIframePromise().then(iframe => { iframe.doc.title = 'Pixel Test'; const link = iframe.doc.createElement('link'); link.setAttribute('href', 'https://pinterest.com/pin1'); link.setAttribute('rel', 'canonical'); iframe.doc.head.appendChild(link); + if (withCid) { + markElementScheduledForTesting(iframe.win, 'amp-analytics'); + installCidService(iframe.win); + } + const replacements = urlReplacementsFor(iframe.win); return replacements.expand(url); }); @@ -82,10 +90,19 @@ describe('UrlReplacements', () => { it('should replace PAGE_VIEW_ID', () => { return expand('?pid=PAGE_VIEW_ID').then(res => { - expect(res).to.not.match(/pid=\d+/); + expect(res).to.match(/pid=\d+/); }); }); + it('should replace CLIENT_ID', () => { + setCookie(window, 'url-abc', 'cid-for-abc'); + setCookie(window, 'url-xyz', 'cid-for-xyz'); + return expand('?a=CLIENT_ID(url-abc)&b=CLIENT_ID(url-xyz)', true) + .then(res => { + expect(res).to.equal('?a=cid-for-abc&b=cid-for-xyz'); + }); + }); + it('should accept $expressions', () => { return expand('?href=$CANONICAL_URL').then(res => { expect(res).to.equal('?href=https%3A%2F%2Fpinterest.com%2Fpin1'); @@ -109,12 +126,26 @@ describe('UrlReplacements', () => { it('should replace new substitutions', () => { const replacements = urlReplacementsFor(window); replacements.set_('ONE', () => 'a'); - expect(replacements.expand('?a=ONE')).to.equal('?a=a'); - + expect(replacements.expand('?a=ONE')).to.eventually.equal('?a=a'); replacements.set_('ONE', () => 'b'); - expect(replacements.expand('?a=ONE')).to.equal('?a=b'); - replacements.set_('TWO', () => 'b'); - expect(replacements.expand('?b=TWO')).to.equal('?b=b'); + return expect(replacements.expand('?a=ONE&b=TWO')) + .to.eventually.equal('?a=b&b=b'); + }); + + it('should support positional arguments', () => { + const replacements = urlReplacementsFor(window); + replacements.set_('FN', (data, one) => one); + expect(replacements.expand('?a=FN(xyz)')).to.eventually.equal('?a=xyz'); + }); + + it('should support promises as replacements', () => { + const replacements = urlReplacementsFor(window); + replacements.set_('P1', () => Promise.resolve('abc ')); + replacements.set_('P2', () => Promise.resolve('xyz')); + replacements.set_('P3', () => Promise.resolve('123')); + replacements.set_('OTHER', () => 'foo'); + return expect(replacements.expand('?a=P1&b=P2&c=P3&d=OTHER')) + .to.eventually.equal('?a=abc%20&b=xyz&c=123&d=foo'); }); });