From e378c9b78d57c5ead5899bc66c9a3d1a199af7e4 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 6 Apr 2021 12:14:09 -0700 Subject: [PATCH 01/18] Deduplicate scripts by src as opposed to custom-element/custom-template attrs Fixes #125 --- src/Optimizer/Transformer/ReorderHead.php | 22 ++-------------- src/Optimizer/Transformer/RewriteAmpUrls.php | 27 +------------------- 2 files changed, 3 insertions(+), 46 deletions(-) diff --git a/src/Optimizer/Transformer/ReorderHead.php b/src/Optimizer/Transformer/ReorderHead.php index c37f9b7a7..009fb18f5 100644 --- a/src/Optimizer/Transformer/ReorderHead.php +++ b/src/Optimizer/Transformer/ReorderHead.php @@ -260,25 +260,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 node. * @@ -331,7 +312,8 @@ private function deduplicateAndSortCustomNodes() foreach (['scriptRenderDelayingExtensions', 'scriptNonRenderDelayingExtensions'] as $set) { $sortedNodes = []; foreach ($this->$set as $node) { - $sortedNodes[$this->getName($node)] = $node; + /** @var Element $node */ + $sortedNodes[$node->getAttribute('src')] = $node; } ksort($sortedNodes); $this->$set = array_values($sortedNodes); diff --git a/src/Optimizer/Transformer/RewriteAmpUrls.php b/src/Optimizer/Transformer/RewriteAmpUrls.php index 0dc627be6..48a331af7 100644 --- a/src/Optimizer/Transformer/RewriteAmpUrls.php +++ b/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 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; - } } From b29a21f9c61995a8a7504c196db9c88824811d19 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 6 Apr 2021 12:18:56 -0700 Subject: [PATCH 02/18] Update tests --- tests/Optimizer/Transformer/ReorderHeadTest.php | 2 +- tests/Optimizer/Transformer/RewriteAmpUrlsTest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Optimizer/Transformer/ReorderHeadTest.php b/tests/Optimizer/Transformer/ReorderHeadTest.php index 8674ba2a8..7bd4b4c86 100644 --- a/tests/Optimizer/Transformer/ReorderHeadTest.php +++ b/tests/Optimizer/Transformer/ReorderHeadTest.php @@ -55,8 +55,8 @@ public function dataTransform() // (5) ' . '' . '', From 3305726b9815120c71a78f38ba0e366830d1d3e0 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 6 Apr 2021 12:25:31 -0700 Subject: [PATCH 03/18] Add ReorderHeadTransformer/supports_esm test files --- .../supports_esm/expected_output.html | 16 ++++++++++++++++ .../supports_esm/input.html | 19 +++++++++++++++++++ 2 files changed, 35 insertions(+) create mode 100644 tests/spec/transformers/valid/ReorderHeadTransformer/supports_esm/expected_output.html create mode 100644 tests/spec/transformers/valid/ReorderHeadTransformer/supports_esm/input.html diff --git a/tests/spec/transformers/valid/ReorderHeadTransformer/supports_esm/expected_output.html b/tests/spec/transformers/valid/ReorderHeadTransformer/supports_esm/expected_output.html new file mode 100644 index 000000000..bef858f53 --- /dev/null +++ b/tests/spec/transformers/valid/ReorderHeadTransformer/supports_esm/expected_output.html @@ -0,0 +1,16 @@ + + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/tests/spec/transformers/valid/ReorderHeadTransformer/supports_esm/input.html b/tests/spec/transformers/valid/ReorderHeadTransformer/supports_esm/input.html new file mode 100644 index 000000000..a693fc582 --- /dev/null +++ b/tests/spec/transformers/valid/ReorderHeadTransformer/supports_esm/input.html @@ -0,0 +1,19 @@ + + + + + + + + + + + + + + + + + + + \ No newline at end of file From f46ecf000926144c2a4dae7dc463678c653226f0 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 6 Apr 2021 12:34:34 -0700 Subject: [PATCH 04/18] Remove stray duplicated semicolon --- src/Optimizer/Transformer/RewriteAmpUrls.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Optimizer/Transformer/RewriteAmpUrls.php b/src/Optimizer/Transformer/RewriteAmpUrls.php index 48a331af7..82529354c 100644 --- a/src/Optimizer/Transformer/RewriteAmpUrls.php +++ b/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 = $document->viewport;; + $referenceNode = $document->viewport; $preloadNodes = array_filter($this->collectPreloadNodes($document, $host)); foreach ($preloadNodes as $preloadNode) { From a3a99fd5c66dad645a1ab47138d000b32b635df4 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 6 Apr 2021 12:55:26 -0700 Subject: [PATCH 05/18] Add modulepreload link to ampResourceHints --- src/Optimizer/Transformer/ReorderHead.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Optimizer/Transformer/ReorderHead.php b/src/Optimizer/Transformer/ReorderHead.php index 009fb18f5..16ee0bc11 100644 --- a/src/Optimizer/Transformer/ReorderHead.php +++ b/src/Optimizer/Transformer/ReorderHead.php @@ -248,6 +248,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; From 4bad718a538c317168a9cb36728a7d977e25b7db Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 6 Apr 2021 13:05:35 -0700 Subject: [PATCH 06/18] Account for .mjs scripts in isRuntimeScript --- src/Amp.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Amp.php b/src/Amp.php index 1bdc60d9f..b58d1c562 100644 --- a/src/Amp.php +++ b/src/Amp.php @@ -151,7 +151,9 @@ public static function isRuntimeScript(DOMNode $node) if ( substr($src, -6) !== '/v0.js' + && substr($src, -7) !== '/v0.mjs' && substr($src, -14) !== '/amp4ads-v0.js' + && substr($src, -15) !== '/amp4ads-v0.mjs' ) { return false; } From cda4bd86e87c17778bb613da0acb155987f2746b Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 6 Apr 2021 13:13:16 -0700 Subject: [PATCH 07/18] Account for multiple runtime scripts in reordering --- src/Optimizer/Transformer/ReorderHead.php | 4 ++-- .../supports_esm/expected_output.html | 9 +++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/Optimizer/Transformer/ReorderHead.php b/src/Optimizer/Transformer/ReorderHead.php index 16ee0bc11..e74ebbd80 100644 --- a/src/Optimizer/Transformer/ReorderHead.php +++ b/src/Optimizer/Transformer/ReorderHead.php @@ -63,7 +63,7 @@ final class ReorderHead implements Transformer private $noscript = null; private $others = []; private $resourceHintLinks = []; - private $scriptAmpRuntime = null; + private $scriptAmpRuntime = []; private $scriptAmpViewer = null; private $scriptNonRenderDelayingExtensions = []; private $scriptRenderDelayingExtensions = []; @@ -157,7 +157,7 @@ private function registerMeta(Element $node) private function registerScript(Element $node) { if (Amp::isRuntimeScript($node)) { - $this->scriptAmpRuntime = $node; + $this->scriptAmpRuntime[$node->getAttribute('src')] = $node; return; } diff --git a/tests/spec/transformers/valid/ReorderHeadTransformer/supports_esm/expected_output.html b/tests/spec/transformers/valid/ReorderHeadTransformer/supports_esm/expected_output.html index bef858f53..3276f5114 100644 --- a/tests/spec/transformers/valid/ReorderHeadTransformer/supports_esm/expected_output.html +++ b/tests/spec/transformers/valid/ReorderHeadTransformer/supports_esm/expected_output.html @@ -1,16 +1,17 @@ + + + - - + + - - \ No newline at end of file From 6f3e006d46ebfa29bf7af19eaf8386fc2153b3d2 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 6 Apr 2021 13:44:57 -0700 Subject: [PATCH 08/18] Implement script deduplication scheme which keeps module/nomodule scripts together --- src/Optimizer/Transformer/ReorderHead.php | 50 ++++++++----------- .../Optimizer/Transformer/ReorderHeadTest.php | 4 +- 2 files changed, 22 insertions(+), 32 deletions(-) diff --git a/src/Optimizer/Transformer/ReorderHead.php b/src/Optimizer/Transformer/ReorderHead.php index e74ebbd80..5829ba298 100644 --- a/src/Optimizer/Transformer/ReorderHead.php +++ b/src/Optimizer/Transformer/ReorderHead.php @@ -64,7 +64,7 @@ final class ReorderHead implements Transformer private $others = []; private $resourceHintLinks = []; private $scriptAmpRuntime = []; - private $scriptAmpViewer = null; + 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->getAttribute('src')] = $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; } @@ -296,28 +300,14 @@ 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); - } - } - } - } - - /** - * Deduplicate and sort custom extensions. - */ - private function deduplicateAndSortCustomNodes() - { - foreach (['scriptRenderDelayingExtensions', 'scriptNonRenderDelayingExtensions'] as $set) { - $sortedNodes = []; - foreach ($this->$set as $node) { - /** @var Element $node */ - $sortedNodes[$node->getAttribute('src')] = $node; + array_walk_recursive( + $this->$category, + static function ( Element $node ) use ( $document ) { + $node = $document->importNode($node); + $document->head->appendChild($node); + } + ); } - ksort($sortedNodes); - $this->$set = array_values($sortedNodes); } } diff --git a/tests/Optimizer/Transformer/ReorderHeadTest.php b/tests/Optimizer/Transformer/ReorderHeadTest.php index 7bd4b4c86..d5eabb87f 100644 --- a/tests/Optimizer/Transformer/ReorderHeadTest.php +++ b/tests/Optimizer/Transformer/ReorderHeadTest.php @@ -56,8 +56,8 @@ public function dataTransform() TestMarkup::SCRIPT_AMPEXPERIMENT . // (6) - - + + + \ No newline at end of file From 5ac47b9f2e4a8801d18399c811313163b3ec1443 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 6 Apr 2021 14:08:36 -0700 Subject: [PATCH 10/18] Update test with modulepreload link --- .../ReorderHeadTransformer/supports_esm/expected_output.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/spec/transformers/valid/ReorderHeadTransformer/supports_esm/expected_output.html b/tests/spec/transformers/valid/ReorderHeadTransformer/supports_esm/expected_output.html index c49d24eed..47b711331 100644 --- a/tests/spec/transformers/valid/ReorderHeadTransformer/supports_esm/expected_output.html +++ b/tests/spec/transformers/valid/ReorderHeadTransformer/supports_esm/expected_output.html @@ -1,6 +1,7 @@ + @@ -10,7 +11,6 @@ - \ No newline at end of file From c158850b703fdb087baba8eb7e9a65a73bdde6b5 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 6 Apr 2021 15:52:04 -0700 Subject: [PATCH 11/18] Ensure nomodule scripts come after module ones --- src/Optimizer/Transformer/ReorderHead.php | 15 +++++++++++++-- .../supports_esm/expected_output.html | 5 +++-- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/src/Optimizer/Transformer/ReorderHead.php b/src/Optimizer/Transformer/ReorderHead.php index 2851ab643..47f0f5343 100644 --- a/src/Optimizer/Transformer/ReorderHead.php +++ b/src/Optimizer/Transformer/ReorderHead.php @@ -300,8 +300,7 @@ private function appendToHead(Document $document) $node = $document->importNode($this->$category); $document->head->appendChild($node); } elseif (is_array($this->$category)) { - // @todo Do recursive sort so that module is always before nomodule? - ksort($this->$category); + $this->recursiveKeySort($this->$category); array_walk_recursive( $this->$category, static function ( Element $node ) use ( $document ) { @@ -313,6 +312,18 @@ static function ( Element $node ) use ( $document ) { } } + /** + * Sort array keys recursively. + * + * @param array|mixed $item Item. + */ + private function recursiveKeySort( &$item ) { + if (is_array($item)) { + ksort($item); + array_walk( $item, [$this, 'recursiveKeySort']); + } + } + /** * Check if a given string contains another string, respecting word boundaries.. * diff --git a/tests/spec/transformers/valid/ReorderHeadTransformer/supports_esm/expected_output.html b/tests/spec/transformers/valid/ReorderHeadTransformer/supports_esm/expected_output.html index 47b711331..de884ee57 100644 --- a/tests/spec/transformers/valid/ReorderHeadTransformer/supports_esm/expected_output.html +++ b/tests/spec/transformers/valid/ReorderHeadTransformer/supports_esm/expected_output.html @@ -5,10 +5,11 @@ - + - + + From 7b23a67959696ce28ef126fe441d42877f5120b2 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 6 Apr 2021 16:25:26 -0700 Subject: [PATCH 12/18] Fix running phpcs when installed as composer package --- .phpcs.xml.dist | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.phpcs.xml.dist b/.phpcs.xml.dist index 0c583c32e..e8e0a901d 100644 --- a/.phpcs.xml.dist +++ b/.phpcs.xml.dist @@ -11,7 +11,7 @@ resources/* - vendor/* + amp-toolbox/vendor/ From 7c08d4e6679d5381471bdee5da8b04c6ff7cc00c Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 6 Apr 2021 16:25:34 -0700 Subject: [PATCH 13/18] Fix phpcs issues --- src/Optimizer/Transformer/ReorderHead.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Optimizer/Transformer/ReorderHead.php b/src/Optimizer/Transformer/ReorderHead.php index 47f0f5343..f75ae62f3 100644 --- a/src/Optimizer/Transformer/ReorderHead.php +++ b/src/Optimizer/Transformer/ReorderHead.php @@ -303,7 +303,7 @@ private function appendToHead(Document $document) $this->recursiveKeySort($this->$category); array_walk_recursive( $this->$category, - static function ( Element $node ) use ( $document ) { + static function (Element $node) use ($document) { $node = $document->importNode($node); $document->head->appendChild($node); } @@ -317,10 +317,11 @@ static function ( Element $node ) use ( $document ) { * * @param array|mixed $item Item. */ - private function recursiveKeySort( &$item ) { + private function recursiveKeySort(&$item) + { if (is_array($item)) { ksort($item); - array_walk( $item, [$this, 'recursiveKeySort']); + array_walk($item, [$this, 'recursiveKeySort']); } } From 5824c99fb1a98ee7d92486ea2a6eb63294c9f65f Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 6 Apr 2021 16:30:33 -0700 Subject: [PATCH 14/18] Add exclude-pattern for non-vendor dependency --- .phpcs.xml.dist | 1 + 1 file changed, 1 insertion(+) diff --git a/.phpcs.xml.dist b/.phpcs.xml.dist index e8e0a901d..851af52d1 100644 --- a/.phpcs.xml.dist +++ b/.phpcs.xml.dist @@ -12,6 +12,7 @@ amp-toolbox/vendor/ + amp-toolbox-php/vendor/ From bd76b1aa506d764f6142ad6f43fda254079892f1 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 6 Apr 2021 16:55:31 -0700 Subject: [PATCH 15/18] Account for non-Element nodes in the head when ordering --- src/Optimizer/Transformer/ReorderHead.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Optimizer/Transformer/ReorderHead.php b/src/Optimizer/Transformer/ReorderHead.php index f75ae62f3..083f26aa2 100644 --- a/src/Optimizer/Transformer/ReorderHead.php +++ b/src/Optimizer/Transformer/ReorderHead.php @@ -303,7 +303,7 @@ private function appendToHead(Document $document) $this->recursiveKeySort($this->$category); array_walk_recursive( $this->$category, - static function (Element $node) use ($document) { + static function (DOMNode $node) use ($document) { $node = $document->importNode($node); $document->head->appendChild($node); } From 63244cc997d320c5b85ec8375235c9bd09cc3a42 Mon Sep 17 00:00:00 2001 From: Alain Schlesser Date: Wed, 7 Apr 2021 15:50:11 +0100 Subject: [PATCH 16/18] Add performance TODO --- src/Amp.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Amp.php b/src/Amp.php index b58d1c562..66bd3b201 100644 --- a/src/Amp.php +++ b/src/Amp.php @@ -150,6 +150,7 @@ 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' From b62d9240fe0e086cb2661b60124eca90d37abd99 Mon Sep 17 00:00:00 2001 From: Alain Schlesser Date: Wed, 7 Apr 2021 15:52:46 +0100 Subject: [PATCH 17/18] Revert change to PHPCS config --- .phpcs.xml.dist | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.phpcs.xml.dist b/.phpcs.xml.dist index 851af52d1..0c583c32e 100644 --- a/.phpcs.xml.dist +++ b/.phpcs.xml.dist @@ -11,8 +11,7 @@ resources/* - amp-toolbox/vendor/ - amp-toolbox-php/vendor/ + vendor/* From 9f4b14f98facb40ad107904dcd19a36c73f43c28 Mon Sep 17 00:00:00 2001 From: Alain Schlesser Date: Wed, 7 Apr 2021 15:58:49 +0100 Subject: [PATCH 18/18] Fix whitespace issues --- src/Amp.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Amp.php b/src/Amp.php index 66bd3b201..5bcad4f7a 100644 --- a/src/Amp.php +++ b/src/Amp.php @@ -150,7 +150,7 @@ public static function isRuntimeScript(DOMNode $node) } if ( - // @TODO Compare performance against single regex. + // @TODO Compare performance against single regex. substr($src, -6) !== '/v0.js' && substr($src, -7) !== '/v0.mjs' && substr($src, -14) !== '/amp4ads-v0.js'