Avoid file name collisions from converted HTML files (#269)

* Convert javascript imports as inline module code in modules * update tests * handle for package layout as well * code cleanup * Add import references to a11ySuite in tests (#270) * Add import references to a11ySuite in tests * add TODO * document converter NOTE
Polymer · Jan 11, 2018 · fc040b2 · fc040b2
1 parent be701bc
commit fc040b2
Show file tree

Hide file tree

Showing 13 changed files with 206 additions and 45 deletions.
diff --git a/custom_typings/ast-types.d.ts b/custom_typings/ast-types.d.ts
@@ -36,6 +36,7 @@ declare module 'ast-types' {
   }
 
   interface VisitorContext {
+    abort(): void;
     traverse(nodePath: NodePath): void;
   }
 
@@ -45,7 +46,10 @@ declare module 'ast-types' {
         (this: VisitorContext, path: NodePath<estree.Program>): void|boolean;
     visitEmptyStatement?
         (this: VisitorContext, path: NodePath<estree.EmptyStatement>):
-            void|boolean;
+      void | boolean;
+    visitCallExpression?
+        (this: VisitorContext, path: NodePath<estree.CallExpression>):
+      void | boolean;
     visitBlockStatement?
         (this: VisitorContext, path: NodePath<estree.BlockStatement>):
             void|boolean;

diff --git a/fixtures/packages/paper-button/expected/test/paper-button.html b/fixtures/packages/paper-button/expected/test/paper-button.html
@@ -16,7 +16,7 @@
 
   <script src="../../../@webcomponents/webcomponentsjs/webcomponents-lite.js"></script>
   <script src="../../../wct-browser-legacy/browser.js"></script>
-  <script src="../../../@polymer/iron-test-helpers/mock-interactions.js"></script>
+  <script src="../../../@polymer/iron-test-helpers/mock-interactions.js" type="module"></script>
 
   <script type="module" src="../paper-button.js"></script>
 
@@ -32,6 +32,7 @@
   <script type="module">
 import '../paper-button.js';
 import { Base } from '../../../@polymer/polymer/polymer.js';
+import { a11ySuite } from '../../../wct-browser-legacy/a11ySuite.js';
 suite('<paper-button>', function() {
   var button;
 

diff --git a/src/cli/command-workspace.ts b/src/cli/command-workspace.ts
@@ -19,9 +19,9 @@ import {Workspace} from 'polymer-workspaces';
 
 import {CliOptions} from '../cli';
 import convertWorkspace from '../convert-workspace';
-import { testWorkspace, testWorkspaceInstallOnly } from '../test-workspace';
-import githubPushWorkspace from '../push-workspace';
 import npmPublishWorkspace from '../publish-workspace';
+import githubPushWorkspace from '../push-workspace';
+import {testWorkspace, testWorkspaceInstallOnly} from '../test-workspace';
 import {logStep} from '../util';
 
 const githubTokenMessage = `

diff --git a/src/document-converter.ts b/src/document-converter.ts
@@ -27,6 +27,7 @@ import * as recast from 'recast';
 import {ConversionSettings} from './conversion-settings';
 import {attachCommentsToFirstStatement, canDomModuleBeInlined, collectIdentifierNames, containsWriteToGlobalSettingsObject, createDomNodeInsertStatements, filterClone, findAvailableIdentifier, getCommentsBetween, getMemberPath, getNodePathInProgram, getPathOfAssignmentTo, getSetterName, insertStatementsIntoProgramBody, serializeNode, serializeNodeToTemplateLiteral} from './document-util';
 import {ConversionResult, JsExport} from './js-module';
+import {addA11ySuiteIfUsed} from './passes/add-a11y-suite-if-used';
 import {removeNamespaceInitializers} from './passes/remove-namespace-initializers';
 import {removeUnnecessaryEventListeners} from './passes/remove-unnecessary-waits';
 import {removeWrappingIIFEs} from './passes/remove-wrapping-iife';
@@ -86,6 +87,20 @@ const legacyJavascriptTypes: ReadonlySet<string|null> = new Set([
   'text/x-javascript',
 ]);
 
+/**
+ * This is a set of JavaScript files that we know to be converted as modules.
+ * This is neccessary because we don't definitively know the converted type
+ * of an external JavaScript file loaded as a normal script in
+ * a top-level HTML file.
+ *
+ * TODO(fks): Add the ability to know via conversion manifest support or
+ * convert dependencies as entire packages instead of file-by-file.
+ * (See https://github.com/Polymer/polymer-modulizer/issues/268)
+ */
+const knownScriptModules = new Set<string>([
+  'iron-test-helpers/mock-interactions.js',
+]);
+
 /**
  * Detect legacy JavaScript "type" attributes.
  */
@@ -235,28 +250,49 @@ export class DocumentConverter {
                 !this.conversionSettings.excludes.has(f.document.url));
   }
 
-  convertToJsModule(): ConversionResult {
+  private isInternalNonModuleImport(scriptImport: Import): boolean {
+    const oldScriptUrl = getDocumentUrl(scriptImport.document);
+    const newScriptUrl = this.convertScriptUrl(oldScriptUrl);
+    const isModuleImport =
+        dom5.getAttribute(scriptImport.astNode, 'type') === 'module';
+    const isInternalImport =
+        this.urlHandler.isImportInternal(this.convertedUrl, newScriptUrl);
+    return isInternalImport && !isModuleImport;
+  }
+
+  convertToJsModule(): ConversionResult[] {
     if (this._isWrapperHTMLDocument) {
-      return {
+      return [{
         originalUrl: this.originalUrl,
         convertedUrl: this.convertedUrl,
         convertedFilePath: getJsModuleConvertedFilePath(this.originalUrl),
         deleteOriginal: true,
         output: undefined,
-      };
+      }];
     }
 
     const combinedToplevelStatements = [];
+    const convertedHtmlScripts = new Set<Import>();
     let prevScriptNode: parse5.ASTNode|undefined = undefined;
-    for (const script of this.document.getFeatures({kind: 'js-document'})) {
+    for (const script of this.document.getFeatures()) {
+      let scriptDocument: Document;
+      if (script.kinds.has('html-script') &&
+          this.isInternalNonModuleImport(script as Import)) {
+        scriptDocument = (script as Import).document;
+        convertedHtmlScripts.add(script as Import);
+      } else if (script.kinds.has('js-document')) {
+        scriptDocument = script as Document;
+      } else {
+        continue;
+      }
       const scriptProgram =
-          recast.parse(script.parsedDocument.contents).program;
+          recast.parse(scriptDocument.parsedDocument.contents).program;
       rewriteToplevelThis(scriptProgram);
       // We need to inline templates on a per-script basis, otherwise we run
       // into trouble matching up analyzer AST nodes with our own.
-      this.inlineTemplates(scriptProgram, script);
+      this.inlineTemplates(scriptProgram, scriptDocument);
       if (this.conversionSettings.addImportPath) {
-        this.addImportPathsToElements(scriptProgram, script);
+        this.addImportPathsToElements(scriptProgram, scriptDocument);
       }
       const comments: string[] = getCommentsBetween(
           this.document.parsedDocument.ast, prevScriptNode, script.astNode);
@@ -265,6 +301,7 @@ export class DocumentConverter {
       combinedToplevelStatements.push(...statements);
       prevScriptNode = script.astNode;
     }
+
     const trailingComments = getCommentsBetween(
         this.document.parsedDocument.ast, prevScriptNode, undefined);
     const maybeCommentStatement =
@@ -274,13 +311,29 @@ export class DocumentConverter {
     removeUnnecessaryEventListeners(program);
     removeWrappingIIFEs(program);
     const importedReferences = this.collectNamespacedReferences(program);
+    const results: ConversionResult[] = [];
+
     // Add imports for every non-module <script> tag to just import the file
     // itself.
     for (const scriptImport of this.document.getFeatures(
              {kind: 'html-script'})) {
       const oldScriptUrl = getDocumentUrl(scriptImport.document);
       const newScriptUrl = this.convertScriptUrl(oldScriptUrl);
-      importedReferences.set(newScriptUrl, new Set());
+      if (convertedHtmlScripts.has(scriptImport)) {
+        // NOTE: This deleted script file path *may* === this document's final
+        // converted file path. Because results are written in order, the
+        // final result (this document) has the final say, and any previous
+        // deletions won't overwrite/conflict with the final document.
+        results.push({
+          originalUrl: oldScriptUrl,
+          convertedUrl: newScriptUrl,
+          convertedFilePath: getJsModuleConvertedFilePath(oldScriptUrl),
+          deleteOriginal: true,
+          output: undefined,
+        });
+      } else {
+        importedReferences.set(newScriptUrl, new Set());
+      }
     }
     this.addJsImports(program, importedReferences);
     this.insertCodeToGenerateHtmlElements(program);
@@ -301,7 +354,7 @@ export class DocumentConverter {
     const outputProgram =
         recast.print(program, {quote: 'single', wrapColumn: 80, tabWidth: 2});
 
-    return {
+    results.push({
       originalUrl: this.originalUrl,
       convertedUrl: this.convertedUrl,
       convertedFilePath: getJsModuleConvertedFilePath(this.originalUrl),
@@ -312,7 +365,8 @@ export class DocumentConverter {
         exportedNamespaceMembers: exportMigrationRecords,
         es6Exports: new Set(exportMigrationRecords.map((r) => r.es6ExportName))
       }
-    };
+    });
+    return results;
   }
 
   convertAsToplevelHtmlDocument(): ConversionResult {
@@ -368,6 +422,7 @@ export class DocumentConverter {
           [],
           dom5.childNodesIncludeTemplate));
     }
+
     for (const astNode of scriptsToConvert) {
       if (!isLegacyJavaScriptTag(astNode)) {
         continue;
@@ -418,6 +473,7 @@ export class DocumentConverter {
       const replacementText = serializeNode(scriptTag);
       edits.push({offsets, replacementText});
     }
+
     for (const scriptImport of this.document.getFeatures(
              {kind: 'html-script'})) {
       // ignore fake script imports injected by various hacks in the
@@ -433,10 +489,18 @@ export class DocumentConverter {
       const offsets = htmlDocument.sourceRangeToOffsets(
           htmlDocument.sourceRangeForNode(scriptImport.astNode)!);
 
-      const correctedUrl = this.formatImportUrl(
-          this.convertDocumentUrl(getDocumentUrl(scriptImport.document)),
-          scriptImport.url);
-      dom5.setAttribute(scriptImport.astNode, 'src', correctedUrl);
+      const convertedUrl =
+          this.convertDocumentUrl(getDocumentUrl(scriptImport.document));
+      const formattedUrl = this.formatImportUrl(convertedUrl, scriptImport.url);
+      dom5.setAttribute(scriptImport.astNode, 'src', formattedUrl);
+
+      // Temporary: Check if imported script is a known module.
+      // See `knownScriptModules` for more information.
+      for (const importUrlEnding of knownScriptModules) {
+        if (scriptImport.url.endsWith(importUrlEnding)) {
+          dom5.setAttribute(scriptImport.astNode, 'type', 'module');
+        }
+      }
 
       edits.push(
           {offsets, replacementText: serializeNode(scriptImport.astNode)});
@@ -516,6 +580,10 @@ export class DocumentConverter {
     removeUnnecessaryEventListeners(program);
     removeWrappingIIFEs(program);
     const importedReferences = this.collectNamespacedReferences(program);
+    const wasA11ySuiteAdded = addA11ySuiteIfUsed(
+        program,
+        this.formatImportUrl(this.urlHandler.createConvertedUrl(
+            'wct-browser-legacy/a11ySuite.js')));
     const wereImportsAdded = this.addJsImports(program, importedReferences);
     // Don't convert the HTML.
     // Don't inline templates, they're fine where they are.
@@ -532,7 +600,7 @@ export class DocumentConverter {
 
     this.warnOnDangerousReferences(program);
 
-    if (!wereImportsAdded) {
+    if (!wasA11ySuiteAdded && !wereImportsAdded) {
       return undefined;  // no imports, no reason to convert to a module
     }
 
@@ -723,10 +791,8 @@ export class DocumentConverter {
               'init', jsc.identifier('_template'), templateLiteral));
         }
       } else {
-        console.error(
-            `Internal Error, Class or CallExpression expected, got ${
-                                                                     node.type
-                                                                   }`);
+        console.error(`Internal Error, Class or CallExpression expected, got ${
+            node.type}`);
       }
     }
   }
@@ -786,10 +852,8 @@ export class DocumentConverter {
               'init', jsc.identifier('importPath'), importMetaUrl));
         }
       } else {
-        console.error(
-            `Internal Error, Class or CallExpression expected, got ${
-                                                                     node.type
-                                                                   }`);
+        console.error(`Internal Error, Class or CallExpression expected, got ${
+            node.type}`);
       }
     }
   }
@@ -1033,6 +1097,7 @@ export class DocumentConverter {
       jsImportDeclarations.push(...getImportDeclarations(
           jsFormattedImportUrl, namedExports, references, usedIdentifiers));
     }
+
     // Prepend JS imports into the program body
     program.body.splice(0, 0, ...jsImportDeclarations);
     // Return true if any imports were added, false otherwise

diff --git a/src/js-module.ts b/src/js-module.ts
@@ -60,7 +60,7 @@ export class JsExport {
   /**
    * Exported name, ie Foo for `export Foo`;
    *
-   * The name * represents the entire module, for when the key in the
+   * The name represents the entire module, for when the key in the
    * namespacedExports Map represents a namespace object.
    */
   readonly name: string;

diff --git a/src/passes/add-a11y-suite-if-used.ts b/src/passes/add-a11y-suite-if-used.ts
@@ -0,0 +1,51 @@
+/**
+ * @license
+ * Copyright (c) 2017 The Polymer Project Authors. All rights reserved.
+ * This code may only be used under the BSD style license found at
+ * http://polymer.github.io/LICENSE.txt
+ * The complete set of authors may be found at
+ * http://polymer.github.io/AUTHORS.txt
+ * The complete set of contributors may be found at
+ * http://polymer.github.io/CONTRIBUTORS.txt
+ * Code distributed by Google as part of the polymer project is also
+ * subject to an additional IP rights grant found at
+ * http://polymer.github.io/PATENTS.txt
+ */
+
+import * as astTypes from 'ast-types';
+import {NodePath} from 'ast-types';
+import * as estree from 'estree';
+import * as jsc from 'jscodeshift';
+
+/**
+ * Rewrite references in a program from their original names to the local names
+ * based on the new named exports system.
+ *
+ * TODO(fks): This standalone A11ySuite handling could be brought into normal
+ * `collectNamespacedReferences()` logic if we added `A11ySuite()` to the known
+ * export graph.
+ */
+export function addA11ySuiteIfUsed(
+    program: estree.Program, a11ySuiteUrl: string): boolean {
+  let isFound = false;
+  astTypes.visit(program, {
+    visitCallExpression(path: NodePath<estree.CallExpression>) {
+      const callee = path.node.callee as estree.Identifier;
+      const name = callee.name;
+      if (name === 'a11ySuite') {
+        isFound = true;
+        this.abort();
+      }
+      this.traverse(path);
+    }
+  });
+
+  if (!isFound) {
+    return false;
+  }
+
+  program.body.unshift(jsc.importDeclaration(
+      [jsc.importSpecifier(jsc.identifier('a11ySuite'))],
+      jsc.literal(a11ySuiteUrl)));
+  return true;
+}
diff --git a/src/project-converter.ts b/src/project-converter.ts
@@ -73,9 +73,7 @@ export class ProjectConverter {
     console.assert(
         document.kinds.has('html-document'),
         `convertDocument() must be called with an HTML document, but got ${
-                                                                           document
-                                                                               .kinds
-                                                                         }`);
+            document.kinds}`);
     try {
       this.conversionSettings.includes.has(document.url) ?
           this.convertDocumentToJs(document, new Set()) :
@@ -136,8 +134,9 @@ export class ProjectConverter {
         this.namespacedExports,
         this.urlHandler,
         this.conversionSettings);
-    const newModule = documentConverter.convertToJsModule();
-    this._handleConversionResult(newModule);
+    documentConverter.convertToJsModule().forEach((result) => {
+      this._handleConversionResult(result);
+    });
   }
 
   /**

diff --git a/src/publish-workspace.ts b/src/publish-workspace.ts
@@ -14,10 +14,9 @@
 
 import chalk from 'chalk';
 import * as inquirer from 'inquirer';
-import {WorkspaceRepo, publishPackagesToNpm} from 'polymer-workspaces';
+import {publishPackagesToNpm, WorkspaceRepo} from 'polymer-workspaces';
 
 export default async function run(reposToConvert: WorkspaceRepo[]) {
-
   console.log(
       chalk.dim('[1/3] ') + chalk.magenta(`Setting up publish to npm...`));