Skip to content

Commit

Permalink
Make NodeModuleResolver able to find aliased modules when server inpu…
Browse files Browse the repository at this point in the history 
…ts are not passed in

Closes #2627

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=177075592
  • Loading branch information
@anmonteiro @brad4d
anmonteiro authored and brad4d committed Nov 28, 2017
1 parent 5ec8dfa commit 38bc30e
Show file tree
Hide file tree
Showing 5 changed files with 68 additions and 33 deletions.
16 changes: 11 additions & 5 deletions src/com/google/javascript/jscomp/RewriteJsonToModule.java
View file
Expand Up @@ -18,7 +18,7 @@
import static com.google.common.base.Preconditions.checkState; import static com.google.common.base.Preconditions.checkState;


import com.google.common.collect.ImmutableMap; 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.IR;
import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Node;
import java.util.HashMap; import java.util.HashMap;
Expand Down Expand Up @@ -176,12 +176,18 @@ private void processBrowserFieldAdvancedUsage(String dirName, Node entry) {


checkState(child.isStringKey() && (value.isString() || value.isFalse())); 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() value.isString()
? dirName + value.getString() ? dirName + value.getString()
: NodeModuleResolver.JSC_BROWSER_BLACKLISTED_MARKER; : ModuleLoader.JSC_BROWSER_BLACKLISTED_MARKER;
// TODO: handle dots in paths (relative paths)
packageJsonMainEntries.put(dirName + child.getString(), val); packageJsonMainEntries.put(dirName + path, replacement);
} }
} }
} }
2 changes: 2 additions & 0 deletions src/com/google/javascript/jscomp/deps/ModuleLoader.java
View file
Expand Up @@ -51,6 +51,8 @@ public final class ModuleLoader {
/** The default module root, the current directory. */ /** The default module root, the current directory. */
public static final String DEFAULT_FILENAME_PREFIX = "." + MODULE_SLASH; 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 = public static final DiagnosticType LOAD_WARNING =
DiagnosticType.error("JSC_JS_MODULE_LOAD_WARNING", "Failed to load module \"{0}\""); DiagnosticType.error("JSC_JS_MODULE_LOAD_WARNING", "Failed to load module \"{0}\"");


Expand Down
32 changes: 16 additions & 16 deletions src/com/google/javascript/jscomp/deps/NodeModuleResolver.java
View file
Expand Up @@ -42,7 +42,6 @@ public class NodeModuleResolver extends ModuleResolver {
ModuleLoader.MODULE_SLASH + "index.js", ModuleLoader.MODULE_SLASH + "index.js",
ModuleLoader.MODULE_SLASH + "index.json" ModuleLoader.MODULE_SLASH + "index.json"
}; };
public static final String JSC_BROWSER_BLACKLISTED_MARKER = "$jscomp$browser$blacklisted";


