fix(npm): correct package_store_prefix_len for links in external-repo sub-packages

e2e/bzlmod/BUILD.bazel

@@ -134,6 +134,9 @@ build_test(
 
         # js_run_binary() running a binary in @other_repo
         ":run_other_module_binary_test",
+
+        # js_run_binary() running a binary linked in a sub-package of @other_repo
+        ":run_other_module_subpackage_binary_test",
     ],
 )
 
@@ -148,3 +151,14 @@ js_run_binary(
     silent_on_success = False,
     tool = "@other_module//:lessc",  # A js_binary() tool in a different repo
 )
+
+js_run_binary(
+    name = "run_other_module_subpackage_binary_test",
+    srcs = ["my.less"],
+    outs = ["other-module-subpackage-my.css"],
+    args = [
+        "my.less",
+        "other-module-subpackage-my.css",
+    ],
+    tool = "@other_module//frontend:lessc",  # A js_binary() tool linked in a sub-package of a different repo
+)

e2e/bzlmod/other_module/.bazelignore

@@ -1 +1,2 @@
 node_modules
+frontend/node_modules

e2e/bzlmod/other_module/MODULE.bazel

@@ -9,7 +9,13 @@ npm.npm_translate_lock(
     pnpm_lock = "//:pnpm-lock.yaml",
     verify_node_modules_ignored = "//:.bazelignore",
 )
-use_repo(npm, "npm_other_module")
+npm.npm_translate_lock(
+    name = "npm_other_module_frontend",
+    npmrc = "//:.npmrc",
+    pnpm_lock = "//frontend:pnpm-lock.yaml",
+    verify_node_modules_ignored = "//:.bazelignore",
+)
+use_repo(npm, "npm_other_module", "npm_other_module_frontend")
 
 # A non-root module may request a pnpm version for the default "pnpm" repo, but
 # the version requested by the e2e test root module takes priority. The e2e test

e2e/bzlmod/other_module/frontend/BUILD.bazel

@@ -0,0 +1,11 @@
+load("@npm_other_module_frontend//:defs.bzl", "npm_link_all_packages")
+load("@npm_other_module_frontend//frontend:less/package_json.bzl", less_bin = "bin")
+
+# npm packages linked in a sub-package of an external module, see
+# https://github.com/aspect-build/rules_js/pull/2896
+npm_link_all_packages()
+
+less_bin.lessc_binary(
+    name = "lessc",
+    visibility = ["//visibility:public"],
+)

e2e/bzlmod/other_module/frontend/package.json

@@ -0,0 +1,8 @@
+{
+    "pnpm": {
+        "onlyBuiltDependencies": []
+    },
+    "dependencies": {
+        "less": "4.1.3"
+    }
+}

npm/private/npm_package_store.bzl

@@ -12,8 +12,6 @@ load(":utils.bzl", "utils")
 
 _SUPPORTS_SYMLINK_TARGET_TYPE = bazel_features.rules.symlink_action_has_target_type
 
-_PACKAGE_STORE_PREFIX_LEN = len("node_modules/{}/".format(utils.package_store_root))
-
 _EXTRACT_EXECUTION_REQUIREMENTS = {
     "supports-path-mapping": "1",
 }
@@ -191,11 +189,8 @@ def _npm_package_store_impl(ctx):
     if not version:
         fail("No package version specified to link to. Package version must either be specified explicitly via 'version' attribute or come from the 'src' 'JsInfo|NpmPackageInfo', typically a 'js_library|npm_package' target")
 
-    package_store_prefix_len = _PACKAGE_STORE_PREFIX_LEN
-    if ctx.label.package:
-        package_store_prefix_len += len(ctx.label.package)
-    if ctx.label.repo_name:
-        package_store_prefix_len += len(ctx.label.repo_name) + 3  # +3 for ../
+    package_store_prefix = utils.package_store_prefix(ctx.label.repo_name, ctx.label.package)
+    package_store_prefix_len = len(package_store_prefix)
 
     package_key = "{}@{}".format(package, version)
     package_store_name = utils.package_store_name(package_key)
