Fix bundling where requires end up being a URL - do not escape the path.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=199157773

src/com/google/javascript/jscomp/Compiler.java

@@ -1714,7 +1714,8 @@ Node parseInputs() {
                 options.moduleRoots,
                 inputs,
                 moduleResolverFactory,
-                ModuleLoader.PathResolver.RELATIVE);
+                ModuleLoader.PathResolver.RELATIVE,
+                options.getPathEscaper());
       } else {
         // Use an empty module loader if we're not actually dealing with modules.
         this.moduleLoader = ModuleLoader.EMPTY;

src/com/google/javascript/jscomp/CompilerOptions.java

@@ -1197,6 +1197,8 @@ public void setWrapGoogModulesForWhitespaceOnly(boolean enable) {
    */
   private ImmutableMap<String, String> browserResolverPrefixReplacements;
 
+  private ModuleLoader.PathEscaper pathEscaper;
+
   /** Which entries to look for in package.json files when processing modules */
   List<String> packageJsonEntryNames;
 
@@ -1226,6 +1228,7 @@ public CompilerOptions() {
     // Modules
     moduleResolutionMode = ModuleLoader.ResolutionMode.BROWSER;
     packageJsonEntryNames = ImmutableList.of("browser", "module", "main");
+    pathEscaper = ModuleLoader.PathEscaper.ESCAPE;
 
     // Checks
     skipNonTranspilationPasses = false;
@@ -2863,6 +2866,14 @@ public void setBrowserResolverPrefixReplacements(
     this.browserResolverPrefixReplacements = browserResolverPrefixReplacements;
   }
 
+  public void setPathEscaper(ModuleLoader.PathEscaper pathEscaper) {
+    this.pathEscaper = pathEscaper;
+  }
+
+  public ModuleLoader.PathEscaper getPathEscaper() {
+    return pathEscaper;
+  }
+
   public List<String> getPackageJsonEntryNames() {
     return this.packageJsonEntryNames;
   }
@@ -2909,6 +2920,7 @@ public String toString() {
             .add("appNameStr", appNameStr)
             .add("assumeClosuresOnlyCaptureReferences", assumeClosuresOnlyCaptureReferences)
             .add("assumeStrictThis", assumeStrictThis())
+            .add("browserResolverPrefixReplacements", browserResolverPrefixReplacements)
             .add("brokenClosureRequiresLevel", brokenClosureRequiresLevel)
             .add("checkDeterminism", getCheckDeterminism())
             .add("checkGlobalNamesLevel", checkGlobalNamesLevel)
@@ -3014,6 +3026,7 @@ public String toString() {
                 "parentChunkCanSeeSymbolsDeclaredInChildren",
                 parentChunkCanSeeSymbolsDeclaredInChildren)
             .add("parseJsDocDocumentation", isParseJsDocDocumentation())
+            .add("pathEscaper", pathEscaper)
             .add("polymerVersion", polymerVersion)
             .add("preferLineBreakAtEndOfFile", preferLineBreakAtEndOfFile)
             .add("preferSingleQuotes", preferSingleQuotes)

src/com/google/javascript/jscomp/Es6RewriteModulesToCommonJsModules.java

@@ -20,6 +20,7 @@
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback;
+import com.google.javascript.jscomp.deps.ModuleLoader;
 import com.google.javascript.jscomp.deps.ModuleLoader.ModulePath;
 import com.google.javascript.jscomp.parsing.parser.FeatureSet.Feature;
 import com.google.javascript.rhino.IR;
@@ -85,6 +86,20 @@ private static class LocalQName {
     }
   }
 
+  /**
+   * Normalizes a registered or import path.
+   *
+   * <p>Absolute import paths need to match the registered path exactly. Some {@link
+   * ModuleLoader.ModulePath}s will have a leading slash and some won't. So in order to have
+   * everything line up AND preserve schemes (if they exist) then just strip leading /.
+   */
+  private static String normalizePath(String path) {
+    if (path.startsWith("/")) {
+      return path.substring(1);
+    }
+    return path;
+  }
+
   /**
    * Rewrites a single ES6 module into a CommonJS like module designed to be loaded in the
    * compiler's module runtime.
@@ -113,7 +128,7 @@ private class Rewriter extends AbstractPostOrderCallback {
     public void visit(NodeTraversal t, Node n, Node parent) {
       switch (n.getToken()) {
         case IMPORT:
-          visitImport(n);
+          visitImport(t.getInput().getPath(), n);
           break;
         case EXPORT:
           visitExport(t, n, parent);
@@ -296,11 +311,10 @@ private void registerAndLoadModule(NodeTraversal t) {
               IR.call(
                   IR.getprop(IR.name("$jscomp"), IR.string("registerAndLoadModule")),
                   moduleFunction,
-                  // Specifically use the input's name rather than modulePath.toString(). The former
-                  // is the raw path and the latter is encoded (special characters are replaced).
-                  // This is designed to run in a web browser and we want to preserve the URL given
-                  // to us. But the encodings will replace : with - due to windows.
-                  IR.string(t.getInput().getName()),
+                  // Resolving this path enables removing module roots from this path.
+                  IR.string(
+                      normalizePath(
+                          compiler.getModuleLoader().resolve(t.getInput().getName()).toString())),
                   shallowDeps));
 
       script.addChildToBack(exprResult.useSourceInfoIfMissingFromForTree(script));
@@ -344,8 +358,12 @@ private void addExport(Node definePropertiesLit, String exportedName, LocalQName
       compiler.reportChangeToChangeScope(getterFunction);
     }
 
-    private void visitImport(Node importDecl) {
-      importRequests.add(importDecl.getLastChild().getString());
+    private void visitImport(ModuleLoader.ModulePath path, Node importDecl) {
+      // Normalize the import path according to the module resolution scheme so that bundles are
+      // compatible with the compiler's module loader options.
+      importRequests.add(
+          normalizePath(
+              path.resolveModuleAsPath(importDecl.getLastChild().getString()).toString()));
       imports.add(importDecl);
     }
 

src/com/google/javascript/jscomp/deps/BrowserModuleResolver.java

@@ -22,6 +22,7 @@
 import com.google.javascript.jscomp.ErrorHandler;
 import com.google.javascript.jscomp.JSError;
 import com.google.javascript.jscomp.deps.ModuleLoader.ModuleResolverFactory;
+import com.google.javascript.jscomp.deps.ModuleLoader.PathEscaper;
 import javax.annotation.Nullable;
 
 /**
@@ -36,8 +37,9 @@ public class BrowserModuleResolver extends ModuleResolver {
   public BrowserModuleResolver(
       ImmutableSet<String> modulePaths,
       ImmutableList<String> moduleRootPaths,
-      ErrorHandler errorHandler) {
-    super(modulePaths, moduleRootPaths, errorHandler);
+      ErrorHandler errorHandler,
+      PathEscaper pathEscaper) {
+    super(modulePaths, moduleRootPaths, errorHandler, pathEscaper);
   }
 
   @Override

src/com/google/javascript/jscomp/deps/BrowserWithTransformedPrefixesModuleResolver.java

@@ -27,6 +27,7 @@
 import com.google.javascript.jscomp.ErrorHandler;
 import com.google.javascript.jscomp.JSError;
 import com.google.javascript.jscomp.deps.ModuleLoader.ModuleResolverFactory;
+import com.google.javascript.jscomp.deps.ModuleLoader.PathEscaper;
 import java.util.Comparator;
 import java.util.Set;
 import javax.annotation.Nullable;
@@ -56,9 +57,10 @@ public Factory(
     public ModuleResolver create(
         ImmutableSet<String> modulePaths,
         ImmutableList<String> moduleRootPaths,
-        ErrorHandler errorHandler) {
+        ErrorHandler errorHandler,
+        PathEscaper pathEscaper) {
       return new BrowserWithTransformedPrefixesModuleResolver(
-          modulePaths, moduleRootPaths, errorHandler, prefixReplacements);
+          modulePaths, moduleRootPaths, errorHandler, pathEscaper, prefixReplacements);
     }
   }
 
@@ -83,8 +85,9 @@ public BrowserWithTransformedPrefixesModuleResolver(
       ImmutableSet<String> modulePaths,
       ImmutableList<String> moduleRootPaths,
       ErrorHandler errorHandler,
+      PathEscaper pathEscaper,
       ImmutableMap<String, String> prefixReplacements) {
-    super(modulePaths, moduleRootPaths, errorHandler);
+    super(modulePaths, moduleRootPaths, errorHandler, pathEscaper);
     Set<PrefixReplacement> p =
         prefixReplacements
             .entrySet()

src/com/google/javascript/jscomp/deps/ModuleLoader.java

@@ -72,39 +72,59 @@ public final class ModuleLoader {
   /** Used to canonicalize paths before resolution. */
   private final PathResolver pathResolver;
 
+  private final PathEscaper pathEscaper;
+
   private final ModuleResolver moduleResolver;
 
   /**
    * Creates an instance of the module loader which can be used to locate ES6 and CommonJS modules.
    *
-   * @param inputs All inputs to the compilation process.
+   * @param moduleRoots path prefixes to strip from module paths
+   * @param inputs all inputs to the compilation process. Used to ensure that resolved paths
+   *     references an valid input.
+   * @param factory creates a module resolver, which determines how module identifiers are resolved
+   * @param pathResolver determines how to sanitize paths before resolving
+   * @param pathEscaper determines if / how paths should be escaped
    */
   public ModuleLoader(
       @Nullable ErrorHandler errorHandler,
       Iterable<String> moduleRoots,
       Iterable<? extends DependencyInfo> inputs,
       ModuleResolverFactory factory,
-      PathResolver pathResolver) {
+      PathResolver pathResolver,
+      PathEscaper pathEscaper) {
     checkNotNull(moduleRoots);
     checkNotNull(inputs);
     checkNotNull(pathResolver);
+    checkNotNull(pathEscaper);
     this.pathResolver = pathResolver;
+    this.pathEscaper = pathEscaper;
     this.errorHandler = errorHandler == null ? new NoopErrorHandler() : errorHandler;
-    this.moduleRootPaths = createRootPaths(moduleRoots, pathResolver);
+    this.moduleRootPaths = createRootPaths(moduleRoots, pathResolver, pathEscaper);
     this.modulePaths =
         resolvePaths(
             Iterables.transform(Iterables.transform(inputs, DependencyInfo::getName), pathResolver),
-            moduleRootPaths);
+            moduleRootPaths,
+            pathEscaper);
     this.moduleResolver =
-        factory.create(this.modulePaths, this.moduleRootPaths, this.errorHandler);
+        factory.create(this.modulePaths, this.moduleRootPaths, this.errorHandler, this.pathEscaper);
+  }
+
+  public ModuleLoader(
+      @Nullable ErrorHandler errorHandler,
+      Iterable<String> moduleRoots,
+      Iterable<? extends DependencyInfo> inputs,
+      ModuleResolverFactory factory,
+      PathResolver pathResolver) {
+    this(errorHandler, moduleRoots, inputs, factory, pathResolver, PathEscaper.ESCAPE);
   }
 
   public ModuleLoader(
       @Nullable ErrorHandler errorHandler,
       Iterable<String> moduleRoots,
       Iterable<? extends DependencyInfo> inputs,
       ModuleResolverFactory factory) {
-    this(errorHandler, moduleRoots, inputs, factory, PathResolver.RELATIVE);
+    this(errorHandler, moduleRoots, inputs, factory, PathResolver.RELATIVE, PathEscaper.ESCAPE);
   }
 
   @VisibleForTesting
@@ -197,14 +217,7 @@ public ModulePath resolveModuleAsPath(String moduleAddress) {
 
   /** Resolves a path into a {@link ModulePath}. */
   public ModulePath resolve(String path) {
-    return new ModulePath(
-        normalize(ModuleNames.escapePath(pathResolver.apply(path)), moduleRootPaths));
-  }
-
-  /** Resolves a path into a {@link ModulePath}. */
-  public ModulePath resolveWithoutEscapingPath(String path) {
-    return new ModulePath(
-        normalize(pathResolver.apply(path), moduleRootPaths));
+    return new ModulePath(normalize(pathEscaper.escape(pathResolver.apply(path)), moduleRootPaths));
   }
 
   /** Whether this is relative to the current file, or a top-level identifier. */
@@ -228,14 +241,14 @@ public static boolean isPathIdentifier(String name) {
   }
 
   /**
-   * @param roots List of module root paths. This path prefix will be removed from module paths when
-   *     resolved.
+   * Normalizes the given root paths, which are path prefixes to be removed from a module path when
+   * resolved.
    */
   private static ImmutableList<String> createRootPaths(
-      Iterable<String> roots, PathResolver resolver) {
+      Iterable<String> roots, PathResolver resolver, PathEscaper escaper) {
     ImmutableList.Builder<String> builder = ImmutableList.builder();
     for (String root : roots) {
-      String rootModuleName = ModuleNames.escapePath(resolver.apply(root));
+      String rootModuleName = escaper.escape(resolver.apply(root));
       if (isAmbiguousIdentifier(rootModuleName)) {
         rootModuleName = MODULE_SLASH + rootModuleName;
       }
@@ -251,11 +264,11 @@ private static ImmutableList<String> createRootPaths(
    * @return List of normalized modules which always have a leading slash
    */
   private static ImmutableSet<String> resolvePaths(
-      Iterable<String> modulePaths, Iterable<String> roots) {
+      Iterable<String> modulePaths, Iterable<String> roots, PathEscaper escaper) {
     ImmutableSet.Builder<String> resolved = ImmutableSet.builder();
     Set<String> knownPaths = new HashSet<>();
     for (String name : modulePaths) {
-      String canonicalizedPath = ModuleNames.escapePath(name);
+      String canonicalizedPath = escaper.escape(name);
       if (!knownPaths.add(normalize(canonicalizedPath, roots))) {
         // Having root paths "a" and "b" and source files "a/f.js" and "b/f.js" is ambiguous.
         throw new IllegalArgumentException(
@@ -304,6 +317,33 @@ public ErrorHandler getErrorHandler() {
     return this.errorHandler;
   }
 
+  /** Indicates whether to escape characters in paths. */
+  public enum PathEscaper {
+    /**
+     * Escapes characters in paths according to {@link ModuleNames#escapePath(String)} and then
+     * canonicalizes it.
+     */
+    ESCAPE {
+      @Override
+      public String escape(String path) {
+        return ModuleNames.escapePath(path);
+      }
+    },
+
+    /**
+     * Does not escaped characters in paths, but does canonicalize it according to {@link
+     * ModuleNames#canonicalizePath(String)}.
+     */
+    CANONICALIZE_ONLY {
+      @Override
+      public String escape(String path) {
+        return ModuleNames.canonicalizePath(path);
+      }
+    };
+
+    public abstract String escape(String path);
+  }
+
   /** An enum indicating whether to absolutize paths. */
   public enum PathResolver implements Function<String, String> {
     RELATIVE {
@@ -328,7 +368,8 @@ public interface ModuleResolverFactory {
     ModuleResolver create(
         ImmutableSet<String> modulePaths,
         ImmutableList<String> moduleRootPaths,
-        ErrorHandler errorHandler);
+        ErrorHandler errorHandler,
+        PathEscaper pathEscaper);
   }
 
   /** A trivial module loader with no roots. */

src/com/google/javascript/jscomp/deps/ModuleResolver.java

@@ -20,6 +20,7 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.javascript.jscomp.ErrorHandler;
+import com.google.javascript.jscomp.deps.ModuleLoader.PathEscaper;
 import java.util.Map;
 import javax.annotation.Nullable;
 
@@ -32,14 +33,17 @@ public abstract class ModuleResolver {
   protected final ImmutableList<String> moduleRootPaths;
 
   protected ErrorHandler errorHandler;
+  private final PathEscaper pathEscaper;
 
   public ModuleResolver(
       ImmutableSet<String> modulePaths,
       ImmutableList<String> moduleRootPaths,
-      ErrorHandler errorHandler) {
+      ErrorHandler errorHandler,
+      ModuleLoader.PathEscaper pathEscaper) {
     this.modulePaths = modulePaths;
     this.moduleRootPaths = moduleRootPaths;
     this.errorHandler = errorHandler;
+    this.pathEscaper = pathEscaper;
   }
 
   Map<String, String> getPackageJsonMainEntries() {
@@ -54,7 +58,7 @@ public String resolveModuleAsPath(String scriptAddress, String moduleAddress) {
     if (!moduleAddress.endsWith(".js")) {
       moduleAddress += ".js";
     }
-    String path = ModuleNames.escapePath(moduleAddress);
+    String path = pathEscaper.escape(moduleAddress);
     if (ModuleLoader.isRelativeIdentifier(moduleAddress)) {
       String ourPath = scriptAddress;
       int lastIndex = ourPath.lastIndexOf(ModuleLoader.MODULE_SLASH);
@@ -103,7 +107,7 @@ protected String locate(String scriptAddress, String name) {
    * relative paths to absolute references.
    */
   protected String canonicalizePath(String scriptAddress, String moduleAddress) {
-    String path = ModuleNames.escapePath(moduleAddress);
+    String path = pathEscaper.escape(moduleAddress);
     if (ModuleLoader.isRelativeIdentifier(moduleAddress)) {
       String ourPath = scriptAddress;
       int lastIndex = ourPath.lastIndexOf(ModuleLoader.MODULE_SLASH);