From 38bc30e96aac143e037ed546f75ff8034d03dfba Mon Sep 17 00:00:00 2001 From: Antonio Nuno Monteiro Date: Mon, 27 Nov 2017 14:51:10 -0800 Subject: [PATCH] Make NodeModuleResolver able to find aliased modules when server inputs are not passed in Closes https://github.com/google/closure-compiler/pull/2627 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=177075592 --- .../jscomp/RewriteJsonToModule.java | 16 ++++++--- .../javascript/jscomp/deps/ModuleLoader.java | 2 ++ .../jscomp/deps/NodeModuleResolver.java | 32 +++++++++--------- .../jscomp/RewriteJsonToModuleTest.java | 33 +++++++++++++++++-- .../jscomp/deps/ModuleLoaderTest.java | 18 +++++----- 5 files changed, 68 insertions(+), 33 deletions(-) diff --git a/src/com/google/javascript/jscomp/RewriteJsonToModule.java b/src/com/google/javascript/jscomp/RewriteJsonToModule.java index c9f3a6ec0e9..31910dc12af 100644 --- a/src/com/google/javascript/jscomp/RewriteJsonToModule.java +++ b/src/com/google/javascript/jscomp/RewriteJsonToModule.java @@ -18,7 +18,7 @@ import static com.google.common.base.Preconditions.checkState; import com.google.common.collect.ImmutableMap; -import com.google.javascript.jscomp.deps.NodeModuleResolver; +import com.google.javascript.jscomp.deps.ModuleLoader; import com.google.javascript.rhino.IR; import com.google.javascript.rhino.Node; import java.util.HashMap; @@ -176,12 +176,18 @@ private void processBrowserFieldAdvancedUsage(String dirName, Node entry) { checkState(child.isStringKey() && (value.isString() || value.isFalse())); - String val = + String path = child.getString(); + + if (path.startsWith(ModuleLoader.DEFAULT_FILENAME_PREFIX)) { + path = path.substring(ModuleLoader.DEFAULT_FILENAME_PREFIX.length()); + } + + String replacement = value.isString() ? dirName + value.getString() - : NodeModuleResolver.JSC_BROWSER_BLACKLISTED_MARKER; - // TODO: handle dots in paths (relative paths) - packageJsonMainEntries.put(dirName + child.getString(), val); + : ModuleLoader.JSC_BROWSER_BLACKLISTED_MARKER; + + packageJsonMainEntries.put(dirName + path, replacement); } } } diff --git a/src/com/google/javascript/jscomp/deps/ModuleLoader.java b/src/com/google/javascript/jscomp/deps/ModuleLoader.java index 8261eb8594e..29b7764fb64 100644 --- a/src/com/google/javascript/jscomp/deps/ModuleLoader.java +++ b/src/com/google/javascript/jscomp/deps/ModuleLoader.java @@ -51,6 +51,8 @@ public final class ModuleLoader { /** The default module root, the current directory. */ public static final String DEFAULT_FILENAME_PREFIX = "." + MODULE_SLASH; + public static final String JSC_BROWSER_BLACKLISTED_MARKER = "$jscomp$browser$blacklisted"; + public static final DiagnosticType LOAD_WARNING = DiagnosticType.error("JSC_JS_MODULE_LOAD_WARNING", "Failed to load module \"{0}\""); diff --git a/src/com/google/javascript/jscomp/deps/NodeModuleResolver.java b/src/com/google/javascript/jscomp/deps/NodeModuleResolver.java index 62321cda92b..26ce57beaf7 100644 --- a/src/com/google/javascript/jscomp/deps/NodeModuleResolver.java +++ b/src/com/google/javascript/jscomp/deps/NodeModuleResolver.java @@ -42,7 +42,6 @@ public class NodeModuleResolver extends ModuleResolver { ModuleLoader.MODULE_SLASH + "index.js", ModuleLoader.MODULE_SLASH + "index.json" }; - public static final String JSC_BROWSER_BLACKLISTED_MARKER = "$jscomp$browser$blacklisted"; /** Named modules found in node_modules folders */ private final ImmutableMap packageJsonMainEntries; @@ -166,23 +165,24 @@ public String resolveJsModule( public String resolveJsModuleFile(String scriptAddress, String moduleAddress) { for (String extension : FILE_EXTENSIONS_TO_SEARCH) { - String loadAddress = locate(scriptAddress, moduleAddress + extension); - if (loadAddress != null) { - // Also look for mappings in packageJsonMainEntries because browser field - // advanced usage allows to override / blacklist specific files, including - // the main entry. - if (packageJsonMainEntries.containsKey(loadAddress)) { - String packageJsonEntry = packageJsonMainEntries.get(loadAddress); - - if (packageJsonEntry != JSC_BROWSER_BLACKLISTED_MARKER) { - return resolveJsModuleFile(scriptAddress, packageJsonEntry); - } else { - return null; - } - } else { - return loadAddress; + String moduleAddressCandidate = moduleAddress + extension; + String canonicalizedCandidatePath = canonicalizePath(scriptAddress, moduleAddressCandidate); + + // Also look for mappings in packageJsonMainEntries because browser field + // advanced usage allows to override / blacklist specific files, including + // the main entry. + if (packageJsonMainEntries.containsKey(canonicalizedCandidatePath)) { + moduleAddressCandidate = packageJsonMainEntries.get(canonicalizedCandidatePath); + + if (ModuleLoader.JSC_BROWSER_BLACKLISTED_MARKER.equals(moduleAddressCandidate)) { + return null; } } + + String loadAddress = locate(scriptAddress, moduleAddressCandidate); + if (loadAddress != null) { + return loadAddress; + } } return null; diff --git a/test/com/google/javascript/jscomp/RewriteJsonToModuleTest.java b/test/com/google/javascript/jscomp/RewriteJsonToModuleTest.java index 8fbc908852a..f16f5b26a01 100644 --- a/test/com/google/javascript/jscomp/RewriteJsonToModuleTest.java +++ b/test/com/google/javascript/jscomp/RewriteJsonToModuleTest.java @@ -20,7 +20,6 @@ import com.google.common.collect.ImmutableList; import com.google.javascript.jscomp.deps.ModuleLoader; -import com.google.javascript.jscomp.deps.NodeModuleResolver; import com.google.javascript.rhino.Node; import java.util.Map; @@ -137,7 +136,37 @@ public void testPackageJsonFileBrowserFieldAdvancedUsage() { // NodeModuleResolver knows how to normalize this entry's value assertThat(packageJsonMainEntries).containsEntry("/override/relative.js", "/./with/this.js"); assertThat(packageJsonMainEntries) - .containsEntry("/dont/include.js", NodeModuleResolver.JSC_BROWSER_BLACKLISTED_MARKER); + .containsEntry("/dont/include.js", ModuleLoader.JSC_BROWSER_BLACKLISTED_MARKER); assertThat(packageJsonMainEntries).containsEntry("/override/explicitly.js", "/with/other.js"); } + + public void testPackageJsonBrowserFieldAdvancedUsageGH2625() { + test( + srcs( + SourceFile.fromCode( + "/package.json", + lines( + "{ 'main': 'foo/bar/baz.js',", + " 'browser': { './a/b.js': './c/d.js',", + " './server.js': 'client.js'} }"))), + expected( + lines( + "goog.provide('module$package_json')", + "var module$package_json = {", + " 'main': 'foo/bar/baz.js',", + " 'browser': {", + " './a/b.js': './c/d.js',", + " './server.js': 'client.js'", + " }", + "};"))); + + Map packageJsonMainEntries = + getLastCompiler().getModuleLoader().getPackageJsonMainEntries(); + assertThat(packageJsonMainEntries).containsExactly( + "/package.json", "/foo/bar/baz.js", + + // Test that we have normalized the key, value is normalized by NodeModuleResolver + "/a/b.js", "/./c/d.js", + "/server.js", "/client.js"); + } } diff --git a/test/com/google/javascript/jscomp/deps/ModuleLoaderTest.java b/test/com/google/javascript/jscomp/deps/ModuleLoaderTest.java index 3ec7a6ddf6d..8e05035052a 100644 --- a/test/com/google/javascript/jscomp/deps/ModuleLoaderTest.java +++ b/test/com/google/javascript/jscomp/deps/ModuleLoaderTest.java @@ -301,19 +301,15 @@ public void testLocateNodeModulesBrowserFieldAdvancedUsage() throws Exception { "/node_modules/mymodule/server.js", "/node_modules/mymodule/client.js", "/node_modules/mymodule/override/relative.js", "/node_modules/mymodule/./with/this.js", "/node_modules/mymodule/exclude/this.js", - NodeModuleResolver.JSC_BROWSER_BLACKLISTED_MARKER, + ModuleLoader.JSC_BROWSER_BLACKLISTED_MARKER, "/node_modules/mymodule/replace/other.js", - "/node_modules/mymodule/with/alternative.js"); + "/node_modules/mymodule/with/alternative.js"); ImmutableList compilerInputs = inputs( - "node_modules/mymodule/package.json", - "node_modules/mymodule/server.js", - "node_modules/mymodule/client.js", - "node_modules/mymodule/exclude/this.js", - "node_modules/mymodule/replace/other.js", - "node_modules/mymodule/with/alternative.js", - "/node_modules/mymodule/override/relative.js", + "/node_modules/mymodule/package.json", + "/node_modules/mymodule/client.js", + "/node_modules/mymodule/with/alternative.js", "/node_modules/mymodule/with/this.js", "/foo.js"); @@ -328,7 +324,6 @@ public void testLocateNodeModulesBrowserFieldAdvancedUsage() throws Exception { assertUri( "/node_modules/mymodule/client.js", loader.resolve("/foo.js").resolveJsModule("mymodule")); - assertUri( "/node_modules/mymodule/with/alternative.js", loader.resolve("/foo.js").resolveJsModule("mymodule/replace/other.js")); @@ -338,6 +333,9 @@ public void testLocateNodeModulesBrowserFieldAdvancedUsage() throws Exception { assertUri( "/node_modules/mymodule/with/this.js", loader.resolve("/foo.js").resolveJsModule("mymodule/override/relative.js")); + assertUri( + "/node_modules/mymodule/with/this.js", + loader.resolve("/node_modules/mymodule/client.js").resolveJsModule("./override/relative.js")); assertNull( loader.resolve("/node_modules/mymodule/client.js").resolveJsModule("./exclude/this.js")); }