fix gulp integration and server so that it can work with mjs files (#…

…30292) * make it so that src transformation only happens strictly in scripts/script-transform.ts each module should do 1 thing and 1 thing only. currently the module transformer also did a src/url transformation. this caused weird mismatches when they are ran together. - added looseScriptSrcCheck to allow for non cdn domains. This will allow us to transition from the bad fixture files to having all html files be valid AMP HTML by default. * temp * temp * fix imports * add transform back to integration * fix html * fix integration * temp * temp * make transformers have a loose mode to be more forgiving on script src * add more tests and fix bug for extention retention * allow for mjs files to be served by the test server * add glob to load mjs files in karma server * lint fixes * lint html * break up this PR * fix bad error formatting * apply recs * Update build-system/server/new-server/transforms/transform.ts Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com> * Update build-system/server/new-server/transforms/utilities/option-set.ts Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com> * fix other locations of "FORTESTING" * fix esm and non esm tests to be green * use writeFileSync instead of async method. seems to be a race * guarantee directory exists first * fix windows issues * skip failing IE test temporarily * applied recs * apply more recs * temporarily read from testing version.txt * temp "only" * dont reroute to max json files * change error to warning * fix max builds and integration tests against max files * revert testing changes. we'll separete it into its own PR Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>
ampproject · Oct 14, 2020 · 73b3f0e · 73b3f0e
1 parent b025f30
commit 73b3f0e
Show file tree

Hide file tree

Showing 49 changed files with 306 additions and 112 deletions.
diff --git a/build-system/server/new-server/transforms/css/css-transform.ts b/build-system/server/new-server/transforms/css/css-transform.ts
@@ -28,9 +28,7 @@ const cwd = process.cwd();
 const cssPath = isTestMode
   ? `${cwd}/${testDir}/css.txt`
   : `${cwd}/build/css/v0.css`;
-const versionPath = isTestMode
-  ? `${cwd}/${testDir}/version.txt`
-  : `${cwd}/dist/version.txt`;
+const versionPath = `${cwd}/${testDir}/version.txt`
 
 const css = readFileSync(cssPath, 'utf8').toString().trim();
 const version = readFileSync(versionPath, 'utf8').toString().trim();
@@ -45,8 +43,21 @@ interface StyleNode extends PostHTML.Node {
   content: string[]
 }
 
