diff --git a/extensions/amp-user-notification/0.1/test/test-amp-user-notification.js b/extensions/amp-user-notification/0.1/test/test-amp-user-notification.js index 29c3e30a43c70..a0c76db823973 100644 --- a/extensions/amp-user-notification/0.1/test/test-amp-user-notification.js +++ b/extensions/amp-user-notification/0.1/test/test-amp-user-notification.js @@ -19,7 +19,7 @@ import { UserNotificationManager, } from '../amp-user-notification'; import {createIframePromise} from '../../../../testing/iframe'; -import {getExistingServiceForDoc} from '../../../../src/service'; +import {getServiceForDoc} from '../../../../src/service'; import * as sinon from 'sinon'; @@ -50,7 +50,7 @@ describe('amp-user-notification', () => { function getUserNotification(attrs = {}) { return createIframePromise().then(iframe_ => { iframe = iframe_; - storage = getExistingServiceForDoc(iframe.ampdoc, 'storage'); + storage = getServiceForDoc(iframe.ampdoc, 'storage'); storageMock = sandbox.mock(storage); return buildElement(iframe.doc, iframe.ampdoc, attrs); }); diff --git a/src/service.js b/src/service.js index f83455888b754..7211f943996ab 100644 --- a/src/service.js +++ b/src/service.js @@ -65,72 +65,38 @@ export class EmbeddableService { adoptEmbedWindow(unusedEmbedWin) {} } - -/** - * Returns a service with the given id. Assumes that it has been constructed - * already. - * @param {!Window} win - * @param {string} id - * @return {!Object} The service. - */ -export function getExistingServiceForWindow(win, id) { - const exists = getExistingServiceForWindowOrNull(win, id); - return dev().assert(/** @type {!Object} */ (exists), - `${id} service not found. Make sure it is installed.`); -} - /** * Returns a service or null with the given id. * @param {!Window} win * @param {string} id * @return {?Object} The service. */ -export function getExistingServiceForWindowOrNull(win, id) { +export function getExistingServiceOrNull(win, id) { win = getTopWindow(win); - if (win.services && win.services[id]) { - return getServiceInternal(win, win, id); + if (isServiceRegistered(win, id)) { + return getServiceInternal(win, id); } else { return null; } } /** - * Returns a service with the given id. Assumes that it has been constructed + * Returns a service with the given id. Assumes that it has been registered * already. * @param {!Window} win * @param {string} id * @return {!Object} The service. */ -export function getExistingServiceForWindowInEmbedScope(win, id) { +export function getExistingServiceInEmbedScope(win, id) { // First, try to resolve via local (embed) window. const local = getLocalExistingServiceForEmbedWinOrNull(win, id); if (local) { return local; } // Fallback to top-level window. - return getExistingServiceForWindow(win, id); -} - - -/** - * Returns a service with the given id. Assumes that it has been constructed - * already. - * @param {!Node|!./service/ampdoc-impl.AmpDoc} nodeOrDoc - * @param {string} id - * @return {!Object} The service. - */ -export function getExistingServiceForDoc(nodeOrDoc, id) { - const ampdoc = getAmpdoc(nodeOrDoc); - const serviceHolder = getAmpdocServiceHolder(nodeOrDoc); - let service; - if (serviceHolder && serviceHolder.services && serviceHolder.services[id]) { - service = getServiceInternal(serviceHolder, ampdoc, id); - } - return dev().assert(/** @type (!Object) */ (service), - `${id} doc service not found. Make sure it is installed.`); + return getService(win, id); } - /** * Returns a service with the given id. Assumes that it has been constructed * already. @@ -150,10 +116,9 @@ export function getExistingServiceForDocInEmbedScope(nodeOrDoc, id) { } } // Fallback to ampdoc. - return getExistingServiceForDoc(nodeOrDoc, id); + return getServiceForDoc(nodeOrDoc, id); } - /** * Installs a service override on amp-doc level. * @param {!Window} embedWin @@ -166,10 +131,16 @@ export function installServiceInEmbedScope(embedWin, id, service) { 'Service override can only be installed in embed window: %s', id); dev().assert(!getLocalExistingServiceForEmbedWinOrNull(embedWin, id), 'Service override has already been installed: %s', id); - getServiceInternal(embedWin, embedWin, id, undefined, () => service); + registerServiceInternal( + embedWin, + embedWin, + id, + /* opt_ctor */ undefined, + () => service); + // Force service to build + getServiceInternal(embedWin, id); } - /** * @param {!Window} embedWin * @param {string} id @@ -180,14 +151,13 @@ function getLocalExistingServiceForEmbedWinOrNull(embedWin, id) { // It does not try to go all the way up the parent window chain. We can change // this in the future, but for now this gives us a better performance. const topWin = getTopWindow(embedWin); - if (embedWin != topWin && embedWin.services && embedWin.services[id]) { - return getServiceInternal(embedWin, embedWin, id); + if (embedWin != topWin && isServiceRegistered(embedWin, id)) { + return getServiceInternal(embedWin, id); } else { return null; } } - /** * Returns a service for the given id and window (a per-window singleton). * If the service is not yet available the factory function is invoked and @@ -197,31 +167,24 @@ function getLocalExistingServiceForEmbedWinOrNull(embedWin, id) { * passed around. * @param {!Window} win * @param {string} id of the service. - * @param {function(!Window):T=} opt_factory Should create the service - * if it does not exist yet. If the factory is not given, it is an error - * if the service does not exist yet. + * @param {function(!Window):T=} opt_factory Factory to create the service + * if the service does not exist yet. It is an error if the service does + * not exist and the caller does not provide opt_factory. * @template T * @return {T} */ export function getService(win, id, opt_factory) { win = getTopWindow(win); - return getServiceInternal(win, win, id, undefined, - opt_factory ? opt_factory : undefined); -} - - -/** - * Returns a service and registers it given a class to be used as - * implementation. - * @param {!Window} win - * @param {string} id of the service. - * @param {function(new:T, !Window)} constructor - * @return {T} - * @template T - */ -export function fromClass(win, id, constructor) { - win = getTopWindow(win); - return getServiceInternal(win, win, id, constructor); + if (!isServiceRegistered(win, id)) { + dev().assert(opt_factory, 'Factory not given and service missing %s', id); + registerServiceBuilder( + win, + id, + /* opt_ctor */undefined, + opt_factory, + /* opt_instantiate */ true); + } + return getServiceInternal(win, id); } /** @@ -240,7 +203,7 @@ export function registerServiceBuilder(win, win = getTopWindow(win); registerServiceInternal(win, win, id, opt_constructor, opt_factory); if (opt_instantiate) { - getServiceInternal(win, win, id); + getServiceInternal(win, id); } } @@ -262,7 +225,7 @@ export function registerServiceBuilderForDoc(nodeOrDoc, const holder = getAmpdocServiceHolder(ampdoc); registerServiceInternal(holder, ampdoc, id, opt_constructor, opt_factory); if (opt_instantiate) { - getServiceInternal(holder, ampdoc, id); + getServiceInternal(holder, id); } } @@ -292,7 +255,6 @@ export function getServicePromiseOrNull(win, id) { return getServicePromiseOrNullInternal(win, id); } - /** * Returns a service for the given id and ampdoc (a per-ampdoc singleton). * If the service is not yet available the factory function is invoked and @@ -300,41 +262,27 @@ export function getServicePromiseOrNull(win, id) { * @param {!Node|!./service/ampdoc-impl.AmpDoc} nodeOrDoc * @param {string} id of the service. * @param {function(!./service/ampdoc-impl.AmpDoc):T=} opt_factory - * Should create the service if it does not exist yet. If the factory is - * not given, it is an error if the service does not exist yet. + * Factory to create the service if the service does not exist yet. + * It is an error if the service does not exist and the caller does + * not provide opt_factory. * @return {T} * @template T */ export function getServiceForDoc(nodeOrDoc, id, opt_factory) { const ampdoc = getAmpdoc(nodeOrDoc); - return getServiceInternal( - getAmpdocServiceHolder(ampdoc), - ampdoc, - id, - /* opt_ctor */ undefined, - opt_factory); -} - - -/** - * Returns a service and registers it given a class to be used as - * implementation. - * @param {!Node|!./service/ampdoc-impl.AmpDoc} nodeOrDoc - * @param {string} id of the service. - * @param {function(new:T, !./service/ampdoc-impl.AmpDoc)} constructor - * @return {T} - * @template T - */ -export function fromClassForDoc(nodeOrDoc, id, constructor) { - const ampdoc = getAmpdoc(nodeOrDoc); - return getServiceInternal( - getAmpdocServiceHolder(ampdoc), - ampdoc, - id, - constructor); + const holder = getAmpdocServiceHolder(ampdoc); + if (!isServiceRegistered(holder, id)) { + dev().assert(opt_factory, 'Factory not given and service missing %s', id); + registerServiceBuilderForDoc( + ampdoc, + id, + /* opt_ctor */ undefined, + opt_factory, + /* opt_instantiate */ true); + } + return getServiceInternal(holder, id); } - /** * Returns a promise for a service for the given id and ampdoc. Also expects * a service that has the actual implementation. The promise resolves when @@ -363,7 +311,6 @@ export function getServicePromiseOrNullForDoc(nodeOrDoc, id) { id); } - /** * Set the parent and top windows on a child window (friendly iframe). * @param {!Window} win @@ -452,47 +399,28 @@ function getAmpdocService(win) { /** + * Get service `id` from `holder`. Assumes the service + * has already been registered. * @param {!Object} holder Object holding the service instance. - * @param {!Window|!./service/ampdoc-impl.AmpDoc} context Win or AmpDoc. * @param {string} id of the service. - * @param {function(new:T, ?)=} opt_constructor - * Constructor function to new the service. Called with context. - * @param {function(?):T=} opt_factory - * Should create the service if it does not exist yet. If the factory - * is not given, it is an error if the service does not exist yet. - * Called with context. * @return {Object} * @template T */ -function getServiceInternal(holder, context, id, opt_constructor, opt_factory) { +function getServiceInternal(holder, id) { + dev().assert(isServiceRegistered(holder, id), + `Expected service ${id} to be registered`); const services = getServices(holder); - let s = services[id]; - if (!s) { - s = services[id] = { - obj: null, - promise: null, - resolve: null, - build: null, - }; - } - + const s = services[id]; if (!s.obj) { - if (s.build) { - s.obj = s.build(); - } else { - // TODO(kmh287): Clean up this code path once all services use the new - // registration code path. - dev().assert(opt_factory || opt_constructor, - 'Factory or class not given and service missing %s', id); - s.obj = opt_constructor - ? new opt_constructor(context) - : opt_factory(context); - } + dev().assert(s.build, `Service ${id} registered without builder nor impl.`); + s.obj = s.build(); + dev().assert(s.obj, `Service ${id} built to null.`); // The service may have been requested already, in which case we have a // pending promise we need to fulfill. if (s.resolve) { s.resolve(s.obj); } + s.build = null; } return s.obj; } @@ -521,7 +449,7 @@ function registerServiceInternal(holder, context, id, opt_ctor, opt_factory) { }; } - if (s.build) { + if (s.build || s.obj) { // Service already registered. return; } @@ -532,14 +460,12 @@ function registerServiceInternal(holder, context, id, opt_ctor, opt_factory) { // The service may have been requested already, in which case there is a // pending promise that needs to fulfilled. - if (s.promise && s.resolve) { - const obj = s.build(); - s.obj = obj; - s.resolve(obj); + if (s.resolve) { + // getServiceInternal will resolve the promise. + getServiceInternal(holder, id); } } - /** * @param {!Object} holder * @param {string} id of the service. @@ -550,37 +476,28 @@ function getServicePromiseInternal(holder, id) { if (cached) { return cached; } + // Service is not registered. // TODO(@cramforce): Add a check that if the element is eventually registered // that the service is actually provided and this promise resolves. let resolve; - const p = new Promise(r => { + const promise = new Promise(r => { resolve = r; }); const services = getServices(holder); - let s = services[id]; - if (s) { - // Service is registered, but not yet instantiated. - s.promise = p; - s.resolve = resolve; - // Instantiate service immediately. - if (s.build) { - s.obj = s.build(); - s.resolve(s.obj); - } - } else { - s = services[id] = { - obj: null, - promise: p, - resolve, - build: null, - }; - } - return p; + services[id] = { + obj: null, + promise, + resolve, + build: null, + }; + return promise; } /** + * Returns a promise for service `id` if the service has been registered + * on `holder`. * @param {!Object} holder * @param {string} id of the service. * @return {?Promise} @@ -591,13 +508,10 @@ function getServicePromiseOrNullInternal(holder, id) { if (s) { if (s.promise) { return s.promise; - } else if (s.obj) { - return s.promise = Promise.resolve(s.obj); } else { - dev().assert(s.build, - 'Expected object, promise, or builder to be present'); - s.obj = s.build(); - return s.promise = Promise.resolve(s.obj); + // Instantiate service if not already instantiated. + getServiceInternal(holder, id); + return s.promise = Promise.resolve(/** @type {!Object} */ (s.obj)); } } return null; @@ -733,7 +647,7 @@ export function adoptServiceForEmbed(embedWin, serviceId) { const frameElement = /** @type {!Node} */ (dev().assert( embedWin.frameElement, 'frameElement not found for embed')); - const service = getExistingServiceForDoc(frameElement, serviceId); + const service = getServiceForDoc(frameElement, serviceId); assertEmbeddable(service).adoptEmbedWindow(embedWin); } @@ -748,3 +662,14 @@ export function resetServiceForTesting(holder, id) { holder.services[id] = null; } } + +/** + * @param {!Object} holder Object holding the service instance. + * @param {string} id of the service. + * @return {boolean} + */ +function isServiceRegistered(holder, id) { + const service = holder.services && holder.services[id]; + // All registered services must have either an implementation or a builder. + return !!(service && (service.build || service.obj)); +} diff --git a/src/services.js b/src/services.js index 5d7096d0727d8..4d951e0cd66a2 100644 --- a/src/services.js +++ b/src/services.js @@ -18,7 +18,7 @@ import { getService, getServiceForDoc, getServicePromiseForDoc, - getExistingServiceForWindowOrNull, + getExistingServiceOrNull, getExistingServiceForDocInEmbedScope, } from './service'; import { @@ -175,7 +175,7 @@ export function performanceFor(window) { */ export function performanceForOrNull(window) { return /** @type {!./service/performance-impl.Performance}*/ ( - getExistingServiceForWindowOrNull(window, 'performance')); + getExistingServiceOrNull(window, 'performance')); } /** diff --git a/test/functional/test-analytics.js b/test/functional/test-analytics.js index c90debc5bade9..424958bd0f979 100644 --- a/test/functional/test-analytics.js +++ b/test/functional/test-analytics.js @@ -14,7 +14,6 @@ * limitations under the License. */ -import {fromClassForDoc} from '../../src/service'; import { getServiceForDoc, registerServiceBuilderForDoc, @@ -88,8 +87,10 @@ describes.realWin('analytics', {amp: true}, env => { const ele = win.document.createElement('div'); win.document.body.appendChild(ele); const baseEle = new BaseElement(ele); - fromClassForDoc( + registerServiceBuilderForDoc( ampdoc, 'amp-analytics-instrumentation', MockInstrumentation); + // Force instantiation + getServiceForDoc(ampdoc, 'amp-analytics-instrumentation'); const config = { 'requests': { 'pageview': 'https://example.com/analytics', diff --git a/test/functional/test-service.js b/test/functional/test-service.js index e7a46f850b2cd..5f2c924535b65 100644 --- a/test/functional/test-service.js +++ b/test/functional/test-service.js @@ -19,11 +19,9 @@ import { assertDisposable, assertEmbeddable, disposeServicesForDoc, - fromClass, getExistingServiceForDocInEmbedScope, - getExistingServiceForWindowInEmbedScope, - getExistingServiceForDoc, - getExistingServiceForWindow, + getExistingServiceInEmbedScope, + getExistingServiceOrNull, getParentWindowFrameElement, getService, getServiceForDoc, @@ -115,19 +113,6 @@ describe('service', () => { expect(factory.args[1][0]).to.equal(window); }); - it('should make instances from class', () => { - - const a1 = fromClass(window, 'a', Class); - const a2 = fromClass(window, 'a', Class); - expect(a1).to.equal(a2); - expect(a1.count).to.equal(1); - - const b1 = fromClass(window, 'b', Class); - const b2 = fromClass(window, 'b', Class); - expect(b1).to.equal(b2); - expect(b1).to.not.equal(a1); - }); - it('should not instantiate service when registered', () => { registerServiceBuilder(window, 'a', Class); expect(count).to.equal(0); @@ -152,14 +137,13 @@ describe('service', () => { it('should return the service when it exists', () => { const c1 = getService(window, 'c', factory); - const c2 = getExistingServiceForWindow(window, 'c'); + const c2 = getExistingServiceOrNull(window, 'c'); expect(c1).to.equal(c2); }); - it('should throw before creation', () => { - getService(window, 'another service to avoid NPE', () => {}); + it('should throw before creation if factory is not provided', () => { expect(() => { - getExistingServiceForWindow(window, 'c'); + getService(window, 'c'); }).to.throw(); }); @@ -217,6 +201,15 @@ describe('service', () => { expect(p).to.not.be.null; }); + it('should set service builders to null after instantiation', () => { + registerServiceBuilder(window, 'a', Class); + expect(window.services['a'].obj).to.be.null; + expect(window.services['a'].build).to.not.be.null; + getService(window, 'a'); + expect(window.services['a'].obj).to.not.be.null; + expect(window.services['a'].build).to.be.null; + }); + it('should resolve service for a child window', () => { const c = getService(window, 'c', factory); @@ -224,13 +217,13 @@ describe('service', () => { const child = {}; setParentWindow(child, window); expect(getService(child, 'c', factory)).to.equal(c); - expect(getExistingServiceForWindow(child, 'c')).to.equal(c); + expect(getExistingServiceOrNull(child, 'c')).to.equal(c); // A grandchild. const grandchild = {}; setParentWindow(grandchild, child); expect(getService(grandchild, 'c', factory)).to.equal(c); - expect(getExistingServiceForWindow(grandchild, 'c')).to.equal(c); + expect(getExistingServiceOrNull(grandchild, 'c')).to.equal(c); }); describe('embed service', () => { @@ -250,30 +243,30 @@ describe('service', () => { }); it('should return top service for top window', () => { - expect(getExistingServiceForWindowInEmbedScope(window, 'c')) + expect(getExistingServiceInEmbedScope(window, 'c')) .to.equal(topService); }); it('should return top service when not overriden', () => { - expect(getExistingServiceForWindowInEmbedScope(childWin, 'c')) + expect(getExistingServiceInEmbedScope(childWin, 'c')) .to.equal(topService); - expect(getExistingServiceForWindowInEmbedScope(grandchildWin, 'c')) + expect(getExistingServiceInEmbedScope(grandchildWin, 'c')) .to.equal(topService); }); it('should return overriden service', () => { const overridenService = {}; installServiceInEmbedScope(childWin, 'c', overridenService); - expect(getExistingServiceForWindowInEmbedScope(childWin, 'c')) + expect(getExistingServiceInEmbedScope(childWin, 'c')) .to.equal(overridenService); // Top-level service doesn't change. - expect(getExistingServiceForWindow(window, 'c')) + expect(getExistingServiceOrNull(window, 'c')) .to.equal(topService); // Notice that only direct overrides are allowed for now. This is // arbitrary can change in the future to allow hierarchical lookup // up the window chain. - expect(getExistingServiceForWindow(grandchildWin, 'c')) + expect(getExistingServiceOrNull(grandchildWin, 'c')) .to.equal(topService); }); }); @@ -325,7 +318,7 @@ describe('service', () => { const b1 = getServiceForDoc(node, 'b', factory); const b2 = getServiceForDoc(node, 'b', factory); - const b3 = getExistingServiceForDoc(node, 'b'); + const b3 = getServiceForDoc(node, 'b'); expect(b1).to.equal(b2); expect(b1).to.equal(b3); expect(b1).to.not.equal(a1); @@ -340,7 +333,7 @@ describe('service', () => { const a1 = getServiceForDoc(ampdoc, 'a', factory); const a2 = getServiceForDoc(ampdoc, 'a', factory); - const a3 = getExistingServiceForDoc(ampdoc, 'a', factory); + const a3 = getServiceForDoc(ampdoc, 'a', factory); expect(a1).to.equal(a2); expect(a1).to.equal(a3); expect(a1).to.equal(1); @@ -434,7 +427,7 @@ describe('service', () => { {nodeType: 1, ownerDocument: {defaultView: childWin}}; setParentWindow(childWin, windowApi); expect(getServiceForDoc(childWinNode, 'c', factory)).to.equal(c); - expect(getExistingServiceForDoc(childWinNode, 'c')).to.equal(c); + expect(getServiceForDoc(childWinNode, 'c')).to.equal(c); // A grandchild. const grandchildWin = {}; @@ -442,7 +435,7 @@ describe('service', () => { {nodeType: 1, ownerDocument: {defaultView: grandchildWin}}; setParentWindow(grandchildWin, childWin); expect(getServiceForDoc(grandChildWinNode, 'c', factory)).to.equal(c); - expect(getExistingServiceForDoc(grandChildWinNode, 'c')).to.equal(c); + expect(getServiceForDoc(grandChildWinNode, 'c')).to.equal(c); }); it('should dispose disposable services', () => {