Filter duplicate extensions by src URL by sebastianbenz · Pull Request #1192 · ampproject/amp-toolbox

sebastianbenz · 2021-04-06T18:19:22Z

westonruter · 2021-04-06T19:26:26Z

Testing PHP implementation: ampproject/amp-toolbox-php#130

westonruter · 2021-04-06T19:48:44Z

...s/optimizer/spec/transformers/valid/ReorderHeadTransformer/supports_esm/expected_output.html

+  <link rel="preload" href="https://cdn.ampproject.org/v0.css" as="style">
+  <link href="https://fonts.googleapis.com/css?foobar" rel="stylesheet" type="text/css">
+  <link as="script" crossorigin="anonymous" href="https://cdn.ampproject.org/v0.mjs" rel="modulepreload">


Shouldn't the link[rel=preload] and link[rel=modulepreload] be moved to the beginning of the head?

westonruter · 2021-04-06T20:15:23Z

Here's the current order in the PHP implementation:

  <link as="script" crossorigin="anonymous" href="https://cdn.ampproject.org/v0.mjs" rel="modulepreload">
  <link rel="preload" href="https://cdn.ampproject.org/v0.css" as="style">
  <link rel="stylesheet" href="https://cdn.ampproject.org/v0.css">
  <script async nomodule src="https://cdn.ampproject.org/v0.js"></script>
  <script async src="https://cdn.ampproject.org/v0.mjs" type="module" crossorigin="anonymous"></script>
  <script async nomodule src="https://cdn.ampproject.org/v0/amp-experiment-0.1.js" custom-element="amp-experiment"></script>
  <script async custom-element="amp-experiment" src="https://cdn.ampproject.org/v0/amp-experiment-0.1.mjs" type="module" crossorigin="anonymous"></script>
  <script async="" nomodule="" src="https://cdn.ampproject.org/v0/amp-mustache-0.2.js" custom-template="amp-mustache"></script>
  <script async="" custom-template="amp-mustache" src="https://cdn.ampproject.org/v0/amp-mustache-0.2.mjs" type="module" crossorigin="anonymous"></script>
  <link href="https://example.com/favicon.ico" rel="icon">
  <link href="https://fonts.googleapis.com/css?foobar" rel="stylesheet" type="text/css">

When outputting AMP scripts, it seems that the module/nomodule versions should be next to each other in general, right? Also, it seems the module version should always come first? That isn't yet the case for the PHP transformer.

sebastianbenz · 2021-04-06T20:33:44Z

When outputting AMP scripts, it seems that the module/nomodule versions should be next to each other in general, right? Also, it seems the module version should always come first? That isn't yet the case for the PHP transformer.

Not sure if that really makes a difference. I'd assume that the browser will simply ignore the nomodule tag and move on.

westonruter · 2021-04-06T20:48:23Z

...s/optimizer/spec/transformers/valid/ReorderHeadTransformer/supports_esm/expected_output.html

+  <script async custom-template="amp-mustache" src="https://cdn.ampproject.org/v0/amp-mustache-0.2.mjs" type="module" crossorigin="anonymous"></script>
+  <link href="https://example.com/favicon.ico" rel="icon">
+  <link href="https://fonts.googleapis.com/css?foobar" rel="stylesheet" type="text/css">
+  <link as="script" crossorigin="anonymous" href="https://cdn.ampproject.org/v0.mjs" rel="modulepreload">


This modulepreload link needs to be moved up along with the preload as well. Ref ampproject/amp-toolbox-php@a3a99fd

Thanks for the patience taking it up with my empty brain today...

westonruter · 2021-04-06T22:43:16Z

packages/optimizer/spec/transformers/valid/ReorderHeadTransformer/supports_esm/input.html

+<!doctype html>
+<html ⚡>
+<head>
+  <script async nomodule src="https://cdn.ampproject.org/v0.js"></script>


Big problem: The nomodule v0.js script is getting dropped in the output. Here's one thing I had to do in the PHP version to account for this: ampproject/amp-toolbox-php@4bad718

