feat: support multiple node_modules roots in require_patch

Refs: bazel-contrib#266

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 = [
+        "@fine_grained_deps_npm//rxjs",
+        "@fine_grained_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;
+}