Amphtml: Further cleanup of service.js

[kmh287]

As discussed on #8698 there are a few more ways to cleanup/simplify service.js

1. Remove the opt_factory param to getService to force usage of the register codepath
2. Eliminate constructor vs factory difference by making all service builders factories. (Dima and I discussed this on hangouts) One potential drawback is that all calls to register services will need to pass in closures.
3. Add getExistingServiceForDocOrNull (even though there aren't necessary uses currently)

/to @choumx @dvoytenko @jridgewell Which of these sound like worthwhile changes to make? Are there any further changes you think would help simplify the file?

[dvoytenko]

Add getExistingServiceForDocOrNull (even though there aren't necessary uses currently)

I think the fact that it's not currently used is a bug :)

[kmh287]

Which services should be using that code path?

[dvoytenko]

I believe all getServiceForDoc in https://github.com/ampproject/amphtml/blob/master/src/services.js should instead be getExistingServiceForDoc. There might not be currently a case for getExistingServiceForDocOrNull, in which case we can forgo. But getServiceForDoc in services.js seems wrong, no?

[kmh287]

I think there are only two different expectations when trying to access a service:

• The service should definitely exist, so you access it through getService{,forDoc}. If the service doesn't exist, a dev assertion will fail.
• The service might not exist, so you access it through getService{,forDoc}orNull If the service doesn't exist, the function will return null.

Previously we had three functions: getService{,forDoc}, getExistingService{,ForDoc}, and getServiceForDoc{,orNull}, but the first and second both failed an assertion if the service didn't exist and no factory nor constructor were provided. Now that we're trying to eliminate passing constructors and factories into getSerice{,ForDoc}, the functions offered identical functionality and one could be eliminated in favor of the other.

[dvoytenko]

I think there are only two different expectations when trying to access a service

That would be nice, but currently there are three in our code:

• getService[ForDoc](node, id) - the service must exist and if not, we will assert.
• getService[ForDoc]OrNull(node, id) - it's ok if service is not there.
• getService[ForDoc](node, id, _factory_) - the service must exist or it will be created otherwise.

The first two is as you describe. The one that I was "complaining" about is the third option. I disliked it b/c I don't believe it'd be a super-rare use, and more likely indicating the inappropriate use than a valid use case.

WDYT?

[kmh287]

I think we're in agreement. I would like to remove the factory param entirely from those methods. :smile: A majority of uses I've found are in tests and should be easily replaceable.

I think that handles points 1 and 3, how about point 2? (Making all service builders factories)

[dvoytenko]

I'd like to resolve #2 and I don't like duality here. And factories are more universal. But I think we need to (a) get the current stats of what we do factory-vs-constructor; and (b) run some jsperf tests. I see that many services are in critical path of rendering so it'd be nice to have it as fast as possible. But I'm not sure what benefit is and how often we'd be able to take advantage of it.

There are other options here as well. So, let's see what we find first.

[kmh287]

a: Very few services are constructed by factory. Ampdoc, History, Viewer, Viewport, Storage, URL Replacement

b: Results.

[jridgewell]

We can rewrite any factories as curried constructors.

---

Your perf test is faulty. You're creating a closure in every loop of the factory test, where as the the constructor test you never create anything. A more realistic tests would involve:

1. Instantiation performance of constructor (your current constructor test, with constructor created in setup)
2. Instantiation performance of factory (both constructor and the factory should be created in setup, not test)
3. Creation performance of constructor (class should be created in test, nothing in setup)
4. Creation performance of factory (class and factory should be created in test, nothing in setup)

Also note, you shouldn't use the class keyword, but a regular function.

[kmh287]

@jridgewell I think I follow your suggestions. PTAL.

[jridgewell]

Almost. "Factory Created in Test" should be creating a Foo2, not a Foo.

[kmh287]

@jridgewell yup, thanks

[dvoytenko]

Curried constructors is an interesting idea. Slightly weird, but I think it'd be overall better comparing to what we have with two args.

[jridgewell]

I think overall, it wouldn't matter if we moved everything to a factory closure. A curried constructor is itself just a closure.

[amphtml-team]

Only for those that require factory, right? Others will just use normal
constructor. But API will be simpler.

On May 2, 2017 2:43 PM, "Justin Ridgewell" [e-mail] wrote:

I think overall, it wouldn't matter if we moved everything to a factory
closure. A curried constructor is itself just a closure.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/ampproject/amphtml/issues/8946#issuecomment-298770283,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ANd2kKccnCC9bpvRung0xHmaMGc2zZCDks5r16NxgaJpZM4NIAHX
.

--
You received this message because you are subscribed to the Google Groups
"amphtml-eng-github" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/a/
google.com/d/msgid/amphtml-eng-github/ampproject/amphtml/
issues/8946/298770283%40github.com
https://groups.google.com/a/google.com/d/msgid/amphtml-eng-github/ampproject/amphtml/issues/8946/298770283%40github.com?utm_medium=email&utm_source=footer
.

--
You received this message because you are subscribed to the Google Groups
"amphtml-eng" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/a/
google.com/d/msgid/amphtml-eng/ampproject/amphtml/issues/
8946/298770283%40github.com
https://groups.google.com/a/google.com/d/msgid/amphtml-eng/ampproject/amphtml/issues/8946/298770283%40github.com?utm_medium=email&utm_source=footer
.

[kmh287]

Also currying constructors works on the assumption that you know what all the arguments are going to be at registration time.

If curried constructors are really just factories as @jridgewell noted, then I don't see a reason not to just convert all service registrations to use factories rather than constructors.

[choumx]

This is done.