Add support for cache:getClientId #14450

RJSumi · 2018-04-05T21:46:08Z

Implements support for cache:getClientId via a new class, CacheCidApi.

Fixes #13118

…he-served pages without a viewer.

…he CCT-embedded implementations are just for the interim.

dreamofabear

Overall looks good! Needs unit tests for cache-cid-api.js.

dreamofabear · 2018-04-09T19:12:13Z

src/service/cache-cid-api.js

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ * amp-lUSTQLTz0csodkt327ZRHm30PG-6sJOuZwTh-PPmzqRMGZrTCRBMJ8Pfdw8f_UGC
+ * amp-lUSTQLTz0csodkt327ZRHm30PG-6sJOuZwTh-PPmzqRMGZrTCRBMJ8Pfdw8f_UGC


dreamofabear · 2018-04-09T19:12:43Z

src/service/cache-cid-api.js

+const TAG = 'CacheCidApi';
+const CACHE_API_URL = 'https://ampcid.google.com/v1/cache:getClientId?key=';
+
+const TIMEOUT = 30000;


Please add JSDoc for all of these constants.

dreamofabear · 2018-04-09T19:13:04Z

src/service/cache-cid-api.js

+ */
+export class CacheCidApi {
+
+  constructor(ampdoc) {


JSDoc please.

dreamofabear · 2018-04-09T19:13:39Z

src/service/cache-cid-api.js

+   * @return {!Promise<?string>}
+   */
+  getScopedCid(scope) {
+    // promise for whether we do stuff at all


Not sure what this comment means.

Sorry, it was a draft holdover from when the structure of the method was more confusing and I was starting comment stubs.

dreamofabear · 2018-04-09T19:37:43Z

src/service/cache-cid-api.js

+              // The client is still responsible for appending API keys to the URL.
+              const alt = `${response['alternateUrl']}?key=${SERVICE_KEY_}`;
+              return this.fetchCid_(dev().assertString(alt))
+                  .then(altRes => {


Rather than dupe this logic, let's add an optional param to fetchCid_ that checks for alternateUrl and defaults to true. Then pass false for this invocation.

Done, thanks.

dreamofabear · 2018-04-09T19:39:06Z

src/service/cache-cid-api.js

+import {dict} from '../utils/object';
+import {getSourceOrigin} from '../url';
+
+const GOOGLE_CLIENT_ID_API_META_NAME = 'amp-google-client-id-api';


Let's be consistent wrt "CLIENT_ID" vs "CID" -- GOOGLE_CID_API_META_NAME.

dreamofabear · 2018-04-09T19:39:33Z

src/service/cache-cid-api.js

+   * Returns scoped CID extracted from the fetched publisherCid.
+   * @param {string} publisherCid
+   * @param {string} scope
+   * @return {?string}


This returns !Promise<string>, right?

Yes, thanks.

dreamofabear · 2018-04-09T19:39:56Z

src/service/cache-cid-api.js

+   * @return {?string}
+   */
+  scopeCid_(publisherCid, scope) {
+    if (!publisherCid) {


{string} is non-nullable by default. Is it possible for this to be empty string or should we change the type?

fetchCid_ returns a nullable string, which used to propagate to this method. I moved the null check to the call site.

dreamofabear · 2018-04-09T19:41:40Z

src/service/cache-cid-api.js

+  }
+
+  /**
+   * @return {!Object<string, string>}


Can you add a description with an example of what the <meta> tag can looks like here?

Is this what you meant?

dreamofabear · 2018-04-09T19:43:37Z

BTW, an editor with eslint plugin configured will save a lot of time compared to gulp lint. SublimeText and VSCode are good choices.

dreamofabear · 2018-04-10T04:50:23Z

Just merged #14224, please merge latest changes from master.

dreamofabear · 2018-04-10T19:20:42Z

src/service/cache-cid-api.js

+  /**
+   * @return {!Object<string, string>}
+   */
+  getOptedInScopes_() {


Is this verbatim from viewer-cid-api.js? Can it live in cid-impl.js instead and be called at time of invocation of isScopeOptedIn and passed in as a param?

… of lint errors

…nd viewer APIs

dreamofabear

Looks good overall.

dreamofabear · 2018-04-10T22:05:58Z

src/service/cache-cid-api.js

+   * Returns scoped CID retrieved from the Viewer.
+   * @param {string|undefined} apiKey
+   * @param {string} scope
+   * @return {!Promise<string>}


!Promise<?string>

dreamofabear · 2018-04-10T22:07:32Z

src/service/cid-impl.js

@@ -94,12 +123,20 @@ export class Cid {
     */
    this.externalCidCache_ = Object.create(null);

+    /**
+     * @private {!CacheCidApi}


Add @const.

dreamofabear · 2018-04-10T22:08:21Z

src/service/cid-impl.js

@@ -175,8 +212,8 @@ export class Cid {
    const scope = getCidStruct.scope;
    /** @const {!Location} */
    const url = parseUrl(this.ampdoc.win.location.href);
+    const apiKey = this.isScopeOptedIn_(scope);


Let's only call this if necessary to avoid wasted work.

dreamofabear · 2018-04-10T22:11:49Z

test/functional/test-cache-cid-api.js

+const SERVICE_KEY_ = 'AIzaSyDKtqGxnoeIqVM33Uf7hRSa3GJxuzR7mLc';
+const API_KEY = 'AIzaSyA65lEHUEizIsNtlbNo-l2K18dT680nsaM';
+const TEST_CID_ =
+      'amp-mJW1ZjoviqBJydzRI8KnitWEpqyhQqDegGClrvvfkCif_N9oYLdZEB976uJDhYgL';


Let's not use the actual keys in tests if possible. If we need to change them in the future then it would appear we'd need to update these tests too. Something readable like my-service-key and my-api-key would suffice.

dreamofabear · 2018-04-10T22:13:04Z

test/functional/test-cache-cid-api.js

+      return api.getScopedCid(API_KEY, 'AMP_ECID_GOOGLE').then(cid => {
+        expect(cid).to.equal(TEST_CID_);
+        expect(fetchJsonStub)
+            .to.be.calledWith(`https://ampcid.google.com/v1/cache:getClientId?key=${SERVICE_KEY_}`,


Just use the actual string literal for readability. ToTT issue 510. 😉

dreamofabear · 2018-04-10T22:20:12Z

test/functional/test-cid.js

@@ -795,7 +800,8 @@ describes.realWin('cid', {amp: true}, env => {

    beforeEach(() => {
      sandbox.stub(url, 'isProxyOrigin').returns(false);
-      sandbox.stub(cid.viewerCidApi_, 'isScopeOptedIn').returns('api-key');
+      ampdoc.win.document.head.innerHTML +=
+          '<meta name="amp-google-client-id-api" content="googleanalytics">';


Is this change (and line 821) necessary? It's better to avoid mutating the document during tests (e.g. can break other tests if you forget to revert in afterEach).

I did notice that this is patterned off of test-viewer-cid-api.js. ☹️

Yeah, I don't think it'll be easy to avoid adding to the document, given that the behaviour under test is supposed to extract the configuration from it.

dreamofabear · 2018-04-10T22:22:38Z

test/functional/test-cid.js

@@ -44,6 +44,8 @@ import {stubServiceForDoc} from '../../testing/test-helper';

 const DAY = 24 * 3600 * 1000;

+const API_KEY = 'AIzaSyA65lEHUEizIsNtlbNo-l2K18dT680nsaM';


Let's remove this and inline it in the original isScopedOptedIn_ tests you moved.

dreamofabear · 2018-04-10T22:38:14Z

test/functional/test-cache-cid-api.js

+    });
+  });
+
+  describe('getScopedCid', () => {


Testing the timeout logic would be nice. You can use sinon's fake clock to simulate 30s passing synchronously.

Done, and this revealed a promise chaining bug (catch block didn't catch the timeout because it was chained to the XHR).

dreamofabear · 2018-04-10T22:46:28Z

/cc @jridgewell Another unavoidable size bump FYI. We could move viewer-cid-api.js to amp-viewer-integration but I don't think it'll help much.

What's interesting is that the browserify build increased by 0.1KB while the minified build increased by 0.26KB. Looks like the obfuscation impeded compressibility.

…cid-api

dreamofabear · 2018-04-11T04:43:12Z

From Travis, looks like Viewer#isProxyOrigin() function might've been lost in a merge. You can also run gulp check-types locally.

jridgewell

So much duplication between this and https://github.com/ampproject/amphtml/blob/9b101fa000b7dc911073cacdf3ccafb187c2012d/src/service/cid-api.js.

And, there are now 4 different CID api files:

src/service/cid-api.js
src/service/cid-impl.js
src/service/viewer-cid-api.js
src/service/cache-cid-api.js

What's the difference? What is the responsibility of each file?

/cc @lannka

jridgewell · 2018-04-12T21:53:37Z

src/service/cid-impl.js

+    if (this.cacheCidApi_.isSupported()) {
+      const apiKey = this.isScopeOptedIn_(scope);
+      if (!apiKey) {
+        return /** @type {!Promise<?string>} */ (Promise.resolve(null));


Why can't CCT fall back to normal cids?

jridgewell · 2018-04-12T21:54:12Z

src/service/cache-cid-api.js

+   */
+  getScopedCid(scope) {
+    if (!this.viewer_.isCctEmbedded()) {
+      return /** @type {!Promise<?string>} */ (Promise.resolve(null));


Throw an error.

jridgewell · 2018-04-12T21:57:45Z

src/service/cache-cid-api.js

+          body: payload,
+        })).then(res => {
+      return res.json().then(response => {
+        if (response['optOut']) {


This entire function's whitespace is borked.

jridgewell · 2018-04-12T21:58:31Z

src/service/cache-cid-api.js

+          // If an alternate url is provided, try again with the alternate url
+          // The client is still responsible for appending API keys to the URL.
+          const alt = `${response['alternateUrl']}?key=${SERVICE_KEY_}`;
+          return this.fetchCid_(dev().assertString(alt), false);


assertString is unnecessary.

* Add a class to interface with the new Google CID API endpoint for cache-served pages without a viewer. * fix lint errors * assorted fixes based on testing * added TODOs to indicate that this will merge with #14224. The CCT-embedded implementations are just for the interim. * unit test stuff * add unit tests for cache cid api * documentation fixes * fix broken partial change * fix secretly broken unit test and copied naming and introduce a bunch of lint errors * fix lint errors * move isScopeOptedIn into cid-impl rather than duplicating for cache and viewer APIs * fix unit tests * Re-add lost size cap bump. * address comments and fix unit tests * fix type errors and missing viewer.isProxyOrigin * Update size caps again.

lannka · 2018-05-10T21:04:18Z

@jridgewell agree with you. This new file can be merged into cid-api.js

The way the code was organized:

cid-impl.js  -> cid-api.js (Get CID from CID API)
             -> viewer-cid-api.js (Get CID from Viewer)

RJSumi added 4 commits March 29, 2018 11:47

Add a class to interface with the new Google CID API endpoint for cac…

c0c41a9

…he-served pages without a viewer.

fix lint errors

51a5c97

assorted fixes based on testing

c66e325

added TODOs to indicate that this will merge with ampproject#14224. T…

ecb8f53

…he CCT-embedded implementations are just for the interim.

RJSumi requested review from zhouyx and dreamofabear April 5, 2018 21:46

googlebot added the cla: yes label Apr 5, 2018

unit test stuff

8f93512

dreamofabear reviewed Apr 9, 2018

View reviewed changes

add unit tests for cache cid api

2f1d7d1

dreamofabear reviewed Apr 10, 2018

View reviewed changes

RJSumi added 7 commits April 10, 2018 15:23

documentation fixes

3901674

fix broken partial change

435002d

fix secretly broken unit test and copied naming and introduce a bunch…

723e78b

… of lint errors

fix lint errors

526fabf

move isScopeOptedIn into cid-impl rather than duplicating for cache a…

866c02b

…nd viewer APIs

fix unit tests

3edce59

resolve merge conflicts

9be9045

dreamofabear approved these changes Apr 10, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into cache-cid-api

21a6c46

RJSumi force-pushed the cache-cid-api branch from d81250e to 21a6c46 Compare April 10, 2018 23:04

William Chou and others added 3 commits April 10, 2018 22:50

Re-add lost size cap bump.

4bfc426

address comments and fix unit tests

0b3154f

Merge branch 'cache-cid-api' of github.com:rjsumi/amphtml into cache-…

8748631

…cid-api

RJSumi and others added 2 commits April 11, 2018 12:24

fix type errors and missing viewer.isProxyOrigin

4d03236

Merge branch 'master' into cache-cid-api

747f77f

Update size caps again.

b1ca3fe

dreamofabear mentioned this pull request Apr 11, 2018

Move surface-specific code out of "src" #14559

Closed

Merge branch 'master' into cache-cid-api

bd8683c

dreamofabear merged commit 82fd3a1 into ampproject:master Apr 11, 2018

jridgewell reviewed Apr 12, 2018

View reviewed changes

rsimha mentioned this pull request May 4, 2018

Leaner AMP: Stop the bleeding (feeding?) #14394

Closed

		@@ -44,6 +44,8 @@ import {stubServiceForDoc} from '../../testing/test-helper';

		const DAY = 24 * 3600 * 1000;

		const API_KEY = 'AIzaSyA65lEHUEizIsNtlbNo-l2K18dT680nsaM';

Add support for cache:getClientId #14450

Add support for cache:getClientId #14450

Conversation

RJSumi commented Apr 5, 2018 • edited

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

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 commented Apr 9, 2018

dreamofabear commented Apr 10, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamofabear commented Apr 10, 2018 • edited

dreamofabear commented Apr 11, 2018

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

lannka commented May 10, 2018

RJSumi commented Apr 5, 2018 •

edited

dreamofabear commented Apr 10, 2018 •

edited