Service cleanup: Require a constructor for all services #9212

kmh287 · 2017-05-08T21:21:55Z

This PR removes the optional constructor or optional factory from service registration and instead requires one function that will always be called with new. Services that were previously built with factories pass those factories in as this function. These factories will work identically as before with one caveat: they must not return primitives.

const f = function() {
  return 'Hello World'
}

f()     // 'Hello World'
new f() // {}

This was only a problem in our tests, which needed to be changed to ensure fake factories always returned an object instead of primitives.

/to @dvoytenko @jridgewell I think this is more what you meant on #8946 and #9141 (comment) though I'm worried about the following scenario.

const g = () => 'Hello World'
g()     // 'Hello World'
new g() // TypeError: h is not a constructor

Is this something we should worry about? I can do this in the developer console, but as I write this all tests are passing, including ones that test services that are constructed via factory.

jridgewell · 2017-05-09T17:22:39Z

So the ES6 spec says you can't new an arrow function. Luckily for us, none of the compilers enforce this (it's an expensive runtime check). We might be able to add a closure type check to ensure every registered service uses a regular function.

kmh287 · 2017-05-10T15:30:41Z

I've gone through the services changed in this PR and replaced all arrow functions with regular functions. One factory case still remains: there are some instances where getService is called with an optional factory, circumventing the registration codepath. I'm going to fix this first in another PR to prevent adding a bunch of new files to this review.

jridgewell · 2017-05-10T17:36:12Z

src/service/viewport-impl.js