...s/optimizer/spec/transformers/valid/ReorderHeadTransformer/supports_esm/expected_output.html

+  <link as="script" crossorigin="anonymous" href="https://cdn.ampproject.org/v0.mjs" rel="modulepreload">
+  <link rel="preload" href="https://cdn.ampproject.org/v0.css" as="style">
+  <link rel="stylesheet" href="https://cdn.ampproject.org/v0.css">
+  <script async src="https://cdn.ampproject.org/v0.mjs" type="module" crossorigin="anonymous"></script>


westonruter · 2021-04-06T22:55:36Z

Not sure if that really makes a difference. I'd assume that the browser will simply ignore the nomodule tag and move on.

Can we go ahead and do that so that there is consistency in the ordering? I've implemented this in the PHP version now. The PHP version now generates the head as follows for ReorderHeadTransformer/supports_esm/input.html:

  <link as="script" crossorigin="anonymous" href="https://cdn.ampproject.org/v0.mjs" rel="modulepreload">
  <link rel="preload" href="https://cdn.ampproject.org/v0.css" as="style">
  <link rel="stylesheet" href="https://cdn.ampproject.org/v0.css">
  <script async src="https://cdn.ampproject.org/v0.mjs" type="module" crossorigin="anonymous"></script>
  <script async nomodule src="https://cdn.ampproject.org/v0.js"></script>
  <script async custom-element="amp-experiment" src="https://cdn.ampproject.org/v0/amp-experiment-0.1.mjs" type="module" crossorigin="anonymous"></script>
  <script async nomodule src="https://cdn.ampproject.org/v0/amp-experiment-0.1.js" custom-element="amp-experiment"></script>
  <script async custom-template="amp-mustache" src="https://cdn.ampproject.org/v0/amp-mustache-0.2.mjs" type="module" crossorigin="anonymous"></script>
  <script async nomodule src="https://cdn.ampproject.org/v0/amp-mustache-0.2.js" custom-template="amp-mustache"></script>
  <link href="https://example.com/favicon.ico" rel="icon">
  <link href="https://fonts.googleapis.com/css?foobar" rel="stylesheet" type="text/css">

This ordering aligns with the AMP boilerplate usage as well, where we have style[amp-boilerplate] followed by noscript > noscript[amp-boilerplate]. In general it seems preferable for the thing which is the normal case to be first. For extremely slow connections, the module version could be loaded sooner than the non-module version if the HTML loading gets interrupted.

westonruter · 2021-04-06T22:57:38Z

The PHP version is also sorting the script components by name, so amp-experiment script(s) will come before amp-mustache for example.

westonruter · 2021-04-06T23:32:19Z

packages/optimizer/lib/transformers/ReorderHeadTransformer.js

  _removeDuplicateCustomExtensions(extensions) {
-    const nodesByName = new Map();
+    const nodesBySrc = new Map();
    for (const node of extensions) {
-      const name = this._getName(node);
-      nodesByName.set(name, node);
+      const src = node.attribs['src'];
+      nodesBySrc.set(src, node);
    }
-    return Array.from(nodesByName.values());
+    return Array.from(nodesBySrc.values());
  }


I took an alternative approach to this in PHP, to do the deduplication in the registerScript function itself: https://github.com/ampproject/amp-toolbox-php/pull/130/files#diff-b0da2cc892f74bf1ebc7a84e63d7065894b24541a48f3a26089f23860ee1209fR156-R190

This also accounts for module and nomodule variations.

schlessera · 2021-04-07T07:01:39Z

