feat: support multiple node_modules roots in require_patch

Refs: #266

WORKSPACE

@@ -506,6 +506,21 @@ filegroup(
     yarn_lock = "//:tools/fine_grained_goldens/yarn.lock",
 )
 
+yarn_install(
+    name = "multi_root_deps_yarn",
+    package_json = "//:tools/multi_root_deps_yarn/package.json",
+    symlink_node_modules = False,
+    yarn_lock = "//:tools/multi_root_deps_yarn/yarn.lock",
+)
+
+npm_install(
+    name = "multi_root_deps_npm",
+    npm_command = "install",
+    package_json = "//:tools/multi_root_deps_npm/package.json",
+    package_lock_json = "//:tools/multi_root_deps_npm/package-lock.json",
+    symlink_node_modules = False,
+)
+
 #
 # RBE configuration
 #

internal/node/node.bzl

@@ -172,8 +172,9 @@ def _nodejs_binary_impl(ctx):
 
     node_modules_roots = _compute_node_modules_roots(ctx)
 
-    if "" in node_modules_roots:
-        node_modules_root = node_modules_roots[""] + "/node_modules"
+    node_modules_roots_key = node_modules_roots.keys()
+    if len(node_modules_roots_key) > 0:
+        node_modules_root = ":".join([root + "/node_modules" for root in node_modules_roots])
     else:
         # there are no fine grained deps but we still need a node_modules_root even if it is a non-existant one
         node_modules_root = "build_bazel_rules_nodejs/node_modules"

internal/node/require_patch.js

@@ -39,6 +39,10 @@ function log_verbose(...m) {
   if (VERBOSE_LOGS) console.error(`[${path.basename(__filename)}]`, ...m);
 }
 
+function pretty_json(m) {
+  return JSON.stringify(m, undefined, 2);
+}
+
 /**
  * The module roots as pairs of a RegExp to match the require path, and a
  * module_root to substitute for the require path.
@@ -54,14 +58,23 @@ const BIN_DIR = 'TEMPLATED_bin_dir';
 const GEN_DIR = 'TEMPLATED_gen_dir';
 const TARGET = 'TEMPLATED_target';
 
+/**
+ * The node_modules roots as a list for importing modules from multiple roots.
+ *
+ * There is currently no support for duplicate modules across roots. Avoid
+ * including identical packages from multiple roots, such as 
+ * ['@npm_a//jest', '@npm_b//jest']
+ */
+const NODE_MODULES_ROOTS = NODE_MODULES_ROOT.split(":")
+
 log_verbose(`patching require for ${TARGET}
   cwd: ${process.cwd()}
   RUNFILES: ${process.env.RUNFILES}
   TARGET: ${TARGET}
   BIN_DIR: ${BIN_DIR}
   GEN_DIR: ${GEN_DIR}
-  MODULE_ROOTS: ${JSON.stringify(MODULE_ROOTS, undefined, 2)}
-  NODE_MODULES_ROOT: ${NODE_MODULES_ROOT}
+  MODULE_ROOTS: ${pretty_json(MODULE_ROOTS)}
+  NODE_MODULES_ROOTS: ${pretty_json(NODE_MODULES_ROOT)}
   USER_WORKSPACE_NAME: ${USER_WORKSPACE_NAME}
 `);
 
@@ -456,16 +469,18 @@ module.constructor._resolveFilename =
   }
 
   // If import was not resolved above then attempt to resolve