@@ -225,14 +220,7 @@ def _npm_package_store_impl(ctx):
         npm_pkg_info = ctx.attr.src[NpmPackageInfo]
 
         # output the package as a TreeArtifact to its package store location
-        if ctx.label.repo_name and ctx.label.package:
-            expected_short_path = "../{}/{}/{}".format(ctx.label.repo_name, ctx.label.package, package_store_directory_path)
-        elif ctx.label.repo_name:
-            expected_short_path = "../{}/{}".format(ctx.label.repo_name, package_store_directory_path)
-        elif ctx.label.package:
-            expected_short_path = "{}/{}".format(ctx.label.package, package_store_directory_path)
-        else:
-            expected_short_path = package_store_directory_path
+        expected_short_path = "{}{}/node_modules/{}".format(package_store_prefix, package_store_name, package)
 
         src = npm_pkg_info.src
         if src.short_path == expected_short_path:

npm/private/test/utils_tests.bzl

@@ -219,13 +219,29 @@ def test_hex_to_base64(ctx):
         )
     return unittest.end(env)
 
+# buildifier: disable=function-docstring
+def test_package_store_prefix(ctx):
+    env = unittest.begin(ctx)
+
+    asserts.equals(env, "node_modules/.aspect_rules_js/", utils.package_store_prefix("", ""))
+    asserts.equals(env, "frontend/node_modules/.aspect_rules_js/", utils.package_store_prefix("", "frontend"))
+    asserts.equals(env, "../other_module+/node_modules/.aspect_rules_js/", utils.package_store_prefix("other_module+", ""))
+    asserts.equals(env, "../other_module+/frontend/node_modules/.aspect_rules_js/", utils.package_store_prefix("other_module+", "frontend"))
+
+    # the prefix is stripped from a dependency's package store short_path to form the relative symlink target
+    short_path = "../other_module+/frontend/node_modules/.aspect_rules_js/less@4.1.3/node_modules/less"
+    asserts.equals(env, "less@4.1.3/node_modules/less", short_path[len(utils.package_store_prefix("other_module+", "frontend")):])
+
+    return unittest.end(env)
+
 t2_test = unittest.make(test_package_store_and_target_name)
 t3_test = unittest.make(test_friendly_name)
 t6_test = unittest.make(test_parse_package_name)
 t7_test = unittest.make(test_npm_registry_download_url)
 t8_test = unittest.make(test_npm_registry_url)
 t9_test = unittest.make(test_package_store_name_link_versions)
 t10_test = unittest.make(test_hex_to_base64)
+t11_test = unittest.make(test_package_store_prefix)
 
 def utils_tests(name):
     unittest.suite(
@@ -237,4 +253,5 @@ def utils_tests(name):
         t8_test,
         t9_test,
         t10_test,
+        t11_test,
     )

npm/private/utils.bzl

@@ -382,16 +382,29 @@ def _hex_to_base64(hex_string):
 
     return "".join(output)
 
+_PACKAGE_STORE_ROOT = ".aspect_rules_js"
+_PACKAGE_STORE_PREFIX = "node_modules/{}/".format(_PACKAGE_STORE_ROOT)
+
+def _package_store_prefix(repo_name, package):
+    """Path of the package store of a repository package, relative to the main repository."""
+    prefix = _PACKAGE_STORE_PREFIX
+    if package:
+        prefix = "{}/{}".format(package, prefix)
+    if repo_name:
+        prefix = "../{}/{}".format(repo_name, prefix)
+    return prefix
+
 utils = struct(
     sorted_map = _sorted_map,
     friendly_name = _friendly_name,
     link_to_importer = _link_to_importer,
     importer_to_link = _importer_to_link,
     package_repo_name = _package_repo_name,
     package_store_name = _package_store_name,
+    package_store_prefix = _package_store_prefix,
     make_directory_symlink = _make_directory_symlink,
     # Symlinked node_modules structure package store path under node_modules
-    package_store_root = ".aspect_rules_js",
+    package_store_root = _PACKAGE_STORE_ROOT,
     # Suffix for npm_import links repository
     links_repo_suffix = "__links",
     # Output group name for the package directory of a linked npm package