fix: exports_directories_only causes node to resolve from runfiles/node_modules (#3380)

Author: thesayyn

internal/node/launcher.sh

@@ -337,7 +337,7 @@ else
   # Entry point is the user-supplied script
   MAIN="${PWD}/"TEMPLATED_entry_point_execroot_path
   # TODO: after we link-all-bins we should not need this extra lookup
-  if [[ ! -f "$MAIN" ]]; then
+  if [[ ! -e "$MAIN" ]]; then
     if [ "$FROM_EXECROOT" = true ]; then
       MAIN="$EXECROOT/"TEMPLATED_entry_point_execroot_path
     else

internal/node/node.bzl

@@ -139,12 +139,21 @@ def _to_manifest_path(ctx, file):
         return ctx.workspace_name + "/" + file.short_path
 
 def _to_execroot_path(ctx, file):
+    # Check for special case of external/<npm>/node_modules or
+    # bazel-out/<platform>/bin/external/<npm>/node_modules.
+    # TODO: This assumes we are linking this to the root node_modules which is not always the case
+    # since the linker was updated to support linking to sub-directories since this special case was
+    # added
     parts = file.path.split("/")
-    if parts[0] == "external":
-        if parts[2] == "node_modules":
-            # external/npm/node_modules -> node_modules/foo
-            # the linker will make sure we can resolve node_modules from npm
-            return "/".join(parts[2:])
+    if file.is_directory and not file.is_source:
+        # Strip the output root to handle the case of a TreeArtifact exported by js_library with
+        # exports directories only. Since a TreeArtifact is generated by an action, it resides
+        # inside bazel-out directory.
+        parts = parts[3:]
+    if len(parts) > 3 and parts[0] == "external" and parts[2] == "node_modules":
+        # Transform external/npm/node_modules/foo to node_modules/foo.
+        # The linker will make sure we can resolve node_modules from npm.
+        return "/".join(parts[2:])
 
     return file.path
 

internal/node/test/instanceof_test_execroot/BUILD.bazel

@@ -0,0 +1,19 @@
+load("@npm//pkg_with_bin:index.bzl", "instanceof_execroot_test")
+load(
+    "@npm_directory_artifacts//pkg_with_bin:index.bzl",
+    instanceof_execroot_test_exports_directories = "instanceof_execroot_test",
+)
+
+instanceof_execroot_test(
+    name = "test",
+    data = [
+        "//internal/node/test/instanceof_test_execroot/lib",
+    ],
+)
+
+instanceof_execroot_test_exports_directories(
+    name = "test_exports_directories_only",
+    data = [
+        "//internal/node/test/instanceof_test_execroot/lib",
+    ],
+)

internal/node/test/instanceof_test_execroot/index.bzl

@@ -0,0 +1,16 @@
+load("//:index.bzl", "js_library")
+
+package(default_visibility = ["//internal/node/test/instanceof_test_execroot:__subpackages__"])
+
+js_library(
+    name = "lib",
+    # required by tools/npm_packages/pkg_with_bin as third-party dep
+    package_name = "instanceof_test_execroot_lib",
+    srcs = [
+        "index.js",
+        "package.json",
+    ],
+    deps = [
+        "@npm//node_resolve_main",
+    ],
+)

internal/node/test/instanceof_test_execroot/lib/BUILD.bazel

@@ -0,0 +1,20 @@
+// This file will fail when one dependency is resolved from two distinct node_modules
+// mostly likely to be one inside runfiles and other from bazel-out
+// See: https://github.com/bazelbuild/rules_nodejs/pull/3380 for more.
+const assert = require("assert");
+
+const resolved_by_lib = require.resolve("node_resolve_main");
+const resolved_by_pkg_with_bin = globalThis["node_resolve_main_resolved_path_by_pkg_with_bin"]
+assert.equal(
+    resolved_by_lib, 
+    resolved_by_pkg_with_bin,
+    `
+Expected to resolve package "node_resolve_main" from the same node_modules but
+
+tools/npm_packages/pkg_with_bin resolved it from ${resolved_by_pkg_with_bin}
+
+internal/node/test/instanceof_test_execroot/lib resolved it from ${resolved_by_lib}
+
+See: https://github.com/bazelbuild/rules_nodejs/pull/3380 for context.
+`
+);

internal/node/test/instanceof_test_execroot/lib/index.js

@@ -0,0 +1 @@
+{"name": "instanceof_test_execroot_lib"}

internal/node/test/instanceof_test_execroot/lib/package.json

@@ -42,6 +42,8 @@ def npm_deps():
             "//:tools/npm_packages/node_resolve_nested_main/nested/package.json",
             "//:tools/npm_packages/testy/index.js",
             "//:tools/npm_packages/testy/package.json",
+            "//:tools/npm_packages/pkg_with_bin/main.js",
+            "//:tools/npm_packages/pkg_with_bin/package.json",
         ],
         environment = {
             "SOME_USER_ENV": "yarn is great!",
@@ -107,6 +109,8 @@ js_library(
             "//:tools/npm_packages/node_resolve_nested_main/nested/package.json",
             "//:tools/npm_packages/testy/index.js",
             "//:tools/npm_packages/testy/package.json",
+            "//:tools/npm_packages/pkg_with_bin/main.js",
+            "//:tools/npm_packages/pkg_with_bin/package.json",
         ],
         environment = {
             "SOME_USER_ENV": "yarn is great!",

npm_deps.bzl

@@ -77,6 +77,7 @@
         "node_resolve_main": "file:./tools/npm_packages/node_resolve_main",
         "node_resolve_main_2": "file:./tools/npm_packages/node_resolve_main_2",
         "node_resolve_nested_main": "file:./tools/npm_packages/node_resolve_nested_main",
+        "pkg_with_bin": "file:./tools/npm_packages/pkg_with_bin",
         "npm": "6.13.7",
         "patch-package": "^6.2.2",
         "protobufjs": "6.8.8",

package.json

@@ -0,0 +1,3 @@
+globalThis["node_resolve_main_resolved_path_by_pkg_with_bin"] = require.resolve("node_resolve_main");
+require("instanceof_test_execroot_lib");
+// See: internal/node/test/instanceof_test_execroot