/** Named modules found in node_modules folders */ /** Named modules found in node_modules folders */
private final ImmutableMap<String, String> packageJsonMainEntries; private final ImmutableMap<String, String> packageJsonMainEntries;
Expand Down Expand Up @@ -166,23 +165,24 @@ public String resolveJsModule(


public String resolveJsModuleFile(String scriptAddress, String moduleAddress) { public String resolveJsModuleFile(String scriptAddress, String moduleAddress) {
for (String extension : FILE_EXTENSIONS_TO_SEARCH) { for (String extension : FILE_EXTENSIONS_TO_SEARCH) {
String loadAddress = locate(scriptAddress, moduleAddress + extension); String moduleAddressCandidate = moduleAddress + extension;
if (loadAddress != null) { String canonicalizedCandidatePath = canonicalizePath(scriptAddress, moduleAddressCandidate);
// Also look for mappings in packageJsonMainEntries because browser field
// advanced usage allows to override / blacklist specific files, including // Also look for mappings in packageJsonMainEntries because browser field
// the main entry. // advanced usage allows to override / blacklist specific files, including
if (packageJsonMainEntries.containsKey(loadAddress)) { // the main entry.
String packageJsonEntry = packageJsonMainEntries.get(loadAddress); if (packageJsonMainEntries.containsKey(canonicalizedCandidatePath)) {

moduleAddressCandidate = packageJsonMainEntries.get(canonicalizedCandidatePath);
if (packageJsonEntry != JSC_BROWSER_BLACKLISTED_MARKER) {
return resolveJsModuleFile(scriptAddress, packageJsonEntry); if (ModuleLoader.JSC_BROWSER_BLACKLISTED_MARKER.equals(moduleAddressCandidate)) {
} else { return null;
return null;
}
} else {
return loadAddress;
} }
} }

String loadAddress = locate(scriptAddress, moduleAddressCandidate);
if (loadAddress != null) {
return loadAddress;
}
} }


return null; return null;
Expand Down
33 changes: 31 additions & 2 deletions test/com/google/javascript/jscomp/RewriteJsonToModuleTest.java
View file
Expand Up @@ -20,7 +20,6 @@


import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableList;
import com.google.javascript.jscomp.deps.ModuleLoader; import com.google.javascript.jscomp.deps.ModuleLoader;
import com.google.javascript.jscomp.deps.NodeModuleResolver;
import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Node;
import java.util.Map; import java.util.Map;


Expand Down Expand Up @@ -137,7 +136,37 @@ public void testPackageJsonFileBrowserFieldAdvancedUsage() {
// NodeModuleResolver knows how to normalize this entry's value // NodeModuleResolver knows how to normalize this entry's value
assertThat(packageJsonMainEntries).containsEntry("/override/relative.js", "/./with/this.js"); assertThat(packageJsonMainEntries).containsEntry("/override/relative.js", "/./with/this.js");
assertThat(packageJsonMainEntries) 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"); 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<String, String> 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");
}
} }
18 changes: 8 additions & 10 deletions test/com/google/javascript/jscomp/deps/ModuleLoaderTest.java
View file
Expand Up @@ -301,19 +301,15 @@ public void testLocateNodeModulesBrowserFieldAdvancedUsage() throws Exception {
"/node_modules/mymodule/server.js", "/node_modules/mymodule/client.js", "/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/override/relative.js", "/node_modules/mymodule/./with/this.js",
"/node_modules/mymodule/exclude/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/replace/other.js",
"/node_modules/mymodule/with/alternative.js"); "/node_modules/mymodule/with/alternative.js");


ImmutableList<CompilerInput> compilerInputs = ImmutableList<CompilerInput> compilerInputs =
inputs( inputs(
"node_modules/mymodule/package.json", "/node_modules/mymodule/package.json",
"node_modules/mymodule/server.js", "/node_modules/mymodule/client.js",
"node_modules/mymodule/client.js", "/node_modules/mymodule/with/alternative.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/with/this.js", "/node_modules/mymodule/with/this.js",
"/foo.js"); "/foo.js");


Expand All @@ -328,7 +324,6 @@ public void testLocateNodeModulesBrowserFieldAdvancedUsage() throws Exception {


assertUri( assertUri(
"/node_modules/mymodule/client.js", loader.resolve("/foo.js").resolveJsModule("mymodule")); "/node_modules/mymodule/client.js", loader.resolve("/foo.js").resolveJsModule("mymodule"));

assertUri( assertUri(
"/node_modules/mymodule/with/alternative.js", "/node_modules/mymodule/with/alternative.js",
loader.resolve("/foo.js").resolveJsModule("mymodule/replace/other.js")); loader.resolve("/foo.js").resolveJsModule("mymodule/replace/other.js"));
Expand All @@ -338,6 +333,9 @@ public void testLocateNodeModulesBrowserFieldAdvancedUsage() throws Exception {
assertUri( assertUri(
"/node_modules/mymodule/with/this.js", "/node_modules/mymodule/with/this.js",
loader.resolve("/foo.js").resolveJsModule("mymodule/override/relative.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( assertNull(
loader.resolve("/node_modules/mymodule/client.js").resolveJsModule("./exclude/this.js")); loader.resolve("/node_modules/mymodule/client.js").resolveJsModule("./exclude/this.js"));
} }
Expand Down

0 comments on commit 38bc30e

Please sign in to comment.