Roll forward: "Allow the transpiler to be configured with a feature s…

…et output rather than to ES5." Allow ClosureBundler to be configured later with a different transpiler. When bundling only remove modules using the transpiler. NEW: Have the internal ES5 transpiler always transpile to ES5. Go back to ClosureBundler having 2 transpilers so that ES6 modules are always transformed. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=200790289
google · Jun 18, 2018 · 07bd5b6 · 07bd5b6
1 parent c654d8e
commit 07bd5b6
Show file tree

Hide file tree

Showing 4 changed files with 122 additions and 112 deletions.
diff --git a/src/com/google/javascript/jscomp/CommandLineRunner.java b/src/com/google/javascript/jscomp/CommandLineRunner.java
@@ -30,11 +30,14 @@
 import com.google.common.io.Files;
 import com.google.javascript.jscomp.AbstractCommandLineRunner.CommandLineConfig.ErrorFormatOption;
 import com.google.javascript.jscomp.CompilerOptions.IsolationMode;
+import com.google.javascript.jscomp.CompilerOptions.LanguageMode;
 import com.google.javascript.jscomp.SourceMap.LocationMapping;
 import com.google.javascript.jscomp.deps.ClosureBundler;
 import com.google.javascript.jscomp.deps.ModuleLoader;
 import com.google.javascript.jscomp.parsing.parser.FeatureSet;
+import com.google.javascript.jscomp.parsing.parser.FeatureSet.Feature;
 import com.google.javascript.jscomp.transpile.BaseTranspiler;
+import com.google.javascript.jscomp.transpile.BaseTranspiler.CompilerSupplier;
 import com.google.javascript.jscomp.transpile.Transpiler;
 import com.google.javascript.rhino.TokenStream;
 import com.google.protobuf.TextFormat;
@@ -1843,13 +1846,9 @@ protected Compiler createCompiler() {
     return new Compiler(getErrorPrintStream());
   }
 
