Send link relations in documentLoaded message by muxin · Pull Request #7142 · ampproject/amphtml

muxin · 2017-01-21T01:38:35Z

cramforce · 2017-01-23T15:51:23Z

src/service/document-info-impl.js

+        }
+        const rel = n.getAttribute('rel');
+        const href = n.getAttribute('href');
+        if (!rel || !href || rel == 'prefetch' || rel == 'preload' ||


Also dns-prefetch

cramforce · 2017-01-23T15:52:05Z

src/service/document-info-impl.js

+          continue;
+        }
+        const rel = n.getAttribute('rel');
+        const href = n.getAttribute('href');


If you read .href instead of using getAttribute you are getting a full resolved URL and can skip the expensive parseUrl below.

cramforce · 2017-01-23T15:52:33Z

src/service/document-info-impl.js

    }
    const pageViewId = getPageViewId(ampdoc.win);
+
+    const doc = ampdoc.win.document;


Factor this into a function

Something like getGlobalDoc ? Why do we need factoring?

cramforce · 2017-01-23T15:53:56Z

src/service/document-info-impl.js

+            rel == 'preconnect') {
+          continue;
+        }
+        linkRels[rel] = parseUrl(href).href;


I think we need to upgrade to an array, if there are more than 1 value for the same rel

Yeah, I will make linkRels type of !Object<string, !Array<string>>

cramforce · 2017-01-23T20:06:12Z

src/service/document-info-impl.js

    }
    const pageViewId = getPageViewId(ampdoc.win);
+
+    const doc = ampdoc.win.document;


I meant to factor all the following code into a function like getLinkRels

jridgewell · 2017-01-23T22:53:09Z

src/service/document-info-impl.js

+
+    const doc = ampdoc.win.document;
+    if (doc.head) {
+      for (let n = doc.head.firstElementChild; n; n = n.nextElementSibling) {


A querySelectorAll might be more appropriate.

muxin · 2017-01-24T19:50:55Z

src/service/document-info-impl.js

@@ -73,13 +78,31 @@ export class DocInfo {
          : sourceUrl;
    }


The canonicalUrl in docInfo could be rootNode.AMP.canonicalUrl or parseUrl(rootNode.querySelector('link[rel=canonical]').href).href or sourceUrl. Shall I make the linkRels['canonical'] the same logic?

I'll keep linkRels['canonical'] to be the value in the link tag

cramforce · 2017-01-24T23:17:36Z

src/service/document-info-impl.js

+    const links = doc.head.querySelectorAll('link');
+    for (let i = 0; i < links.length; i++) {
+      const link = links[i];
+      const rel = link.getAttribute('rel');


need to handle the case where the rel has multiple mappings to the same href.

<link rel="sharelink canonical">

split(/\s+/)

muxin · 2017-01-25T01:45:01Z

@cramforce PTAL

jridgewell · 2017-01-25T19:48:37Z

src/service/document-info-impl.js

+function getLinkRels(doc) {
+  const linkRels = map();
+  if (doc.head) {
+    const links = doc.head.querySelectorAll('link');


You can narrow down further by using link[rel].

jridgewell · 2017-01-25T19:50:37Z

src/service/document-info-impl.js

+          if (!isArray(linkRels[rel])) {
+            const firstHref = linkRels[rel];
+            linkRels[rel] = [];
+            linkRels[rel].push(firstHref);


Nit: we can combine the above statements:

linkRels[rel] = [linkRels[rel]];

thanks for simplifying this~ done

jridgewell · 2017-01-25T19:51:34Z

src/service/document-info-impl.js

+      }
+
+      rels.split(/\s+/).forEach(rel => {
+        if (rel == 'prefetch' || rel == 'preload' || rel == 'preconnect' ||


Should we hoist this to a const array, and do an #indexOf search?

Good idea. Done

jridgewell · 2017-01-25T22:47:24Z

src/service/document-info-impl.js

 import {map} from '../utils/object';
 import {isArray} from '../types';

+/** @private @const {Array<string>} */


{!Array<string>}

jridgewell · 2017-01-25T22:49:04Z

src/service/document-info-impl.js

-        if (rel == 'prefetch' || rel == 'preload' || rel == 'preconnect' ||
-            rel == 'dns-prefetch') {
-          return;
+        for (let i = 0; i < filteredLinkRels.length; i++) {


No need for this for loop, just the body.

cramforce · 2017-01-25T22:52:46Z

src/service/document-info-impl.js

+      }
+
+      rels.split(/\s+/).forEach(rel => {
+        for (let i = 0; i < filteredLinkRels.length; i++) {


Check if rel is truthy. This may produce empty strings if there is leading or trailing whitespace in a value.

Going to adopt what @jridgewell suggested which will avoid the truth check

if (filteredLinkRels.indexOf(rel) != -1) { return; }

cramforce · 2017-01-25T22:54:04Z

src/service/document-info-impl.js

+          }
+        }
+
+        if (!linkRels[rel]) {


You can assign the existing value to a variable and then avoid a few lookups of it further down.

jridgewell · 2017-01-25T23:38:21Z

src/service/document-info-impl.js

-          if (!isArray(linkRels[rel])) {
-            linkRels[rel] = [linkRels[rel]];
+          if (!isArray(value)) {
+            linkRels[rel] = [value];


If we want to get super nit picky:

let value = linkRels[rel]; if (!isArray) { value = linkRels[rel] = [value]; } value.push(href);

jridgewell · 2017-01-25T23:38:30Z

src/service/document-info-impl.js


-        if (!linkRels[rel]) {
+        const value = linkRels[rel];
+        if (!value) {


Nit: Please flip the conditionals.

muxin assigned cramforce Jan 21, 2017

muxin requested a review from cramforce January 21, 2017 01:38

cramforce requested changes Jan 23, 2017

View reviewed changes

cramforce reviewed Jan 23, 2017

View reviewed changes

jridgewell reviewed Jan 23, 2017

View reviewed changes

muxin commented Jan 24, 2017

View reviewed changes

muxin added 2 commits January 24, 2017 13:26

Send link relations in documentLoaded message

bb14bc3

Make linkRels's value to be array

0f96874

muxin force-pushed the link-rels branch from f1481b0 to 0f96874 Compare January 24, 2017 21:27

cramforce reviewed Jan 24, 2017

View reviewed changes

muxin added 2 commits January 24, 2017 16:08

Make linkRels's value to be string or array

4778443

Handle case for <link rel='foo bar' href='same.html'>

6383011

jridgewell reviewed Jan 25, 2017

View reviewed changes

Addressed comments

42ab05b

muxin force-pushed the link-rels branch from ff4b020 to 42ab05b Compare January 25, 2017 21:57

jridgewell requested changes Jan 25, 2017

View reviewed changes

cramforce requested changes Jan 25, 2017

View reviewed changes

Addressed comments

91f8c7c

jridgewell approved these changes Jan 25, 2017

View reviewed changes

nit

040bba1

cramforce approved these changes Jan 26, 2017

View reviewed changes

muxin merged commit e3ca900 into ampproject:master Jan 26, 2017

muxin deleted the link-rels branch January 26, 2017 01:42

jridgewell pushed a commit to jridgewell/amphtml that referenced this pull request Jan 31, 2017

Send link relations in documentLoaded message (ampproject#7142)

0e3de91

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

Send link relations in documentLoaded message (ampproject#7142)

556b001

Conversation

muxin commented Jan 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muxin commented Jan 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone