Merge pull request #130 from ampproject/fix/reorder-head-deduplication

src/Amp.php

@@ -150,8 +150,11 @@ public static function isRuntimeScript(DOMNode $node)
         }
 
         if (
+            // @TODO Compare performance against single regex.
             substr($src, -6) !== '/v0.js'
+            && substr($src, -7) !== '/v0.mjs'
             && substr($src, -14) !== '/amp4ads-v0.js'
+            && substr($src, -15) !== '/amp4ads-v0.mjs'
         ) {
             return false;
         }

src/Optimizer/Transformer/ReorderHead.php

@@ -63,8 +63,8 @@ final class ReorderHead implements Transformer
     private $noscript                          = null;
     private $others                            = [];
     private $resourceHintLinks                 = [];
-    private $scriptAmpRuntime                  = null;
-    private $scriptAmpViewer                   = null;
+    private $scriptAmpRuntime                  = [];
+    private $scriptAmpViewer                   = [];
     private $scriptNonRenderDelayingExtensions = [];
     private $scriptRenderDelayingExtensions    = [];
     private $styleAmpBoilerplate               = null;
@@ -91,7 +91,6 @@ public function transform(Document $document, ErrorCollection $errors)
             $this->registerNode($node);
         }
 
-        $this->deduplicateAndSortCustomNodes();
         $this->appendToHead($document);
     }
 
@@ -127,7 +126,7 @@ private function registerNode(DOMNode $node)
                 $this->registerLink($node);
                 break;
             case Tag::NOSCRIPT:
-                $this->noscript = $node;
+                $this->noscript = $node; // @todo Make this an array.
                 break;
             default:
                 $this->others[] = $node;
@@ -156,32 +155,37 @@ private function registerMeta(Element $node)
      */
     private function registerScript(Element $node)
     {
+        $nomodule = (int)$node->hasAttribute('nomodule');
+
         if (Amp::isRuntimeScript($node)) {
-            $this->scriptAmpRuntime = $node;
+            $this->scriptAmpRuntime[$nomodule] = $node;
             return;
         }
 
         if (Amp::isViewerScript($node)) {
-            $this->scriptAmpViewer = $node;
+            $this->scriptAmpViewer[$nomodule] = $node;
             return;
         }
 
         if ($node->hasAttribute(Attribute::CUSTOM_ELEMENT)) {
+            $name = $node->getAttribute(Attribute::CUSTOM_ELEMENT);
             if (Amp::isRenderDelayingExtension($node)) {
-                $this->scriptRenderDelayingExtensions[] = $node;
+                $this->scriptRenderDelayingExtensions[$name][$nomodule] = $node;
                 return;
             }
-            $this->scriptNonRenderDelayingExtensions[] = $node;
+            $this->scriptNonRenderDelayingExtensions[$name][$nomodule] = $node;
             return;
         }
 
         if ($node->hasAttribute(Attribute::CUSTOM_TEMPLATE)) {
-            $this->scriptNonRenderDelayingExtensions[] = $node;
+            $name = $node->getAttribute(Attribute::CUSTOM_TEMPLATE);
+            $this->scriptNonRenderDelayingExtensions[$name][$nomodule] = $node;
             return;
         }
 
         if ($node->hasAttribute(Attribute::HOST_SERVICE)) {
-            $this->scriptNonRenderDelayingExtensions[] = $node;
+            $name = $node->getAttribute(Attribute::HOST_SERVICE);
+            $this->scriptNonRenderDelayingExtensions[$name][$nomodule] = $node;
             return;
         }
 
@@ -248,6 +252,7 @@ private function registerLink(Element $node)
             || $this->containsWord($rel, Attribute::REL_PREFETCH)
             || $this->containsWord($rel, Attribute::REL_DNS_PREFETCH)
             || $this->containsWord($rel, Attribute::REL_PRECONNECT)
+            || $this->containsWord($rel, Attribute::REL_MODULEPRELOAD)
         ) {
             if ($this->isHintForAmp($node)) {
                 $this->ampResourceHints[] = $node;
@@ -260,25 +265,6 @@ private function registerLink(Element $node)
         $this->others[] = $node;
     }
 
-    /**
-     * Get the name of the custom node or template.
-     *
-     * @param Element $node Node to get the name of.
-     * @return string Name of the custom node or template. Empty string if none found.
-     */
-    private function getName(Element $node)
-    {
-        if ($node->hasAttribute(Attribute::CUSTOM_ELEMENT)) {
-            return $node->getAttribute(Attribute::CUSTOM_ELEMENT);
-        }
-
-        if ($node->hasAttribute(Attribute::CUSTOM_TEMPLATE)) {
-            return $node->getAttribute(Attribute::CUSTOM_TEMPLATE);
-        }
-
-        return '';
-    }
-
     /**
      * Append all registered nodes to the <head> node.
      *
@@ -314,27 +300,28 @@ private function appendToHead(Document $document)
                 $node = $document->importNode($this->$category);
                 $document->head->appendChild($node);
             } elseif (is_array($this->$category)) {
-                // @todo Maybe sort by attribute-name, attribute-value?
-                foreach ($this->$category as $node) {
-                    $node = $document->importNode($node);
-                    $document->head->appendChild($node);
-                }
+                $this->recursiveKeySort($this->$category);
+                array_walk_recursive(
+                    $this->$category,
+                    static function (DOMNode $node) use ($document) {
+                        $node = $document->importNode($node);
+                        $document->head->appendChild($node);
+                    }
+                );
             }
         }
     }
 
     /**
-     * Deduplicate and sort custom extensions.
+     * Sort array keys recursively.
+     *
+     * @param array|mixed $item Item.
      */
-    private function deduplicateAndSortCustomNodes()
+    private function recursiveKeySort(&$item)
     {
-        foreach (['scriptRenderDelayingExtensions', 'scriptNonRenderDelayingExtensions'] as $set) {
-            $sortedNodes = [];
-            foreach ($this->$set as $node) {
-                $sortedNodes[$this->getName($node)] = $node;
-            }
-            ksort($sortedNodes);
-            $this->$set = array_values($sortedNodes);
+        if (is_array($item)) {
+            ksort($item);
+            array_walk($item, [$this, 'recursiveKeySort']);
         }
     }
 

src/Optimizer/Transformer/RewriteAmpUrls.php

@@ -88,7 +88,7 @@ public function __construct(TransformerConfiguration $configuration)
     public function transform(Document $document, ErrorCollection $errors)
     {
         $host          = $this->calculateHost();
-        $referenceNode = $this->findReferenceNode($document);
+        $referenceNode = $document->viewport;
 
         $preloadNodes = array_filter($this->collectPreloadNodes($document, $host));
         foreach ($preloadNodes as $preloadNode) {
@@ -347,29 +347,4 @@ private function addMeta(Document $document, $name, $content)
         $firstScript = $document->xpath->query('./script', $document->head)->item(0);
         $document->head->insertBefore($meta, $firstScript);
     }
-
-    /**
-     * Find the reference node to use for adding elements.
-     *
-     * It fetches the last <link> after the viewport as the reference node, falling back to the viewport if needed.
-     *
-     * @param Document $document Document to find the reference node in.
-     * @return Element|null Reference node to use, or null if none found.
-     */
-    private function findReferenceNode(Document $document)
-    {
-        $referenceNode = $document->viewport;
-
-        while (
-            $referenceNode !== null
-            &&
-            $referenceNode->nextSibling instanceof Element
-            &&
-            $referenceNode->nextSibling->tagName === Tag::LINK
-        ) {
-            $referenceNode = $referenceNode->nextSibling;
-        }
-
-        return $referenceNode;
-    }
 }

tests/Optimizer/Transformer/ReorderHeadTest.php

@@ -55,8 +55,8 @@ public function dataTransform()
                 // (5) <script> tags that are render delaying
                 TestMarkup::SCRIPT_AMPEXPERIMENT .
                 // (6) <script> tags for remaining extensions
-                TestMarkup::SCRIPT_AMPMRAID .
                 TestMarkup::SCRIPT_AMPAUDIO .
+                TestMarkup::SCRIPT_AMPMRAID .
                 TestMarkup::SCRIPT_AMPMUSTACHE .
                 // (7) <link> tag for favicons
                 TestMarkup::LINK_FAVICON .

tests/Optimizer/Transformer/RewriteAmpUrlsTest.php

@@ -54,8 +54,8 @@ public function testOrderingOfScripts()
         $this->assertCount(0, $errors);
         $this->assertSimilarMarkup(
             TestMarkup::DOCTYPE . '<html amp><head>' . TestMarkup::META_CHARSET . TestMarkup::META_VIEWPORT
-            . TestMarkup::LINK_CANONICAL . TestMarkup::LINK_GOOGLE_FONT_PRECONNECT
             . '<link as="script" crossorigin="anonymous" href="https://cdn.ampproject.org/v0.mjs" rel="modulepreload">'
+            . TestMarkup::LINK_CANONICAL . TestMarkup::LINK_GOOGLE_FONT_PRECONNECT
             . '<script async nomodule src="https://cdn.ampproject.org/v0.js"></script>'
             . '<script async crossorigin="anonymous" src="https://cdn.ampproject.org/v0.mjs" type="module"></script>'
             . '</head><body></body></html>',