-  // within the node_modules filegroup in use
-  try {
-    const resolved = originalResolveFilename(
-        resolveRunfiles(undefined, NODE_MODULES_ROOT, request), parent, isMain, options);
-    log_verbose(
-        `resolved ${request} within node_modules (${NODE_MODULES_ROOT}) to ` +
-        `${resolved} from ${parentFilename}`);
-    return resolved;
-  } catch (e) {
-    failedResolutions.push(`node_modules attribute (${NODE_MODULES_ROOT}) - ${e.toString()}`);
+  // within the node_modules filegroups in use
+  for (const nodeModulesRoot of NODE_MODULES_ROOTS) {
+    try {
+      const resolved = originalResolveFilename(
+          resolveRunfiles(undefined, nodeModulesRoot, request), parent, isMain, options);
+      log_verbose(
+          `resolved ${request} within node_modules (${nodeModulesRoot}) to ` +
+          `${resolved} from ${parentFilename}`);
+      return resolved;
+    } catch (e) {
+      failedResolutions.push(`node_modules attribute (${nodeModulesRoot}) - ${e.toString()}`);
+    }
   }
 
   // Print the same error message that vanilla nodejs does.

internal/node/test/BUILD.bazel

@@ -90,7 +90,7 @@ nodejs_test(
 )
 
 nodejs_test(
-    name = "mode_resolution_test",
+    name = "module_resolution_test",
     data = [
         "//internal/node/test/lib1",
         "//internal/node/test/lib2",
@@ -209,7 +209,7 @@ write_file(
 )
 
 nodejs_test(
-    name = "mode_resolution_built_test",
+    name = "module_resolution_built_test",
     data = [
         "//internal/node/test/lib1",
         "//internal/node/test/lib2",
@@ -226,6 +226,17 @@ nodejs_test(
     templated_args = ["--bazel_patch_module_resolver"],
 )
 
+nodejs_test(
+    name = "multi_module_resolution_built_test",
+    data = [
+        "@multi_root_deps_npm//rxjs",
+        "@multi_root_deps_yarn//rxjs",
+    ],
+    entry_point = ":multi_module_resolution.spec.js",
+    # TODO: passes locally without this flag but fails on CircleCI
+    templated_args = ["--bazel_patch_module_resolver"],
+)
+
 nodejs_test(
     name = "data_resolution_built_test",
     data = [

internal/node/test/multi_module_resolution.spec.js

@@ -0,0 +1,7 @@
+// test node npm resolution with multiple roots
+try {
+  require('rxjs');
+} catch (err) {
+  console.error('should resolve to one of the rxjs modules by default');
+  process.exitCode = 1;
+}

tools/fine_grained_deps_yarn/package.json

@@ -1,21 +1,6 @@
 {
-  "description": "runtime dependencies fine_grained_deps e2e test",
-  "devDependencies": {
-    "jasmine": "3.3.0",
-    "jasmine-core": "3.3.0",
-    "typescript": "3.0.3"
-  },
+  "description": "runtime dependencies multi_root_deps e2e test",
   "dependencies": {
-    "@gregmagolan/test-a": "0.0.4",
-    "@gregmagolan/test-b": "0.0.2",
-    "ajv": "5.5.2",
-    "chokidar": "2.0.4",
-    "http-server": "github:alexeagle/http-server#97205e945b69091606ed83aa0c8489e9ce65d282",
-    "klaw": "1.3.1",
-    "local-module": "link:../../tools/npm_packages/local_module/yarn",
     "rxjs": "6.5.0"
-  },
-  "scripts": {
-    "postinstall": "node ../../internal/npm_install/test/postinstall.js"
   }
 }

tools/multi_root_deps_npm/package.json

@@ -0,0 +1,6 @@
+{
+  "description": "runtime dependencies multi_root_deps e2e test",
+  "dependencies": {
+    "rxjs": "6.5.0"
+  }
+}

tools/multi_root_deps_yarn/package.json

@@ -0,0 +1,6 @@
+{
+  "description": "runtime dependencies multi_root_deps e2e test",
+  "dependencies": {
+    "rxjs": "6.5.0"
+  }
+}

tools/multi_root_deps_yarn/yarn.lock

@@ -0,0 +1,15 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
+# yarn lockfile v1
+
+
+[email protected]:
+  version "6.5.0"
+  resolved "https://registry.yarnpkg.com/rxjs/-/rxjs-6.5.0.tgz#1e4bc933f14a0c5c2ce7bb71ddd4244d42f4b04b"
+  integrity sha512-FOTfP3LK5VN3dDtr+wjFAeKVe5nTPPTC2+NUFJ8kyuO+YbIl/aME0eQDiX2MCVgnhKyuUYaEjgZEx8iL/4AV6A==
+  dependencies:
+    tslib "^1.9.0"
+
+tslib@^1.9.0:
+  version "1.14.1"
+  resolved "https://registry.yarnpkg.com/tslib/-/tslib-1.14.1.tgz#cf2d38bdc34a134bcaf1091c41f6619e2f672d00"
+  integrity sha512-Xni35NKzjgMrwevysHTCArtLDpPvye8zV/0E4EyYn43P7/7qvQwPh9BGkHewbMulVntbigmcT7rdX3BNo9wRJg==