+function isStyleNode(node: PostHTML.Node | string): node is StyleNode {
+  return node !== undefined && typeof node !== 'string' &&
+    (node as StyleNode).tag === 'style';
+}
+
 function prependAmpStyles(head: PostHTML.Node): PostHTML.Node {
   const content = head.content || [];
+
+  const firstStyleNode = content.filter(isStyleNode)[0];
+
+  // If 'amp-runtime' already exists bail out.
+  if (firstStyleNode?.attrs && 'amp-runtime' in firstStyleNode.attrs) {
+    return head;
+  }
+
   const styleNode: StyleNode = {
     walk: head.walk,
     match: head.match,

diff --git a/build-system/server/new-server/transforms/css/test/no-insert/input.html b/build-system/server/new-server/transforms/css/test/no-insert/input.html
@@ -0,0 +1,4 @@
+<head>
+<style amp-runtime>.amp-runtime{}</style>
+<style amp-custom>.my-personal-style {}</style>
+</head>
diff --git a/build-system/server/new-server/transforms/css/test/no-insert/output.html b/build-system/server/new-server/transforms/css/test/no-insert/output.html
@@ -0,0 +1,4 @@
+<head>
+<style amp-runtime="">.amp-runtime{}</style>
+<style amp-custom="">.my-personal-style {}</style>
+</head>
diff --git a/build-system/server/new-server/transforms/modules/modules-transform.ts b/build-system/server/new-server/transforms/modules/modules-transform.ts
@@ -15,44 +15,36 @@
  */
 
 import {PostHTML} from 'posthtml';
-import {URL} from 'url';
-import {isValidScript, ScriptNode} from '../utilities/script';
-import {CDNURLToLocalDistURL} from '../utilities/cdn';
+import {isJsonScript, isValidScript, toExtension, ScriptNode, tryGetUrl} from '../utilities/script';
 import {OptionSet} from '../utilities/option-set';
-import minimist from 'minimist';
-const argv = minimist(process.argv.slice(2));
 
 /**
  * @param head
  * @param script
- * @param compiled 
  */
-function appendModuleScript(head: PostHTML.Node, script: ScriptNode, compiled: boolean): void {
-  let modulePath;
-  if (argv.compiled || compiled) {
-    modulePath = CDNURLToLocalDistURL(
-      new URL(script.attrs.src || ''),
-      undefined,
-      '.mjs'
-    ).toString();
-    script.attrs.src = modulePath.replace('.mjs', '.js');
-  }
-  else {
-    const urlName = script.attrs.src.toString();
-    modulePath = urlName.replace('.js', '.mjs');
-  }
-
-  const insert: ScriptNode = {
-    ...script,
+function appendModuleScript(head: PostHTML.Node, nomoduleScript: ScriptNode, options: OptionSet): void {
+  const modulePath = toExtension(tryGetUrl(nomoduleScript.attrs.src), '.mjs').toString();
+  const moduleScript : ScriptNode = {
+    ...nomoduleScript,
     attrs: {
-      ...script.attrs,
+      ...nomoduleScript.attrs,
       src: modulePath,
       type: 'module',
     },
   };
-  delete insert.attrs.nomodule;
+  delete moduleScript.attrs.nomodule;
 
-  (head.content || []).push(insert);
+  const content = head.content || [];
+  const nomoduleIdx = content.indexOf(nomoduleScript);
+  // If we are testing and in esm mode, outright replace the nomodule script
+  // with the module script. This is so that we testing the module script in
+  // isolation without a fallback.
+  if (options.fortesting && options.esm) {
+    content.splice(nomoduleIdx, 1, moduleScript);
+  } else {
+    // Add the module script after the nomodule script.
+    content.splice(nomoduleIdx + 1, 0, '\n', moduleScript);
+  }
 }
 
 /**
@@ -62,13 +54,20 @@ function appendModuleScript(head: PostHTML.Node, script: ScriptNode, compiled: b
 export default function(options: OptionSet = {}): (tree: PostHTML.Node) => void {
   return function(tree: PostHTML.Node): void {
     let head: PostHTML.Node | undefined = undefined;
-    let compiled: boolean = options.compiled || false;
     const scripts: Array<ScriptNode> = [];
     tree.walk(node => {
       if (node.tag === 'head') {
         head = node;
       }
-      if (!isValidScript(node)) {
+
+      // Make sure that isJsonScript is used before `isValidScript`. We bail out
+      // early if the ScriptNofe is of type="application/json" since it wouldn't
+      // have any src url to modify.
+      if (isJsonScript(node)) {
+        return node;
+      }
+
+      if (!isValidScript(node, options.looseScriptSrcCheck)) {
         return node;
       }
 
@@ -84,7 +83,7 @@ export default function(options: OptionSet = {}): (tree: PostHTML.Node) => void
     }
 
     for (const script of scripts) {
-      appendModuleScript(head, script, compiled);
+      appendModuleScript(head, script, options);
     }
   }
 }
diff --git a/build-system/server/new-server/transforms/modules/test/esm-env-compiled/options.json b/build-system/server/new-server/transforms/modules/test/esm-env-compiled/options.json
diff --git a/build-system/server/new-server/transforms/modules/test/esm-env-compiled/output.html b/build-system/server/new-server/transforms/modules/test/esm-env-compiled/output.html
diff --git a/build-system/server/new-server/transforms/modules/test/esm-env/options.json b/build-system/server/new-server/transforms/modules/test/esm-env/options.json
diff --git a/build-system/server/new-server/transforms/modules/test/esm-env/output.html b/build-system/server/new-server/transforms/modules/test/esm-env/output.html
diff --git a/.../modules/test/esm-env-compiled/input.html → ...test/module-nomodule-cdn-input/input.html b/.../modules/test/esm-env-compiled/input.html → ...test/module-nomodule-cdn-input/input.html
@@ -1,3 +1,4 @@
 <head>
 <script async src="https://cdn.ampproject.org/v0.js"></script>
+<script async src="https://cdn.ampproject.org/v0/amp-bind-0.1.js"></script>
 </head>
diff --git a/build-system/server/new-server/transforms/modules/test/module-nomodule-cdn-input/output.html b/build-system/server/new-server/transforms/modules/test/module-nomodule-cdn-input/output.html
@@ -0,0 +1,6 @@
+<head>
+<script async="" src="https://cdn.ampproject.org/v0.js" nomodule=""></script>
+<script async="" src="https://cdn.ampproject.org/v0.mjs" type="module"></script>
+<script async="" src="https://cdn.ampproject.org/v0/amp-bind-0.1.js" nomodule=""></script>
+<script async="" src="https://cdn.ampproject.org/v0/amp-bind-0.1.mjs" type="module"></script>
+</head>
diff --git a/...erver/new-server/transforms/modules/test/module-nomodule-localhost-input-loose/input.html b/...erver/new-server/transforms/modules/test/module-nomodule-localhost-input-loose/input.html
@@ -0,0 +1,5 @@
+<head>
+<script async src="http://localhost:8000/amp.js"></script>
+<script async src="https://cdn.ampproject.org/v0.js"></script>
+<script async src="http://localhost:8000/v0/amp-bind-0.1.js"></script>
+</head>
diff --git a/...ver/new-server/transforms/modules/test/module-nomodule-localhost-input-loose/options.json b/...ver/new-server/transforms/modules/test/module-nomodule-localhost-input-loose/options.json
@@ -0,0 +1,3 @@
+{
+  "looseScriptSrcCheck": true
+}
diff --git a/...rver/new-server/transforms/modules/test/module-nomodule-localhost-input-loose/output.html b/...rver/new-server/transforms/modules/test/module-nomodule-localhost-input-loose/output.html
@@ -0,0 +1,8 @@
+<head>
+<script async="" src="http://localhost:8000/amp.js" nomodule=""></script>
+<script async="" src="http://localhost:8000/amp.mjs" type="module"></script>
+<script async="" src="https://cdn.ampproject.org/v0.js" nomodule=""></script>
+<script async="" src="https://cdn.ampproject.org/v0.mjs" type="module"></script>
+<script async="" src="http://localhost:8000/v0/amp-bind-0.1.js" nomodule=""></script>
+<script async="" src="http://localhost:8000/v0/amp-bind-0.1.mjs" type="module"></script>
+</head>
diff --git a/...rver/new-server/transforms/modules/test/module-nomodule-localhost-input-strict/input.html b/...rver/new-server/transforms/modules/test/module-nomodule-localhost-input-strict/input.html
@@ -0,0 +1,5 @@
+<head>
+<script async src="http://localhost:8000/amp.js"></script>
+<script async src="https://cdn.ampproject.org/v0.js"></script>
+<script async src="http://localhost:8000/v0/amp-bind-0.1.js"></script>
+</head>
diff --git a/...ver/new-server/transforms/modules/test/module-nomodule-localhost-input-strict/output.html b/...ver/new-server/transforms/modules/test/module-nomodule-localhost-input-strict/output.html
@@ -0,0 +1,6 @@
+<head>
+<script async="" src="http://localhost:8000/amp.js"></script>
+<script async="" src="https://cdn.ampproject.org/v0.js" nomodule=""></script>
+<script async="" src="https://cdn.ampproject.org/v0.mjs" type="module"></script>
+<script async="" src="http://localhost:8000/v0/amp-bind-0.1.js"></script>
+</head>
diff --git a/...ransforms/modules/test/esm-env/input.html → ...module-replace-nomodule-script/input.html b/...ransforms/modules/test/esm-env/input.html → ...module-replace-nomodule-script/input.html
@@ -1,3 +1,4 @@
 <head>
 <script async src="https://cdn.ampproject.org/v0.js"></script>
+<script async src="https://cdn.ampproject.org/v0/amp-bind-0.1.js"></script>
 </head>
diff --git a/...tem/server/new-server/transforms/modules/test/module-replace-nomodule-script/options.json b/...tem/server/new-server/transforms/modules/test/module-replace-nomodule-script/options.json
@@ -0,0 +1,4 @@
+{
+  "esm": true,
+  "fortesting": true
+}
diff --git a/...stem/server/new-server/transforms/modules/test/module-replace-nomodule-script/output.html b/...stem/server/new-server/transforms/modules/test/module-replace-nomodule-script/output.html
@@ -0,0 +1,4 @@
+<head>
+<script async="" src="https://cdn.ampproject.org/v0.mjs" type="module"></script>
+<script async="" src="https://cdn.ampproject.org/v0/amp-bind-0.1.mjs" type="module"></script>
+</head>
diff --git a/build-system/server/new-server/transforms/modules/test/skip-over-json-script/input.html b/build-system/server/new-server/transforms/modules/test/skip-over-json-script/input.html
@@ -0,0 +1,3 @@
+<head>
+<script type="application/json">{}</script>
+</head>
diff --git a/build-system/server/new-server/transforms/modules/test/skip-over-json-script/output.html b/build-system/server/new-server/transforms/modules/test/skip-over-json-script/output.html
@@ -0,0 +1,3 @@
+<head>
+<script type="application/json">{}</script>
+</head>
diff --git a/build-system/server/new-server/transforms/scripts/scripts-transform.ts b/build-system/server/new-server/transforms/scripts/scripts-transform.ts
@@ -15,21 +15,31 @@
  */
 
 import {PostHTML} from 'posthtml';
-import {URL} from 'url';
-import {isValidScript} from '../utilities/script';
+import {isJsonScript, isValidScript, tryGetUrl} from '../utilities/script';
 import {CDNURLToLocalDistURL} from '../utilities/cdn';
 import {OptionSet} from '../utilities/option-set';
+import {parse} from 'path';
 
 /**
  * For any script, with a valid path to AMP Project CDN, replace it with a local value.
  * @param script
  */
-function modifySrc(script: PostHTML.Node): PostHTML.Node {
-  if (!isValidScript(script)) {
+function modifySrc(script: PostHTML.Node, options: OptionSet): PostHTML.Node {
+  // Make sure that isJsonScript is used before `isValidScript`. We bail out
+  // early if the ScriptNode is of type="application/json" since it wouldn't
+  // have any src url to modify.
+  if (isJsonScript(script)) {
     return script;
   }
 
-  const src = CDNURLToLocalDistURL(new URL(script.attrs.src || '')).toString();
+  if (!isValidScript(script, options.looseScriptSrcCheck)) {
+    return script;
+  }
+
+  const url = tryGetUrl(script.attrs.src || '');
+  const parsedPath = parse(url.pathname);
+  const src = CDNURLToLocalDistURL(url, [null, null], parsedPath.ext, options.port, options.useMaxNames)
+      .toString();
   script.attrs.src = src;
   return script;
 }
@@ -39,6 +49,8 @@ function modifySrc(script: PostHTML.Node): PostHTML.Node {
  */
 export default function(options: OptionSet = {}): (tree: PostHTML.Node) => void {
   return function(tree: PostHTML.Node) {
-    tree.match({tag: 'script'}, modifySrc);
+    tree.match({tag: 'script'}, (script) => {
+      return modifySrc(script, options);
+    });
   }
 }
diff --git a/build-system/server/new-server/transforms/scripts/test/convert-to-max-names/input.html b/build-system/server/new-server/transforms/scripts/test/convert-to-max-names/input.html
@@ -0,0 +1,3 @@
+<script src="https://cdn.ampproject.org/v0.js" async></script>
+<script src="https://cdn.ampproject.org/v0/amp-bind-0.1.js" async></script>
+<script src="https://cdn.ampproject.org/v0/amp-bind-0.2.js" async></script>
diff --git a/build-system/server/new-server/transforms/scripts/test/convert-to-max-names/options.json b/build-system/server/new-server/transforms/scripts/test/convert-to-max-names/options.json
@@ -0,0 +1,3 @@
+{
+  "useMaxNames": true
+}
diff --git a/build-system/server/new-server/transforms/scripts/test/convert-to-max-names/output.html b/build-system/server/new-server/transforms/scripts/test/convert-to-max-names/output.html
@@ -0,0 +1,3 @@
+<script src="http://localhost:8000/dist/amp.js" async=""></script>
+<script src="http://localhost:8000/dist/v0/amp-bind-0.1.max.js" async=""></script>
+<script src="http://localhost:8000/dist/v0/amp-bind-0.2.max.js" async=""></script>
diff --git a/build-system/server/new-server/transforms/scripts/test/invalid-src-loose/input.html b/build-system/server/new-server/transforms/scripts/test/invalid-src-loose/input.html
@@ -0,0 +1,3 @@
+<script src="https://cdn/v0.js" async></script>
+<script src="amp.js" async></script>
+<script src="http://localhost:8000/amp.js" async></script>
diff --git a/build-system/server/new-server/transforms/scripts/test/invalid-src-loose/options.json b/build-system/server/new-server/transforms/scripts/test/invalid-src-loose/options.json
@@ -0,0 +1,3 @@
+{
+  "looseScriptSrcCheck": true
+}
diff --git a/build-system/server/new-server/transforms/scripts/test/invalid-src-loose/output.html b/build-system/server/new-server/transforms/scripts/test/invalid-src-loose/output.html
@@ -0,0 +1,3 @@
+<script src="http://localhost:8000/dist/v0.js" async=""></script>
+<script src="http://localhost:8000/dist/amp.js" async=""></script>
+<script src="http://localhost:8000/dist/amp.js" async=""></script>
diff --git a/...forms/scripts/test/invalid-src/input.html → ...cripts/test/invalid-src-strict/input.html b/...forms/scripts/test/invalid-src/input.html → ...cripts/test/invalid-src-strict/input.html
@@ -1 +1,2 @@
 <script src="https://cdn/v0.js" async></script>
+<script src="/dist/amp.js" async></script>
diff --git a/...orms/scripts/test/invalid-src/output.html → ...ripts/test/invalid-src-strict/output.html b/...orms/scripts/test/invalid-src/output.html → ...ripts/test/invalid-src-strict/output.html
@@ -1 +1,2 @@
 <script src="https://cdn/v0.js" async=""></script>
+<script src="/dist/amp.js" async=""></script>
diff --git a/build-system/server/new-server/transforms/scripts/test/port-transform-loose/input.html b/build-system/server/new-server/transforms/scripts/test/port-transform-loose/input.html
@@ -0,0 +1,5 @@
+<script async="" src="http://localhost:8000/amp.js"></script>
+<script async="" src="https://cdn.ampproject.org/v0.js" nomodule=""></script>
+<script async="" src="https://cdn.ampproject.org/v0.mjs" type="module"></script>
+<script async="" src="http://localhost:8000/v0/amp-bind-0.1.js"></script>
+<script async="" src="/dist/amp.js"></script>
diff --git a/build-system/server/new-server/transforms/scripts/test/port-transform-loose/options.json b/build-system/server/new-server/transforms/scripts/test/port-transform-loose/options.json
@@ -0,0 +1,4 @@
+{
+  "port": 9876,
+  "looseScriptSrcCheck": true
+}
diff --git a/build-system/server/new-server/transforms/scripts/test/port-transform-loose/output.html b/build-system/server/new-server/transforms/scripts/test/port-transform-loose/output.html
@@ -0,0 +1,5 @@
+<script async="" src="http://localhost:9876/dist/amp.js"></script>
+<script async="" src="http://localhost:9876/dist/v0.js" nomodule=""></script>
+<script async="" src="http://localhost:9876/dist/v0.mjs" type="module"></script>
+<script async="" src="http://localhost:9876/dist/v0/amp-bind-0.1.js"></script>
+<script async="" src="http://localhost:9876/dist/dist/amp.js"></script>
diff --git a/build-system/server/new-server/transforms/scripts/test/port-transform-strict/input.html b/build-system/server/new-server/transforms/scripts/test/port-transform-strict/input.html
@@ -0,0 +1,5 @@
+<script async="" src="http://localhost:8000/amp.js"></script>
+<script async="" src="https://cdn.ampproject.org/v0.js" nomodule=""></script>
+<script async="" src="https://cdn.ampproject.org/v0.mjs" type="module"></script>
+<script async="" src="http://localhost:8000/v0/amp-bind-0.1.js"></script>
+<script async="" src="/dist/amp.js"></script>
diff --git a/build-system/server/new-server/transforms/scripts/test/port-transform-strict/options.json b/build-system/server/new-server/transforms/scripts/test/port-transform-strict/options.json
@@ -0,0 +1,3 @@
+{
+  "port": 9876
+}
diff --git a/build-system/server/new-server/transforms/scripts/test/port-transform-strict/output.html b/build-system/server/new-server/transforms/scripts/test/port-transform-strict/output.html
@@ -0,0 +1,5 @@
+<script async="" src="http://localhost:8000/amp.js"></script>
+<script async="" src="http://localhost:9876/dist/v0.js" nomodule=""></script>
+<script async="" src="http://localhost:9876/dist/v0.mjs" type="module"></script>
+<script async="" src="http://localhost:8000/v0/amp-bind-0.1.js"></script>
+<script async="" src="/dist/amp.js"></script>
diff --git a/build-system/server/new-server/transforms/scripts/test/retain-extension/input.html b/build-system/server/new-server/transforms/scripts/test/retain-extension/input.html
@@ -0,0 +1,3 @@
+<script async="" src="https://cdn.ampproject.org/v0.js"></script>
+<script async="" src="https://cdn.ampproject.org/v0.mjs"></script>
+<script async="" src="https://cdn.ampproject.org/v0.sxg.js"></script>
diff --git a/build-system/server/new-server/transforms/scripts/test/retain-extension/output.html b/build-system/server/new-server/transforms/scripts/test/retain-extension/output.html
@@ -0,0 +1,3 @@
+<script async="" src="http://localhost:8000/dist/v0.js"></script>
+<script async="" src="http://localhost:8000/dist/v0.mjs"></script>
+<script async="" src="http://localhost:8000/dist/v0.sxg.js"></script>
diff --git a/build-system/server/new-server/transforms/scripts/test/runtime/input.html b/build-system/server/new-server/transforms/scripts/test/runtime/input.html
@@ -1 +1,2 @@
 <script src="https://cdn.ampproject.org/v0.js" async></script>
+<script src="https://cdn.ampproject.org/v0/amp-bind-0.1.js" async></script>
diff --git a/build-system/server/new-server/transforms/scripts/test/runtime/output.html b/build-system/server/new-server/transforms/scripts/test/runtime/output.html
@@ -1 +1,2 @@
 <script src="http://localhost:8000/dist/v0.js" async=""></script>
+<script src="http://localhost:8000/dist/v0/amp-bind-0.1.js" async=""></script>
diff --git a/build-system/server/new-server/transforms/scripts/test/skip-over-json-script/input.html b/build-system/server/new-server/transforms/scripts/test/skip-over-json-script/input.html
@@ -0,0 +1,3 @@
+<head>
+<script type="application/json">{}</script>
+</head>
diff --git a/build-system/server/new-server/transforms/scripts/test/skip-over-json-script/output.html b/build-system/server/new-server/transforms/scripts/test/skip-over-json-script/output.html
@@ -0,0 +1,3 @@
+<head>
+<script type="application/json">{}</script>
+</head>
diff --git a/build-system/server/new-server/transforms/stories/stories-transform.ts b/build-system/server/new-server/transforms/stories/stories-transform.ts