-      /* instantiate */ true);
+  registerServiceBuilderForDoc(ampdoc,
+      'viewport',
+      function(ampdoc) {


Nit: Can this just be createViewport?

Ditto here, as with createHistory

whoops, done for both

kmh287 · 2017-05-10T17:37:09Z

Blocked on #9248

dvoytenko · 2017-05-10T21:23:23Z

src/runtime.js

@@ -525,13 +523,12 @@ function prepareAndRegisterServiceForDoc(global, extensions,
 * @param {!Window} global
 * @param {!./service/extensions-impl.Extensions} extensions
 * @param {string} name
- * @param {function(new:Object, !./service/ampdoc-impl.AmpDoc)=} opt_ctor
- * @param {function(!./service/ampdoc-impl.AmpDoc):!Object=} opt_factory
+ * @param {!function(new:Object, !./service/ampdoc-impl.AmpDoc)} ctor


Oops. Do we need to think through backward compatibility? We usually provide the backward compatibility between two PROD versions. E.g. new v0.js should work with the old amp-carousel.js

Should I revert changes in runtime.js and service.js for now and then resubmit after two releases?

Can we figure out how to make this a bi-directional code in runtime.js only? Ideally, service doesn't need to do that. Or at the worst, you may need to preserve build() function that you had before?

Ping me to discuss if needed. It is somewhat important. It should also come with TODO and a cleanup task.

@dvoytenko Okay, I think I've found a way to do that. PTAL.

dvoytenko · 2017-05-10T21:31:20Z

src/service/history-impl.js

@@ -866,6 +866,7 @@ export function installHistoryServiceForDoc(ampdoc) {
  registerServiceBuilderForDoc(
      ampdoc,
      'history',
-      /* opt_constructor */ undefined,
-      ampdoc => createHistory(ampdoc));
+      function(ampdoc) {


Can't this be just registerServiceBuilderForDoc(..., createHistory)? Why reclosure the function with same args?

dvoytenko · 2017-05-10T21:32:03Z

src/service/viewport-impl.js

-      /* instantiate */ true);
+  registerServiceBuilderForDoc(ampdoc,
+      'viewport',
+      function(ampdoc) {


Ditto here, as with createHistory

dvoytenko · 2017-05-15T17:43:32Z

Sorry. Lost it. Looking now.

dvoytenko · 2017-05-15T17:47:37Z

src/runtime.js

@@ -545,12 +545,18 @@ function prepareAndRegisterServiceForDocShadowMode(global, extensions,
 * @param {function(!./service/ampdoc-impl.AmpDoc):!Object=} opt_factory
 */
 function registerServiceForDoc(ampdoc, name, opt_ctor, opt_factory) {
+  // TODO(kmh287, #9292): Refactor to remove opt_factory param once #9212 has been


This looks great! Please also add TODO(kmh287, #9292) with the corresponding explanation of what will be done to prepareAndRegisterServiceForDoc and prepareAndRegisterServiceForDocShadowMode.

dvoytenko · 2017-05-15T17:48:03Z

Looks great! Please just update more TODOs as I commented above.

kmh287 · 2017-05-15T18:15:19Z

Thank you @dvoytenko and @jridgewell for the reviews. I will merge after #9248 has been merged

kmh287 added the PR use: Work in Progress label May 8, 2017

googlebot added the cla: yes label May 8, 2017

kmh287 requested review from dvoytenko and jridgewell May 8, 2017 23:14

kmh287 removed the PR use: Work in Progress label May 8, 2017

kmh287 changed the title ~~[WIP] Service cleanup: Require a constructor for all services~~ Service cleanup: Require a constructor for all services May 8, 2017

jridgewell approved these changes May 10, 2017

View reviewed changes

dvoytenko suggested changes May 10, 2017

View reviewed changes

kmh287 mentioned this pull request May 12, 2017

Cleanup opt_ctor and opt_factory params in runtime.js #9292

Closed

dvoytenko reviewed May 15, 2017

View reviewed changes

dvoytenko approved these changes May 15, 2017

View reviewed changes

kmh287 force-pushed the service_cleanup4 branch from 733f50e to 43e1c8e Compare May 15, 2017 18:01

kmh287 and others added 11 commits May 15, 2017 15:03

change factories to constructors

3110cc8

lint and type check cleanup

e807d09

fix some tests

2517cc8

fix tests

7e8e9ef

more test fixes

82a239c

convert arrow functions to regular functions

9c58fcc

cleanup

0cd8b1c

dima's comment

30ba682

pr comments

e66efd3

fix todo

5090e3d

pr comment

0c109a1

kmh287 force-pushed the service_cleanup4 branch from 43e1c8e to 0c109a1 Compare May 15, 2017 19:16

cleanup

0fb3c39

kmh287 merged commit 28a052b into ampproject:master May 15, 2017

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels May 23, 2017

ampprojectbot mentioned this pull request Jul 6, 2020

🚨 Error: Cumulative Layout Shift not supported #29151

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Service cleanup: Require a constructor for all services #9212

Service cleanup: Require a constructor for all services #9212

kmh287 commented May 8, 2017 •

edited

Loading

jridgewell commented May 9, 2017

kmh287 commented May 10, 2017

jridgewell May 10, 2017

dvoytenko May 10, 2017

kmh287 May 11, 2017 •

edited

Loading

kmh287 commented May 10, 2017

dvoytenko May 10, 2017

kmh287 May 10, 2017

dvoytenko May 11, 2017

kmh287 May 12, 2017

dvoytenko May 10, 2017

dvoytenko May 10, 2017

dvoytenko commented May 15, 2017

dvoytenko May 15, 2017

kmh287 May 15, 2017

dvoytenko commented May 15, 2017

kmh287 commented May 15, 2017

Service cleanup: Require a constructor for all services #9212

Service cleanup: Require a constructor for all services #9212

Conversation

kmh287 commented May 8, 2017 • edited Loading

jridgewell commented May 9, 2017

kmh287 commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmh287 May 11, 2017 • edited Loading

Choose a reason for hiding this comment

kmh287 commented May 10, 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

dvoytenko commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko commented May 15, 2017

kmh287 commented May 15, 2017

kmh287 commented May 8, 2017 •

edited

Loading

kmh287 May 11, 2017 •

edited

Loading