Distinguish installing a service and creating it #7839

kmh287 · 2017-02-28T18:08:10Z

This PR creates a distinction between registering/installing a service. Installation/registration of a service now registers a constructor that is used to instantiate the service the first time the service is requested.

Installation should be idempotent; registering a service multiple times should not clear out an existing instance of the service.

Part 1 of #4986

/to @choumx @jridgewell

…tration1

jridgewell · 2017-03-01T18:12:27Z

src/service.js

+          : opt_factory(context);
+      // The service may have been requested already, in which case we have a
+      // pending promise we need to fulfill.
+      if (s.resolve) {


Won't we need to resolve regardless of s.ctor?

jridgewell · 2017-03-01T18:12:54Z

src/service.js

+      const ctor = s.ctor;
+      s.obj = new ctor(context);
+    } else {
+      // TODO(kmh287): Replace opt_constructor param with ctor on service


Is this a legacy path while we convert over?

Yes. Eventually the opt_constructor argument can be removed entirely, simplifying this a bit

dreamofabear

Good catch! OOC, which services have getServicePromise called before registration?

dreamofabear · 2017-03-03T18:43:31Z

src/runtime.js

@@ -507,13 +506,15 @@ function prepareAndRegisterServiceForDocShadowMode(global, extensions,
 * @param {function(!./service/ampdoc-impl.AmpDoc):!Object=} opt_factory
 */
 function registerServiceForDoc(ampdoc, name, opt_ctor, opt_factory) {
-  dev().assert((opt_ctor || opt_factory) && (!opt_ctor || !opt_factory),
+  dev().assert(!!opt_ctor != !!opt_factory,


Nit: Single negation would suffice.

dreamofabear · 2017-03-03T18:44:23Z

src/runtime.js

+  registerServiceBuilderForDoc(ampdoc,
+                               name,
+                               opt_factory || undefined,
+                               opt_ctor || undefined,


|| undefined is unnecessary.

dreamofabear · 2017-03-03T18:46:05Z

src/service.js

+ */
+function registerServiceInternal(holder, context, id, opt_factory, opt_ctor) {
+  dev().assert(!!opt_factory != !!opt_ctor,
+    `Provide a constructor or a factory, but not both for service ${id}`);


Nit: +2 spaces

dreamofabear · 2017-03-03T19:02:15Z

src/service.js

+    `Provide a constructor or a factory, but not both for service ${id}`);
+  const services = getServices(holder);
+  let s = services[id];
+  if (s && !s.obj && !s.ctor && !s.factory) {


Long chains of logical ops are not very readable. I'd rewrite this like:

if (!s) { s = services[id] = ... } s.ctor = s.ctor || opt_ctor; s.factory = s.factory || opt_factory;

The above code won't overwrite existing ctor/factory props. I recall you mention checking for that case?

There is a slight difference here. The proposed code would allow a constructor or factory to be registered even if the other type of creator already exists.

For instance:

registerService(window, 'Foo', /* constructor */ undefined, window => new FooClass1(window)); registerService(window, 'Foo', FooClass2); getService(window, 'Foo')

The last line will return an instance of FooClass2, when it should return an instance of FooClass1. Reversing the lines will still result in an instance of FooClass2 because when we instantiate the service, we prefer a constructor to a factory.

True, but that should be fine. Besides, we shouldn't implement this method based on the implementation details of getService.

Resolved offline.

dreamofabear · 2017-03-03T19:08:34Z

src/service.js

      return s.promise = Promise.resolve(s.obj);
+    } else if (!s.ctor && !s.factory) {


Nit: IMO this is more readable if you remove this branch and put the condition into the assert(). This conditional implies that ctor and factory are relevant to the implementation of this method when they aren't really.

dreamofabear · 2017-03-03T19:12:05Z

src/service.js

+  const holder = getAmpdocServiceHolder(ampdoc);
+  registerServiceInternal(holder, ampdoc, id, opt_factory, opt_constructor);
+  if (opt_instantiate) {
+    getServiceInternal(holder, ampdoc, id);


Shouldn't this also be called if getServicePromise has already been called for this service?

I don't think so. If the caller of registerServiceBuilder chose to only register and not instantiate the service, then I'm not sure we should instantiate it here if there's an existing service promise. What are your thoughts?

If I call getServicePromise on service foo and then register it afterwards, when will the promise be resolved?

Isn't the point of this issue to not instantiate the service until requested? Does getServicePromise not count?

Before this change, getServicePromise returned a promise that didn't resolve until another caller created the service.

In your example, the promise won't be resolved until getService(window, 'foo') is called.

kmh287 · 2017-03-03T19:55:10Z

@choumx viewer, storage, amp-analytics-instrumentation are all services that are requested via getServicePromiseForDoc.

jridgewell · 2017-03-04T01:16:03Z

src/runtime.js

@@ -507,13 +506,15 @@ function prepareAndRegisterServiceForDocShadowMode(global, extensions,
 * @param {function(!./service/ampdoc-impl.AmpDoc):!Object=} opt_factory
 */
 function registerServiceForDoc(ampdoc, name, opt_ctor, opt_factory) {
-  dev().assert((opt_ctor || opt_factory) && (!opt_ctor || !opt_factory),
+  dev().assert(!opt_ctor != !opt_factory,


Shouldn't this be moved into registerServiceBuilderForDoc?

I already have a check in service.js for this. Do you think this check should be deleted from here?

jridgewell · 2017-03-04T01:17:13Z

src/service.js

 * @typedef {{
 *   obj: (?Object),
 *   promise: (?Promise|undefined),
 *   resolve: (?function(!Object)|undefined),
+ *   ctor: (?function(new:Object, !Window)|?function(new:Object, !./service/ampdoc-impl.AmpDoc)),
+ *   factory: (?function(!Window)|?function(!./service/ampdoc-impl.AmpDoc))


(?function(!Window):!Object|?function(!./service/ampdoc-impl.AmpDoc):Object)

Maybe (?function(!Window|!./service/ampdoc-impl.AmpDoc):!Object)?

First one SGTM. The second seems incorrect to me. We want either a function that takes in a window, or a function that takes in an ampdoc. That reads to me as "a function that can take either a window or an ampdoc" which isn't what we want.

dreamofabear · 2017-03-06T19:42:55Z

src/service.js

+  // pending promise we need to fulfill.
+  const p = getServicePromiseOrNullInternal(win, id);
+  if (opt_instantiate || p) {
+    /** Force instantiation and resolve service promise if it exists */


Nit: Use // style comment here.

kmh287 · 2017-03-06T21:32:03Z

@choumx @jridgewell

Still not quite sure about this PR yet. The following sequence of events doesn't quite work out

1: Service is registered, but not instantiated
2: Service is requested by promise

If this happens, the promise never resolves.

The requester may not have access to the argument the constructor/factory needs to create the service, so I don't think we can just have the caller pass it in. I think the only viable solution here is to also store the context with the constructor and factory. Thoughts?

jridgewell · 2017-03-06T21:54:52Z

The requester may not have access to the argument the constructor/factory needs to create the service

Isn't it always AMP?

dreamofabear · 2017-03-06T23:11:29Z

Oops, please add a unit test for this scenario too. Another option is to store a closure that generates the service with the ctor or factory.

kmh287 · 2017-03-07T13:26:14Z

@jridgewell it's either a window or an ampdoc.

@choumx I think the closure is probably the cleanest option and could allow us to clean up what we're putting on the service as we won't need distinct properties for constructor vs factory.

kmh287 · 2017-03-07T14:10:20Z

Thanks for your patience on the many rounds of comments on this PR. I've addressed the issue I mentioned before and the code seems a lot cleaner for it. PTAL when you have a chance.

dreamofabear

Thanks for taking the time to clean up the PR on this new revision.

dreamofabear · 2017-03-08T19:13:07Z

src/service.js

+  if (s.promise && s.resolve) {
+    s.resolve(s.build());
+  }
+


Nit: Extra line.

dreamofabear · 2017-03-08T19:13:31Z

src/service.js

+  }
+
+  if (s.build) {
+    // Service already registered


Warn?

Also, period at end of sentence (nit).

Added the period. Not sure that warning here is a good idea. We will end up registering a service multiple times anyway since any time a service is requested, we'll need to make sure it's registered first. Registrations after the first will just noop (as they'll bail out on this line)

dreamofabear · 2017-03-08T19:14:47Z

src/service.js

+    s.resolve = resolve;
+    // Instantiate service immediately.
+    if (s.build) {
+      s.obj = s.build();


We only want the object to be built once. Doesn't this build it on every invocation?

Please add a unit test for this.

Nevermind. 😄 Do we have a unit test for this case though?

It's admittedly a bit confusing. The call to getServicePromiseOrnullInternal will return a promise if the service has already been built, preventing this from executing more than once. I've added a test regardless.

dreamofabear · 2017-03-08T20:39:58Z

test/functional/test-service.js

+    it('should only instantiate the service once', () => {
+      registerServiceBuilder(window, 'b', Class);
+      expect(count).to.equal(0);
+      for (let i = 0; i < 10; i++) {


Just two getService calls would suffice here.

kmh287 added 2 commits February 28, 2017 12:01

changes to service.js

beabda7

cleanup

2ca1cdf

kmh287 requested review from jridgewell and dreamofabear February 28, 2017 18:08

This was referenced Mar 1, 2017

Service Registration: Document State #7857

Merged

Service Registration: Extensions #7858

Merged

Service Registration: Cid #7859

Merged

Service Registration: Activity #7860

Merged

kmh287 added 2 commits March 1, 2017 13:06

Merge branch 'master' of github.com:kmh287/amphtml into service_regis…

3869a83

…tration1

register not instantiate in runtime.js

87d7204

This was referenced Mar 1, 2017

Service Registration: Instrumentation #7870

Merged

Service Registration: Variable #7871

Merged

Service Registration: User Notification #7872

Merged

jridgewell approved these changes Mar 1, 2017

View reviewed changes

kmh287 requested a review from jridgewell March 3, 2017 16:59

dreamofabear requested changes Mar 3, 2017

View reviewed changes

pr comments

a3fa55b

kmh287 added 2 commits March 3, 2017 16:48

pr comments

abeedb0

cleanup

20e9e5e

jridgewell reviewed Mar 4, 2017

View reviewed changes

pr comments and another test

7f3378f

dreamofabear approved these changes Mar 6, 2017

View reviewed changes

pr comments

c463251

service registration then promise edge case

536f724

kmh287 added 3 commits March 7, 2017 09:43

swap opt_ctor and opt_factory args as ctor is much more common

bb3ec75

test fix

4ff62ad

refactoring woes

d2a11b5

dreamofabear requested changes Mar 8, 2017

View reviewed changes

pr comments

0d424d6

dreamofabear reviewed Mar 8, 2017

View reviewed changes

dreamofabear approved these changes Mar 8, 2017

View reviewed changes

pr comment

38962a9

kmh287 merged commit 9317fd1 into ampproject:master Mar 9, 2017

kmh287 added a commit to kmh287/amphtml that referenced this pull request Mar 13, 2017

Distinguish installing a service and creating it (ampproject#7839)

62cf820

dvoytenko mentioned this pull request Apr 6, 2017

Registered/expected services should never resolve null from any of the methods #8616

Closed

kmh287 mentioned this pull request Apr 10, 2017

Service Registration: Parallax #8662

Merged

dvoytenko added the PR use: In Stable label Apr 12, 2017

mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017

Distinguish installing a service and creating it (ampproject#7839)

3a96562

		return s.promise = Promise.resolve(s.obj);
		} else if (!s.ctor && !s.factory) {

Distinguish installing a service and creating it #7839

Distinguish installing a service and creating it #7839

Conversation

kmh287 commented Feb 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmh287 Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

dreamofabear left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmh287 Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmh287 commented Mar 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmh287 commented Mar 6, 2017

jridgewell commented Mar 6, 2017

dreamofabear commented Mar 6, 2017

kmh287 commented Mar 7, 2017

kmh287 commented Mar 7, 2017

dreamofabear left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmh287 Mar 1, 2017 •

edited

Loading

kmh287 Mar 3, 2017 •

edited

Loading