optimizer: Do not append rtv/{rtv}/ to ampUrlPrefix (#517)

* optimizer: Do not append rtv/{rtv}/ to ampUrlPrefix BREAKING CHANGE When both ampUrlPrefix and ampRuntimeVersion are specified, do not append rtv/{rtv}/ to the prefix. If version information is expected in the URL, it can be included in ampUrlPrefix. * optimizer: Update tests for revised ampUrlPrefix handling * optimizer: Revise version in ampUrlPrefix notice
ampproject · Oct 9, 2019 · 9e7533c · 9e7533c
1 parent be53879
commit 9e7533c
Show file tree

Hide file tree

Showing 6 changed files with 32 additions and 26 deletions.
diff --git a/packages/optimizer/README.md b/packages/optimizer/README.md
@@ -187,9 +187,13 @@ const optimizer = AmpOptimizer.create({
 
 ### Self-hosted AMP components
 
-It's possible to rewrite the AMP framework and component imports to a different domain than `cdn.ampproject.org`.
+It's possible to rewrite the AMP framework and component imports to a different domain than `cdn.ampproject.org`. These options tailor URL rewriting performed by `ampOptimizer.transformHtml`:
 
-Example:
+- `ampUrlPrefix`: Replace `https://cdn.ampproject.org/` with another origin or relative path.
+
+  **Notice:** The behavior of `ampUrlPrefix` when used in conjunction with `ampRuntimeVersion` changed beginning with version 1.1.2. Prior to 1.1.2, `rtv/{rtv}/` was automatically appended to `ampUrlPrefix` when `ampRuntimeVersion` was specified. Since version 1.1.2, `ampUrlPrefix` is not modified when `ampRuntimeVersion` is also specified.
+
+Examples:
 ```
 const ampOptimizer = require('@ampproject/toolbox-optimizer');
 
@@ -200,12 +204,14 @@ const originalHtml = `
 ...
 `
 
-// Additional options can be passed as the second argument
-const optimizedHtml = await ampOptimizer.transformHtml(originalHtml, {
-  ampUrl: 'canonical.amp.html',
-  // this will rewrite https://cdn.ampproject.org/v0.js to /amp/v0.js
+// this will rewrite https://cdn.ampproject.org/v0.js to /amp/v0.js
+const optimizedHtmlA = await ampOptimizer.transformHtml(originalHtml, {
   ampUrlPrefix: '/amp'
 });
 
-console.log(optimizedHtml);
+// this will rewrite https://cdn.ampproject.org/v0.js to /amp/v0.js
+const optimizedHtmlB = await ampOptimizer.transformHtml(originalHtml, {
+  ampRuntimeVersion: '001515617716922',
+  ampUrlPrefix: '/amp'
+});
 ```
diff --git a/packages/optimizer/lib/transformers/RewriteAmpUrls.js b/packages/optimizer/lib/transformers/RewriteAmpUrls.js
@@ -49,7 +49,7 @@ class RewriteAmpUrls {
     if (!head) return;
 
     let ampUrlPrefix = params.ampUrlPrefix || AMP_CACHE_HOST;
-    if (params.ampRuntimeVersion) {
+    if (params.ampRuntimeVersion && !params.ampUrlPrefix) {
       ampUrlPrefix = appendRuntimeVersion(ampUrlPrefix, params.ampRuntimeVersion);
     }
 

diff --git a/packages/optimizer/spec/end-to-end/hello-world/expected_output.html b/packages/optimizer/spec/end-to-end/hello-world/expected_output.html
@@ -4,11 +4,11 @@
   <meta charset="utf-8">
   <style amp-runtime="" i-amphtml-version="123456789000000">/* v0.css */</style>
   <meta name="viewport" content="width=device-width,minimum-scale=1">
-  <link rel="preload" href="/amp/rtv/123456789000000/v0.js" as="script">
-  <script async="" src="/amp/rtv/123456789000000/v0.js"></script>
-  <script async="" custom-element="amp-experiment" src="/amp/rtv/123456789000000/v0/amp-experiment-0.1.js"></script>
-  <script async="" custom-element="amp-video" src="/amp/rtv/123456789000000/v0/amp-video-0.1.js"></script>
-  <script async="" custom-template="amp-mustache" src="/amp/rtv/123456789000000/v0/amp-mustache-0.1.js"></script>
+  <link rel="preload" href="/amp/v0.js" as="script">
+  <script async="" src="/amp/v0.js"></script>
+  <script async="" custom-element="amp-experiment" src="/amp/v0/amp-experiment-0.1.js"></script>
+  <script async="" custom-element="amp-video" src="/amp/v0/amp-video-0.1.js"></script>
+  <script async="" custom-template="amp-mustache" src="/amp/v0/amp-mustache-0.1.js"></script>
   <style amp-custom="">h1 {
   margin: 16px;
 }</style>

diff --git a/...nsformers/experimental/RewriteAmpUrls/rewrites_host_and_adds_version/expected_output.html b/...nsformers/experimental/RewriteAmpUrls/rewrites_host_and_adds_version/expected_output.html
diff --git a/...rmers/experimental/RewriteAmpUrls/rewrites_host_and_excludes_version/expected_output.html b/...rmers/experimental/RewriteAmpUrls/rewrites_host_and_excludes_version/expected_output.html
@@ -0,0 +1,13 @@
+<!doctype html>
+<html ⚡>
+<head>
+  <link rel="preload" href="/amp/v0.js" as="script">
+  <link rel="preload" href="/amp/v0.css" as="style">
+  <script async custom-element=amp-experiment src=/amp/v0/amp-experiment-0.1.js></script>
+  <script async src="/amp/v0.js"></script>
+  <link rel=stylesheet href=/amp/v0.css>
+  <link href=https://fonts.googleapis.com/css?foobar rel=stylesheet type=text/css>
+  <link href=https://example.com/favicon.ico rel=icon>
+</head>
+<body></body>
+</html>
diff --git a/...rewrites_host_and_adds_version/input.html → ...ites_host_and_excludes_version/input.html b/...rewrites_host_and_adds_version/input.html → ...ites_host_and_excludes_version/input.html