packages/optimizer/lib/transformers/ReorderHeadTransformer.js

    for (const node of extensions) {
-      const name = this._getName(node);
-      nodesByName.set(name, node);
+      const src = node.attribs['src'];


Shouldn't this remove the host to make sure a same extension is not downloaded twice from two different hosts?

westonruter · 2021-04-07T15:52:47Z

...s/optimizer/spec/transformers/valid/ReorderHeadTransformer/supports_esm/expected_output.html

+  <script async nomodule src="https://cdn.ampproject.org/v0/amp-mustache-0.2.js" custom-template="amp-mustache"></script>
+  <script async custom-template="amp-mustache" src="https://cdn.ampproject.org/v0/amp-mustache-0.2.mjs" type="module" crossorigin="anonymous"></script>


Can the order be reversed, with the nomodule one coming after?

westonruter · 2021-04-07T15:53:21Z

packages/optimizer/lib/transformers/ReorderHeadTransformer.js

  }

  _registerScript(node) {
+    const moduleIndex = hasAttribute(node, 'nomodule') ? 0 : 1;


This should ensure nomodule comes after type=module.

Suggested change

const moduleIndex = hasAttribute(node, 'nomodule') ? 0 : 1;

const moduleIndex = hasAttribute(node, 'nomodule') ? 1 : 0;

sebastianbenz

Addressed the feedback. Scripts should now have the right order.

Not super happy about the changes as the implementation is now more complex (and expensive) than before (without a real need, as the module scripts are injected in a later stage of the transformation).

sebastianbenz · 2021-04-07T16:02:03Z

...s/optimizer/spec/transformers/valid/ReorderHeadTransformer/supports_esm/expected_output.html

+  <script async nomodule src="https://cdn.ampproject.org/v0/amp-mustache-0.2.js" custom-template="amp-mustache"></script>
+  <script async custom-template="amp-mustache" src="https://cdn.ampproject.org/v0/amp-mustache-0.2.mjs" type="module" crossorigin="anonymous"></script>


sebastianbenz · 2021-04-07T16:02:08Z

packages/optimizer/spec/transformers/valid/ReorderHeadTransformer/supports_esm/input.html

+<!doctype html>
+<html ⚡>
+<head>
+  <script async nomodule src="https://cdn.ampproject.org/v0.js"></script>


schlessera · 2021-04-07T16:42:30Z

packages/optimizer/lib/transformers/ReorderHeadTransformer.js

+    appendAll(head, this._metaOther);
+    appendAll(head, this._resourceHintLinks);


The metaOther can potentially includes lots of non-critical stuff like OpenGraph tags. I think in terms of performance, the resource hints should come right after the charset and before these other meta tags.

good point! done

schlessera

I'm still wondering about the following priorities:

) Is the viewport needed for making decisions about the resource hints? I don't think so, but I'm not entirely sure.
) The boilerplate style is now much further down, after the scripts. Are we sure this does not lead to FOUC?

schlessera · 2021-04-07T18:01:26Z

packages/optimizer/lib/DomTransformer.js

  // needs to run after ReorderHeadTransformer
  'RewriteAmpUrls',


Suggested change

// needs to run after ReorderHeadTransformer

'RewriteAmpUrls',

'RewriteAmpUrls',

schlessera · 2021-04-07T18:01:37Z

packages/optimizer/lib/DomTransformer.js

  // needs to run after ReorderHeadTransformer
  'RewriteAmpUrls',


Suggested change

// needs to run after ReorderHeadTransformer

'RewriteAmpUrls',

'RewriteAmpUrls',

schlessera · 2021-04-07T18:01:49Z

packages/optimizer/lib/DomTransformer.js

  // needs to run after ReorderHeadTransformer
  'RewriteAmpUrls',


Suggested change

// needs to run after ReorderHeadTransformer

'RewriteAmpUrls',

'RewriteAmpUrls',

sebastianbenz · 2021-04-07T18:16:13Z

re 1) Yes, viewport needs to come first (I think that was the reason why I added metaOthers before the resource hints originally. The correct way is to also special case viewport.
re 2) I'm not sure about this. My thinking for the current order is: component downloads should start asap hence they should be placed before custom css. Boilerplate should come after custom CSS to avoid custom CSS accidentally overriding the boilerplate. FOUC should not be a problem as the browser will parse all CSS before rendering the page.

sebastianbenz · 2021-04-07T18:19:24Z

