Service Registration: Cleanup of service.js #8698

kmh287 · 2017-04-11T17:14:25Z

Now that no services use the fromClass and fromClassForDoc codepaths, service.js can be cleaned up a bit. This PR:

removes fromClass and fromClassForDoc. The same functionality is provided by registerService + getService and registerServiceForDoc + getServiceForDoc
Removes getExistingService{,forDoc} methods whose functionality is provided by asserts elsewhere.
getExistingServiceForWindowOrNull has been renamed to be consistent with other function names
Removes legacy code path related to the removed functions

dreamofabear · 2017-04-12T22:23:29Z

src/service.js

-export function fromClass(win, id, constructor) {
-  win = getTopWindow(win);
-  return getServiceInternal(win, win, id, constructor);
+  if (!isServiceRegistered(win, id)) {


The existence of opt_factory here and in getServiceForDoc breaks the registration pattern. What do you think we should do about this?

The current behavior is far from ideal: if you call getService with a factory but the service is already registered then the factory is ignored.

I can look into removing them, but I would rather not add that to this PR.

if you call getService with a factory but the service is already registered then the factory is ignored.

No different from trying to register a service twice, right?

I would rather not add that to this PR.

That's fine, just thinking about how to make this API simpler.

No different from trying to register a service twice, right?

Right, and incidentally this is how these functions worked before the service registration PRs. The recent changes just make this more apparent.

That's fine, just thinking about how to make this API simpler.

Gotcha. I will definitely followup after this.

dreamofabear · 2017-04-12T22:32:05Z

src/service.js

+ * @param {string} id
+ * @return {?Object}
+ */
+function getExistingServiceForEmbedWinOrNull(embedWin, id) {


Can you undo the change in position for these methods? It makes the diff harder to read.

In general, recommend minimizing diffs in sensitive changes like this PR.

dreamofabear · 2017-04-12T22:32:22Z

src/service.js

+ * @return {Object}
+ * @template T
+ */
+function getServiceInternal(holder, context, id) {


Ditto re: position.

dreamofabear · 2017-04-13T14:38:42Z

src/service.js

-    if (s.build) {
-      s.obj = s.build();
-      s.resolve(s.obj);
-    }


Removed because this is already done in getServicePromiseOrNullInternal?

Yes. getServicePromiseOrnullInternal should only return null if the service has never been registered. This means that there won't be an existing service.

dreamofabear · 2017-04-13T14:44:33Z

src/service.js

 /**
 * @param {!Window} embedWin
 * @param {string} id
 * @return {?Object}
 */
-function getLocalExistingServiceForEmbedWinOrNull(embedWin, id) {
+function getExistingServiceForEmbedWinOrNull(embedWin, id) {


Why remove the "Local" qualifier? Seems important vis-a-vis getExistingServiceInEmbedScope since the former falls back to the top-level window.

I could go either way on this. I assumed that the distinction came form explicitly mentioning the embed window in the function name vs the other function which just mentioned a more nebulous 'scope'. I'll revert it.

dreamofabear · 2017-04-13T14:47:47Z

src/service.js

-      id,
-      constructor);
+  const holder = getAmpdocServiceHolder(ampdoc);
+  if (!isServiceRegistered(holder, id)) {


Update the documentation to state that the service will be registered with opt_factory if unregistered.

dreamofabear · 2017-04-13T14:49:38Z

src/service.js

    // 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);
+    if (s.promise && s.resolve) {


Is there an instance where s.resolve is not null but s.promise is?

No, I just thought this would make the intention more explicit. (IIRC we were checking for both elsewhere as well) I'll update all other similar checks to only check for resolve

dreamofabear · 2017-04-13T14:55:47Z

src/service.js

-export function fromClass(win, id, constructor) {
-  win = getTopWindow(win);
-  return getServiceInternal(win, win, id, constructor);
+  if (!isServiceRegistered(win, id)) {


if you call getService with a factory but the service is already registered then the factory is ignored.

No different from trying to register a service twice, right?

I would rather not add that to this PR.

That's fine, just thinking about how to make this API simpler.

dreamofabear · 2017-04-13T15:00:26Z

Thanks for reverting the positioning changes. Diff is much more readable now.

dreamofabear · 2017-04-19T23:32:46Z

src/service.js

+      /* opt_ctor */ undefined,
+      () => service);
+  // Force service to build
+  getServiceInternal(embedWin, embedWin, id);


This can be reverted. Let's address the future of the convenience builder separately.

I'd rather we didn't as reverting this means reverting getServiceInternal as well. The public interface nor its behavior should change due to this.

dreamofabear · 2017-04-19T23:33:48Z

src/service.js

    }
+    s.build = null;


Please add a unit test for this.

dreamofabear · 2017-04-19T23:34:58Z

src/service.js

+  if (s.resolve) {
+    s.obj = s.build();
+    s.resolve(/** @type {!Object} */(s.obj));
+    s.build = null;


line deleted

dreamofabear · 2017-04-19T23:37:57Z

src/service.js

 }


 /**
+ * Returns a promise for service `id` if the service has been registered
+ * on `holder`.


Need to set s.build to null in this method. And a unit test. 😄

routing all of this through getServiceInternal to avoid duplication

jridgewell · 2017-04-20T21:09:03Z

src/service.js

    // 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.resolve(/** @type {!Object} */(s.obj));


Can we get rid of these typecasts by marking s.build correctly?

Looks like it can just be deleted.

jridgewell · 2017-04-20T21:11:12Z

src/service.js

-    s.obj = obj;
-    s.resolve(obj);
+  if (s.resolve) {
+    s.obj = s.build();


Just call getServiceInternal?

jridgewell

Now that you're familiar with the service.js functions, are there any duplicated ones that we can get rid of? Some of them seem very similar.

jridgewell · 2017-04-21T17:29:24Z

src/service.js

@@ -596,8 +512,9 @@ function getServicePromiseOrNullInternal(holder, id) {
    } else {
      dev().assert(s.build,


Isn't this handled by getServiceInternal?

We can get rid of it

jridgewell · 2017-04-21T17:29:37Z

src/service.js

@@ -596,8 +512,9 @@ function getServicePromiseOrNullInternal(holder, id) {
    } else {


Can we collapse the previous block into this one?

jridgewell · 2017-04-21T17:30:26Z

src/service.js

    // 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.resolve((s.obj));


kmh287

@jridgewell This PR set out to remove quite a few redundant functions. I think there's more work to do eliminating calls to getService that provide a factory, but I'm not sure how much cleaner this can get. We need to worry about

services on a window vs services on an ampdoc
embedded services vs top-level services
services that can be returned directly vs services that need to be resolved by promise
functions that get a service or service promise and expect them to exist vs functions that get a service or service promise if one exists and return null otherwise

We could probably clean those up some more, but I'm not sure how much more we could do without (further) altering the interface.

kmh287 · 2017-04-21T18:55:01Z

src/service.js

    // 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.resolve((s.obj));


kmh287 · 2017-04-21T18:57:55Z

src/service.js

@@ -596,8 +512,9 @@ function getServicePromiseOrNullInternal(holder, id) {
    } else {
      dev().assert(s.build,


We can get rid of it

kmh287 · 2017-04-21T18:57:59Z

src/service.js

@@ -596,8 +512,9 @@ function getServicePromiseOrNullInternal(holder, id) {
    } else {


dreamofabear · 2017-04-22T15:37:12Z

src/service.js

-    s.obj = obj;
-    s.resolve(obj);
+  if (s.resolve) {
+    // getServiceInternal will resolve the project.


"resolve the promise."

whoops, thanks

dreamofabear · 2017-04-22T15:40:16Z

test/functional/test-analytics.js

-      fromClassForDoc(
-          ampdoc, 'amp-analytics-instrumentation', MockInstrumentation);
+      registerServiceBuilderForDoc(
+        ampdoc, 'amp-analytics-instrumentation', MockInstrumentation);


Nit: +2 spaces

dreamofabear · 2017-04-22T15:40:59Z

test/functional/test-service.js

+      getService(window, 'a');
+      expect(window.services['a'].obj).to.not.be.null;
+      expect(window.services['a'].build).to.be.null;
+    });


dreamofabear · 2017-04-22T15:49:30Z

src/service.js

-      s.obj = s.build();
-      return s.promise = Promise.resolve(s.obj);
+      if (!s.obj) {
+        // Instantiate service


I think we can remove the conditional and change this comment to "Instantiate service if not already instantiated."

dvoytenko · 2017-04-25T16:30:34Z

src/service.js

 /**
 * 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
 * expected to return the service.
 * @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.


So, we now have getExistingService* for window and embed scope, but not for doc scope? Does this help to conflate registration and use-service cases/APIs?

I noticed this too but looks like performanceForOrNull uses it. If those calls can be changed to performanceFor, then we can remove the getExisting... methods altogether.

Recommend making any additional changes after this PR though.

There are getExistingService*orNull functions, but I've completely removed functions that will fail an assertion if a service doesn't already exist as getServiceInternal now handles that case.

There may not be some forDoc ones as they weren't needed anywhere. I can add some in if you'd like.

As for performaneForOrNull, my understanding is that there are times we don't expect the performance service to exist. If that's not the case then we can remove performanceForOrNull as @choumx suggested.

Yeah, I think performanceForOrNull is used as you describe and thus it's needed. I mostly reacted to absence of getExistingServiceForDoc because I thought we were trying to remove API confusion between service registration and service use. E.g. Viewer is registered only once, but is being used via viewerFor all over the place.

But, I'm ok if we reconsider this in a separate PR.

Callers shouldn't need to worry about registered vs built. I removed those functions since they were redundant with changes made to getServiceInternal.

kmh287 · 2017-04-25T17:52:24Z

@choumx @dvoytenko @jridgewell Thank you all for the very thorough reviews.

kmh287 added PR use: Work in Progress and removed PR use: Work in Progress labels Apr 11, 2017

kmh287 requested review from dvoytenko and dreamofabear April 11, 2017 19:31

kmh287 force-pushed the service_cleanup branch from ce0828f to 2da8398 Compare April 12, 2017 19:26

dreamofabear reviewed Apr 12, 2017

View reviewed changes

dreamofabear reviewed Apr 13, 2017

View reviewed changes

kmh287 mentioned this pull request Apr 17, 2017

Weekly Status 2017-04-17 #8794

Closed

dreamofabear requested changes Apr 19, 2017

View reviewed changes

jridgewell reviewed Apr 20, 2017

View reviewed changes

kmh287 force-pushed the service_cleanup branch from 2fb6ed0 to c85c1fa Compare April 21, 2017 15:32

jridgewell approved these changes Apr 21, 2017

View reviewed changes

kmh287 commented Apr 21, 2017

View reviewed changes

dreamofabear approved these changes Apr 22, 2017

View reviewed changes

kmh287 and others added 11 commits April 24, 2017 09:12

remove unused functions and args from service.js

369a747

merge

bbdc633

merge

ad3ed75

service test fixes

3975138

use getServiceInternal

8a339f4

de-order service.js functions

f8ea1b3

pr comments

1c03019

pr comments

4d20893

pr comments

f0f7aae

cleanup

a4f6a93

pr comments

6b211a1

kmh287 force-pushed the service_cleanup branch from 8942c63 to 6b211a1 Compare April 24, 2017 13:18

dvoytenko suggested changes Apr 25, 2017

View reviewed changes

dvoytenko approved these changes Apr 25, 2017

View reviewed changes

kmh287 merged commit 9041fc1 into ampproject:master Apr 25, 2017

kmh287 mentioned this pull request Apr 25, 2017

Further cleanup of service.js #8946

Closed

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

Service Registration: Cleanup of service.js (ampproject#8698)

c7aced2

ampprojectbot added the PR use: In Beta / Experimental label May 1, 2017

KenneyE pushed a commit to spotxchange/amphtml that referenced this pull request May 3, 2017

Service Registration: Cleanup of service.js (ampproject#8698)

a16f5ed

		@@ -596,8 +512,9 @@ function getServicePromiseOrNullInternal(holder, id) {
		} else {
		dev().assert(s.build,

Service Registration: Cleanup of service.js #8698

Service Registration: Cleanup of service.js #8698

Conversation

kmh287 commented Apr 11, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamofabear Apr 13, 2017 • edited by dvoytenko

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

dreamofabear commented Apr 13, 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

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

jridgewell 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

kmh287 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

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 Apr 25, 2017

kmh287 commented Apr 11, 2017 •

edited

dreamofabear Apr 13, 2017 •

edited by dvoytenko