-  private ClosureBundler bundler;
-
-  private ClosureBundler getBundler() {
-    if (bundler != null) {
-      return bundler;
-    }
-
+  @Override
+  protected void prepForBundleAndAppendTo(Appendable out, CompilerInput input, String content)
+      throws IOException {
     ImmutableList<String> moduleRoots;
     if (!flags.moduleRoot.isEmpty()) {
       moduleRoots = ImmutableList.copyOf(flags.moduleRoot);
@@ -1858,25 +1857,29 @@ private ClosureBundler getBundler() {
     }
 
     CompilerOptions options = createOptions();
-    return bundler = new ClosureBundler(
-        Transpiler.NULL,
-        new BaseTranspiler(
-            new BaseTranspiler.EsmToCjsCompilerSupplier(
-                options.getModuleResolutionMode(),
-                moduleRoots,
-                options.getBrowserResolverPrefixReplacements()),
-            /* runtimeLibraryName= */ ""));
-  }
 
-  @Override
-  protected void prepForBundleAndAppendTo(Appendable out, CompilerInput input, String content)
-      throws IOException {
-    getBundler().withPath(input.getName()).appendTo(out, input, content);
+    new ClosureBundler(
+            "es6".equals(input.getLoadFlags().get("module"))
+                ? new BaseTranspiler(
+                    new CompilerSupplier(
+                        LanguageMode.ECMASCRIPT_NEXT.toFeatureSet().without(Feature.MODULES),
+                        options.getModuleResolutionMode(),
+                        moduleRoots,
+                        options.getBrowserResolverPrefixReplacements()),
+                    /* runtimeLibraryName= */ "")
+                : Transpiler.NULL)
+        .withPath(input.getName())
+        .appendTo(out, input, content);
   }
 
   @Override
   protected void appendRuntimeTo(Appendable out) throws IOException {
-    getBundler().appendRuntimeTo(out);
+    new ClosureBundler(
+            new BaseTranspiler(
+                new CompilerSupplier(),
+                // Default runtime is the module runtime.
+                /* runtimeLibraryName= */ ""))
+        .appendRuntimeTo(out);
   }
 
   @Override

diff --git a/src/com/google/javascript/jscomp/deps/ClosureBundler.java b/src/com/google/javascript/jscomp/deps/ClosureBundler.java
@@ -51,7 +51,7 @@ public ClosureBundler() {
   }
 
   public ClosureBundler(Transpiler transpiler) {
-    this(transpiler, BaseTranspiler.ES_MODULE_TO_CJS_TRANSPILER);
+    this(transpiler, BaseTranspiler.LATEST_TRANSPILER);
   }
 
   public ClosureBundler(Transpiler transpiler, Transpiler es6ModuleTranspiler) {
@@ -79,6 +79,20 @@ private ClosureBundler(
     this.es6ModuleTranspiler = es6ModuleTranspiler;
   }
 
+  public ClosureBundler withTranspilers(
+      Transpiler newTranspiler, Transpiler newEs6ModuleTranspiler) {
+    return new ClosureBundler(
+        newTranspiler, newEs6ModuleTranspiler, mode, sourceUrl, path, sourceMapCache);
+  }
+
+  public ClosureBundler withTranspiler(Transpiler newTranspiler) {
+    return withTranspilers(newTranspiler, es6ModuleTranspiler);
+  }
+
+  public ClosureBundler withEs6ModuleTranspiler(Transpiler newEs6ModuleTranspiler) {
+    return withTranspilers(transpiler, newEs6ModuleTranspiler);
+  }
+
   public final ClosureBundler useEval(boolean useEval) {
     EvalMode newMode = useEval ? EvalMode.EVAL : EvalMode.NORMAL;
     return new ClosureBundler(

diff --git a/src/com/google/javascript/jscomp/transpile/BaseTranspiler.java b/src/com/google/javascript/jscomp/transpile/BaseTranspiler.java
@@ -27,7 +27,6 @@
 import com.google.javascript.jscomp.CompilerOptions.LanguageMode;
 import com.google.javascript.jscomp.DiagnosticGroup;
 import com.google.javascript.jscomp.DiagnosticType;
-import com.google.javascript.jscomp.Es6RewriteModulesToCommonJsModules;
 import com.google.javascript.jscomp.PropertyRenamingPolicy;
 import com.google.javascript.jscomp.Result;
 import com.google.javascript.jscomp.SourceFile;
@@ -37,7 +36,8 @@
 import com.google.javascript.jscomp.deps.ModuleLoader;
 import com.google.javascript.jscomp.deps.ModuleLoader.PathEscaper;
 import com.google.javascript.jscomp.deps.ModuleLoader.ResolutionMode;
-import com.google.javascript.rhino.Node;
+import com.google.javascript.jscomp.parsing.parser.FeatureSet;
+import com.google.javascript.jscomp.parsing.parser.FeatureSet.Feature;
 import java.io.IOException;
 import java.net.URI;
 import java.net.URISyntaxException;
@@ -74,11 +74,17 @@ public String runtime() {
     return sb.toString();
   }
 
-  public static final BaseTranspiler ES5_TRANSPILER = new BaseTranspiler(
-      new CompilerSupplier(), "es6_runtime");
+  public static final BaseTranspiler LATEST_TRANSPILER = to(FeatureSet.latest(), "");
 
-  public static final BaseTranspiler ES_MODULE_TO_CJS_TRANSPILER =
-      new BaseTranspiler(new EsmToCjsCompilerSupplier(), "");
+  public static final BaseTranspiler ES5_TRANSPILER = to(LanguageMode.ECMASCRIPT5.toFeatureSet());
+
+  public static final BaseTranspiler to(FeatureSet featureSet, String runtime) {
+    return new BaseTranspiler(new CompilerSupplier(featureSet), runtime);
+  }
+
+  public static final BaseTranspiler to(FeatureSet featureSet) {
+    return to(featureSet, "es6_runtime");
+  }
 
   /**
    * Wraps the Compiler into a more relevant interface, making it
@@ -92,10 +98,19 @@ public static class CompilerSupplier {
     protected final ResolutionMode moduleResolution;
     protected final ImmutableList<String> moduleRoots;
     protected final ImmutableMap<String, String> prefixReplacements;
+    protected final FeatureSet outputFeatureSet;
 
     public CompilerSupplier() {
+      this(LanguageMode.ECMASCRIPT5.toFeatureSet());
+    }
+
+    public CompilerSupplier(FeatureSet outputFeatureSet) {
       // Use the default resolution mode
-      this(new CompilerOptions().getModuleResolutionMode(), ImmutableList.of(), ImmutableMap.of());
+      this(
+          outputFeatureSet,
+          new CompilerOptions().getModuleResolutionMode(),
+          ImmutableList.of(),
+          ImmutableMap.of());
     }
 
     /**
@@ -106,9 +121,11 @@ public CompilerSupplier() {
      *     ModuleLoader.ResolutionMode#BROWSER_WITH_TRANSFORMED_PREFIXES}
      */
     public CompilerSupplier(
+        FeatureSet outputFeatureSet,
         ModuleLoader.ResolutionMode moduleResolution,
         ImmutableList<String> moduleRoots,
         ImmutableMap<String, String> prefixReplacements) {
+      this.outputFeatureSet = outputFeatureSet;
       this.moduleResolution = moduleResolution;
       this.moduleRoots = moduleRoots;
       this.prefixReplacements = prefixReplacements;
@@ -157,16 +174,8 @@ protected CompilerOptions options() {
 
     protected void setOptions(CompilerOptions options) {
       options.setLanguageIn(LanguageMode.ECMASCRIPT_NEXT);
-      // TODO(sdh): It would be nice to allow people to output code in
-      // strict mode.  But currently we swallow all the input language
-      // strictness checks, and there are various tests that are never
-      // compiled and so are broken when we output 'use strict'.  We
-      // could consider adding some sort of logging/warning/error in
-      // cases where the input was not strict, though there could still
-      // be semantic differences even if syntax is strict.  Possibly
-      // the first step would be to allow the option of outputting strict
-      // and then change the default and see what breaks.  b/33005948
-      options.setLanguageOut(LanguageMode.ECMASCRIPT5);
+      options.setOutputFeatureSet(outputFeatureSet.without(Feature.MODULES));
+      options.setEmitUseStrict(false);
       options.setQuoteKeywordProperties(true);
       options.setSkipNonTranspilationPasses(true);
       options.setVariableRenaming(VariableRenamingPolicy.OFF);
@@ -206,76 +215,6 @@ protected void setOptions(CompilerOptions options) {
         DiagnosticType.error("JSC_CANNOT_CONVERT", ""));
   }
 
-  /**
-   * CompilerSupplier that only transforms EcmaScript Modules into a form that can be safely
-   * transformed on a file by file basis and concatenated.
-   */
-  public static class EsmToCjsCompilerSupplier extends CompilerSupplier {
-
-    public EsmToCjsCompilerSupplier() {
-      super();
-    }
-
-    public EsmToCjsCompilerSupplier(
-        ModuleLoader.ResolutionMode moduleResolution,
-        ImmutableList<String> moduleRoots,
-        ImmutableMap<String, String> prefixReplacements) {
-      super(moduleResolution, moduleRoots, prefixReplacements);
-    }
-
-
-    @Override
-    public CompileResult compile(URI path, String code) {
-      CompilerOptions options = new CompilerOptions();
-      options.setLanguageIn(LanguageMode.ECMASCRIPT_NEXT);
-      options.setEmitUseStrict(false);
-      options.setSourceMapOutputPath("/dev/null");
-      options.setSourceMapIncludeSourcesContent(true);
-      options.setPrettyPrint(true);
-      options.setModuleResolutionMode(moduleResolution);
-      options.setModuleRoots(moduleRoots);
-      options.setBrowserResolverPrefixReplacements(prefixReplacements);
-
-      // Create a compiler and run specifically this one pass on it.
-      Compiler compiler = compiler();
-      compiler.init(
-          ImmutableList.of(),
-          ImmutableList.of(SourceFile.fromCode(path.toString(), code)),
-          options);
-      compiler.parseForCompilation();
-
-      boolean transpiled = false;
-
-      if (!compiler.hasErrors()
-          && compiler.getRoot().getSecondChild().getFirstFirstChild().isModuleBody()
-          && !compiler
-              .getRoot()
-              .getSecondChild()
-              .getFirstChild()
-              .getBooleanProp(Node.GOOG_MODULE)) {
-        new Es6RewriteModulesToCommonJsModules(compiler)
-            .process(null, compiler.getRoot().getSecondChild());
-        compiler.getRoot().getSecondChild().getFirstChild().putBooleanProp(Node.TRANSPILED, true);
-        transpiled = true;
-      }
-
-      Result result = compiler.getResult();
-      String source = compiler.toSource();
-      StringBuilder sourceMap = new StringBuilder();
-      if (result.sourceMap != null) {
-        try {
-          result.sourceMap.appendTo(sourceMap, path.toString());
-        } catch (IOException e) {
-          // impossible, and not a big deal even if it did happen.
-        }
-      }
-      if (result.errors.length > 0) {
-        throw new TranspilationException(compiler, result.errors, result.warnings);
-      }
-      return new CompileResult(source, transpiled, transpiled ? sourceMap.toString() : "");
-    }
-  }
-
   /**
    * The source together with the additional compilation results.
    */

diff --git a/test/com/google/javascript/jscomp/deps/ClosureBundlerTest.java b/test/com/google/javascript/jscomp/deps/ClosureBundlerTest.java
@@ -20,6 +20,8 @@
 import static org.mockito.Mockito.when;
 
 import com.google.common.collect.ImmutableMap;
+import com.google.javascript.jscomp.bundle.TranspilationException;
+import com.google.javascript.jscomp.transpile.BaseTranspiler;
 import com.google.javascript.jscomp.transpile.TranspileResult;
 import com.google.javascript.jscomp.transpile.Transpiler;
 import java.io.IOException;
@@ -140,9 +142,10 @@ public void testEs6Module() throws IOException {
     String input =
         "import {x} from './other.js';\n"
             + "export {x as y};"
-            + "var local;\n"
+            + "let local;\n"
             + "export function foo() { return local; }\n";
-    ClosureBundler bundler = new ClosureBundler().withPath("nested/path/foo.js");
+    ClosureBundler bundler =
+        new ClosureBundler(BaseTranspiler.LATEST_TRANSPILER).withPath("nested/path/foo.js");
     StringBuilder sb = new StringBuilder();
     bundler.appendRuntimeTo(sb);
     bundler.appendTo(
@@ -163,10 +166,61 @@ public void testEs6Module() throws IOException {
                 + "    return module$nested$path$other.x;\n"
                 + "  }}});\n"
                 + "  var module$nested$path$other = $$require(\"nested/path/other.js\");\n"
-                + "  var local;\n"
+                + "  let local;\n"
                 + "  function foo() {\n"
                 + "    return local;\n"
                 + "  }\n"
                 + "}, \"nested/path/foo.js\", [\"nested/path/other.js\"]);\n");
   }
+
+  public void testPassThroughIfNoTranspilationNeeded() throws Exception {
+    String input = "/** Hello Comments! */ const s = 0;\n  let intended;";
+    ClosureBundler bundler = new ClosureBundler(BaseTranspiler.LATEST_TRANSPILER);
+    StringBuilder sb = new StringBuilder();
+    bundler.appendTo(
+        sb,
+        SimpleDependencyInfo.builder("", "").build(),
+        input);
+    assertThat(sb.toString()).isEqualTo(input);
+  }
+
+  public void testCommentsAndFormattingRemovedWithTranspilation() throws Exception {
+    String input = "/** Hello Comments! */ const s = 0;\n  let intended;";
+    ClosureBundler bundler = new ClosureBundler(BaseTranspiler.ES5_TRANSPILER);
+    StringBuilder sb = new StringBuilder();
+    bundler.appendTo(
+        sb,
+        SimpleDependencyInfo.builder("", "").build(),
+        input);
+    assertThat(sb.toString()).isEqualTo("var s = 0;\nvar intended;\n");
+  }
+
+  public void testFullWidthLowLineWithDefaultTranspilerIsOkay() throws Exception {
+    // The last character is something the compiler doesn't handle correctly
+    String input = "var ａｅｓｔｈｅｔｉｃ＿";
+    ClosureBundler bundler = new ClosureBundler();
+    StringBuilder sb = new StringBuilder();
+    bundler.appendTo(
+        sb,
+        SimpleDependencyInfo.builder("", "").build(),
+        input);
+    assertThat(sb.toString()).isEqualTo(input);
+  }
+
+  // TODO(johnplaisted): If / when the compiler can parse full width low line in identifiers
+  // this should be okay to be transpiled.
+  public void testFullWidthLowLineInTranspiledCodeIsError() throws Exception {
+    // The last character is something the compiler doesn't handle correctly
+    String input = "let ａｅｓｔｈｅｔｉｃ＿";
+    ClosureBundler bundler = new ClosureBundler(BaseTranspiler.ES5_TRANSPILER);
+    StringBuilder sb = new StringBuilder();
+    try {
+      bundler.appendTo(sb, SimpleDependencyInfo.builder("", "").build(), input);
+      fail("Expected an exception");
+    } catch (TranspilationException e) {
+      assertThat(e)
+          .hasMessageThat()
+          .contains("Parse error. Character '＿' (U+FF3F) is not a valid identifier start char");
+    }
+  }
 }