Hm. Following that logic, we should also move v0.css down.

westonruter

LGTM, but I defer to @schlessera.

schlessera · 2021-04-08T15:38:00Z

sebastianbenz · 2021-04-08T16:50:46Z

Thanks a lot for the patience and detailed review!

Filter duplicate extensions by src URL

2efc49d

Fixes #1191

sebastianbenz requested a review from schlessera April 6, 2021 18:19

This was referenced Apr 6, 2021

ReorderHead transformer accidentally deduplicates based on URL ampproject/amp-toolbox-php#125

Closed

Fix deduplication of AMP scripts and improve ReorderHead transformer ampproject/amp-toolbox-php#130

Merged

westonruter reviewed Apr 6, 2021

View reviewed changes

move resource hints up

d96effd

sebastianbenz requested a review from westonruter April 6, 2021 19:57

move runtime styles after resource hints

118fab7

westonruter reviewed Apr 6, 2021

View reviewed changes

don't forget the modulepreload...

ec427be

westonruter reviewed Apr 6, 2021

View reviewed changes

schlessera reviewed Apr 7, 2021

View reviewed changes

Sort extensions alphabetically and don't filter nomodule v0.js

0ce925f

westonruter reviewed Apr 7, 2021

View reviewed changes

sebastianbenz added 2 commits April 7, 2021 17:59

workaround for lack of Array#flat in Node10

85396ef

swap module and nomodule

63ef718

sebastianbenz commented Apr 7, 2021

View reviewed changes

sebastianbenz requested review from schlessera and westonruter April 7, 2021 16:05

schlessera reviewed Apr 7, 2021

View reviewed changes

sebastianbenz added 2 commits April 7, 2021 19:07

inject resource hints before other meta tags

68804c2

run ReOrderHeadTransformer last

3192282

sebastianbenz requested a review from schlessera April 7, 2021 17:56

schlessera suggested changes Apr 7, 2021

View reviewed changes

sebastianbenz added 2 commits April 7, 2021 20:24

Move viewport after meta charset

6901062

clean up

f8d0d1c

sebastianbenz requested a review from schlessera April 7, 2021 20:11

westonruter approved these changes Apr 8, 2021

View reviewed changes

schlessera approved these changes Apr 8, 2021

View reviewed changes

sebastianbenz merged commit 359aea3 into main Apr 8, 2021

sebastianbenz deleted the reorder-esm branch April 8, 2021 16:50

sebastianbenz added the enhancement label Apr 8, 2021

		<script async nomodule src="https://cdn.ampproject.org/v0/amp-mustache-0.2.js" custom-template="amp-mustache"></script>
		<script async custom-template="amp-mustache" src="https://cdn.ampproject.org/v0/amp-mustache-0.2.mjs" type="module" crossorigin="anonymous"></script>

	const moduleIndex = hasAttribute(node, 'nomodule') ? 0 : 1;
	const moduleIndex = hasAttribute(node, 'nomodule') ? 1 : 0;

		appendAll(head, this._metaOther);
		appendAll(head, this._resourceHintLinks);

		// needs to run after ReorderHeadTransformer
		'RewriteAmpUrls',

Conversation

sebastianbenz commented Apr 6, 2021

Uh oh!

westonruter commented Apr 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter commented Apr 6, 2021

Uh oh!

sebastianbenz commented Apr 6, 2021

Uh oh!

westonruter Apr 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

This comment was marked as resolved.

Uh oh!

westonruter commented Apr 6, 2021

Uh oh!

westonruter commented Apr 6, 2021

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!

sebastianbenz left a comment

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!

schlessera left a comment

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!

sebastianbenz commented Apr 7, 2021

Uh oh!

sebastianbenz commented Apr 7, 2021

Uh oh!

westonruter left a comment

Choose a reason for hiding this comment

Uh oh!

schlessera commented Apr 8, 2021

Uh oh!

sebastianbenz commented Apr 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

westonruter Apr 6, 2021 •

edited

Loading