fix: overriding version of "pnpm" repo (#2883)

The default `pnpm` repo was always taking priority and could not be
overridden. See #2349 (comment)

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases
- New test cases added

Author: jbedard

e2e/bzlmod/BUILD.bazel

@@ -26,6 +26,49 @@ js_test(
     entry_point = "main.mjs",
 )
 
+# Capture the versions of the pnpm binaries resolved by the pnpm module extension
+# so :pnpm_versions_test can assert on them.
+js_run_binary(
+    name = "pnpm_version",
+    args = ["--version"],
+    stdout = "pnpm_version.txt",
+    tool = "@pnpm",
+)
+
+js_run_binary(
+    name = "other_pnpm_version",
+    args = ["--version"],
+    stdout = "other_pnpm_version.txt",
+    tool = "@other_pnpm//:pnpm",
+)
+
+# Run npm via the resolved @pnpm to prove it was bundled (include_npm). This only
+# succeeds if npm is on PATH, which requires the version-less include_npm tag to be
+# honored even though @pnpm's version comes from a different tag. A package.json must
+# be present for `pnpm exec` to run.
+js_run_binary(
+    name = "pnpm_npm_version",
+    srcs = ["package.json"],
+    args = [
+        "exec",
+        "npm",
+        "--version",
+    ],
+    stdout = "pnpm_npm_version.txt",
+    tool = "@pnpm",
+)
+
+js_test(
+    name = "pnpm_versions_test",
+    data = [
+        "other_pnpm_version.txt",
+        "package.json",
+        "pnpm_npm_version.txt",
+        "pnpm_version.txt",
+    ],
+    entry_point = "pnpm_versions.test.mjs",
+)
+
 lessc(
     name = "styles",
     css = "my.less",

e2e/bzlmod/MODULE.bazel

@@ -55,6 +55,14 @@ pnpm.pnpm(
     name = "pnpm",
     pnpm_version_from = "//:package.json",
 )
+
+# A second, version-less tag for the same repo that only asks to bundle npm. The
+# version is selected by the tag above; include_npm must still be honored even
+# though it comes from a different (version-less) tag.
+pnpm.pnpm(
+    name = "pnpm",
+    include_npm = True,
+)
 pnpm.pnpm(
     name = "other_pnpm",
     pnpm_version = "9.15.9",

e2e/bzlmod/other_module/MODULE.bazel

@@ -11,11 +11,11 @@ npm.npm_translate_lock(
 )
 use_repo(npm, "npm_other_module")
 
-pnpm = use_extension(
-    "@aspect_rules_js//npm:extensions.bzl",
-    "pnpm",
-    dev_dependency = True,
-)
+# 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
+# root module uses pnpm_version_from, which must win over this registration; this
+# is asserted by //:pnpm_versions_test in the e2e test root module.
+pnpm = use_extension("@aspect_rules_js//npm:extensions.bzl", "pnpm")
 pnpm.pnpm(
     name = "pnpm",
     pnpm_version = "9.15.9",

e2e/bzlmod/pnpm_versions.test.mjs

@@ -0,0 +1,37 @@
+import { readFileSync } from 'fs'
+
+// The version of the @pnpm repo is resolved from `pnpm_version_from = "//:package.json"`
+// on the root module tag, and must match the package.json `packageManager` field.
+// It takes priority over both the version requested by the non-root module
+// `other_module` and the default version registered by rules_js itself.
+// See https://github.com/aspect-build/rules_js/pull/2349#discussion_r3362093839
+const packageManager = JSON.parse(
+    readFileSync('package.json', 'utf8')
+).packageManager
+const expected = packageManager.split('@')[1].split('+')[0]
+const pnpmVersion = readFileSync('pnpm_version.txt', 'utf8').trim()
+
+if (pnpmVersion !== expected) {
+    throw new Error(
+        `Incorrect @pnpm version: got ${pnpmVersion}, expected ${expected} from package.json#packageManager`
+    )
+}
+
+// The root module may register additional pnpm repos with their own pinned version.
+const otherPnpmVersion = readFileSync('other_pnpm_version.txt', 'utf8').trim()
+
+if (otherPnpmVersion !== '9.15.9') {
+    throw new Error(
+        `Incorrect @other_pnpm version: got ${otherPnpmVersion}, expected 9.15.9`
+    )
+}
+
+// @pnpm was configured with include_npm via a separate, version-less tag, so the
+// bundled npm must be runnable. `pnpm exec npm --version` (captured above) only
+// produces a version string if npm is on PATH; otherwise the include_npm request
+// was dropped because the selected version came from a different tag.
+const npmVersion = readFileSync('pnpm_npm_version.txt', 'utf8').trim()
+
+if (!/^\d+\.\d+\.\d+/.test(npmVersion)) {
+    throw new Error(`Expected a bundled npm version, got: ${npmVersion}`)
+}

npm/extensions.bzl

@@ -393,8 +393,10 @@ pnpm = module_extension(
                     default = False,
                 ),
                 "pnpm_version": attr.string(
-                    doc = "pnpm version to use. The string `latest` will be resolved to LATEST_PNPM_VERSION.",
-                    default = DEFAULT_PNPM_VERSION,
+                    doc = """pnpm version to use. The string `latest` will be resolved to LATEST_PNPM_VERSION.
+
+                    If unset, a default version chosen by rules_js is used, but only if no other
+                    module requests an explicit version for this repository.""",
                 ),
                 "pnpm_version_from": attr.label(
                     doc = """Label to a package.json file to read the pnpm version from.

npm/private/pnpm_extension.bzl

@@ -1,6 +1,5 @@
 """pnpm extension logic (the extension itself is in npm/extensions.bzl)."""
 
-load("@bazel_lib//lib:lists.bzl", "unique")
 load(":pnpm_repository.bzl", "DEFAULT_PNPM_VERSION", "LATEST_PNPM_VERSION")
 load(":utils.bzl", "utils")
 load(":versions.bzl", "PNPM_VERSIONS")
@@ -27,30 +26,47 @@ def resolve_pnpm_repositories(mctx):
 
     # Collect all the module tags and associated versions/integrities/options
     integrity = {}
-    registrations = {}
+
+    # Whether npm should be bundled with pnpm, OR'd across all tags for a repo
+    # (name -> bool). Like patches, this is a per-repo setting, not per-version.
+    include_npm_by_repo = {}
 
     # Patches and patch_args are per-repo-name (NOT per-version) and only
     # accepted from the root module. Multiple tags for the same repo name
     # concatenate their patches and the last patch_args wins.
     patches_by_repo = {}
     patch_args_by_repo = {}
+
+    # Versions explicitly requested for each repo: name -> version -> is_root. Tags
+    # that carry no version (the default, e.g. the always-present registration from
+    # rules_js itself) are excluded, so they only matter as a fallback. A version
+    # requested by the root module (is_root = True) takes priority over the rest.
+    requested = {}
     for mod in mctx.modules:
         for attr in mod.tags.pnpm:
             if attr.name != DEFAULT_PNPM_REPO_NAME and not mod.is_root:
                 fail("""\
                 Only the root module may override the default name for the pnpm repository.
                 This prevents conflicting registrations in the global namespace of external repos.
                 """)
-            if not registrations.get(attr.name, False):
-                registrations[attr.name] = {}
+            if attr.name not in requested:
+                requested[attr.name] = {}
+                include_npm_by_repo[attr.name] = False
+            include_npm_by_repo[attr.name] = include_npm_by_repo[attr.name] or attr.include_npm
             if mod.is_root and (getattr(attr, "patches", None) or getattr(attr, "patch_args", None)):
                 patches_by_repo.setdefault(attr.name, [])
                 patches_by_repo[attr.name].extend(getattr(attr, "patches", []) or [])
                 patch_args_by_repo[attr.name] = list(attr.patch_args)
 
-            if attr.pnpm_version_from and attr.pnpm_version and attr.pnpm_version != DEFAULT_PNPM_VERSION:
+            if attr.pnpm_version_from and attr.pnpm_version:
                 fail("Cannot specify both pnpm_version = {} and pnpm_version_from = {}".format(attr.pnpm_version, attr.pnpm_version_from))
 
+            # A tag that specifies no version (neither pnpm_version nor pnpm_version_from)
+            # carries the default version, e.g. the registration from rules_js itself.
+            # Such registrations must not influence version selection when any module
+            # explicitly requests a version.
+            is_default_registration = attr.pnpm_version == "" and attr.pnpm_version_from == None
+
             # Handle pnpm_version_from by reading package.json
             if attr.pnpm_version_from != None:
                 # Read package.json and extract packageManager field
@@ -77,17 +93,15 @@ def resolve_pnpm_repositories(mctx):
 
             elif attr.pnpm_version == "latest":
                 v = LATEST_PNPM_VERSION
+            elif attr.pnpm_version == "":
+                v = DEFAULT_PNPM_VERSION
             else:
                 v = attr.pnpm_version
 
-            # Avoid inserting the default version from a non-root module
-            # (likely rules_js itself) if the root module already has a version.
-            if mod.is_root or len(registrations[attr.name]) == 0:
-                if v not in registrations[attr.name]:
-                    registrations[attr.name][v] = []
-                registrations[attr.name][v].append(attr.include_npm)
+            if not is_default_registration:
+                requested[attr.name][v] = requested[attr.name].get(v, False) or mod.is_root
             if attr.pnpm_version_integrity:
-                integrity[attr.pnpm_version] = attr.pnpm_version_integrity
+                integrity[v] = attr.pnpm_version_integrity
 
             # If no integrity was provided or found via package.json load from known versions
             if not integrity.get(v, False) and PNPM_VERSIONS.get(v, False):
@@ -96,11 +110,15 @@ def resolve_pnpm_repositories(mctx):
     # From the collected registrations, resolve to a single version per repository name
     notes = []
     repositories = {}
-    for name, versions_map in registrations.items():
-        # Disregard repeated version numbers and convert {version:include_npm} to version[]
-        versions = unique(versions_map.keys())
-
-        # Use "Minimal Version Selection" like bzlmod does for resolving module conflicts
+    for name, version_requests in requested.items():
+        # Versions requested by the root module take priority, then versions requested
+        # by any other module, falling back to the default version when no module
+        # requested a specific version.
+        root_versions = [v for v in version_requests if version_requests[v]]
+        versions = root_versions or version_requests.keys() or [DEFAULT_PNPM_VERSION]
+
+        # Within the selected tier, pick the highest version like bzlmod does. This
+        # runs after the tiering above, so a root request wins even if it is lower.
         # Note, the 'sorted(list)' function in starlark doesn't allow us to provide a custom comparator
         if len(versions) > 1:
             selected = versions[0]
@@ -116,7 +134,7 @@ def resolve_pnpm_repositories(mctx):
 
         selected = {
             "version": selected,
-            "include_npm": 0 < len([i for i in versions_map[selected] if i]),
+            "include_npm": include_npm_by_repo[name],
             "integrity": integrity.get(selected, None),
             "patches": patches_by_repo.get(name, []),
             "patch_args": patch_args_by_repo.get(name, ["-p1"]),

npm/private/test/pnpm_test.bzl

@@ -6,7 +6,9 @@ load("//npm/private:pnpm_extension.bzl", "DEFAULT_PNPM_REPO_NAME", "resolve_pnpm
 load("//npm/private:pnpm_repository.bzl", "DEFAULT_PNPM_VERSION", "LATEST_PNPM_VERSION")
 load("//npm/private:versions.bzl", "PNPM_VERSIONS")
 
-def _fake_pnpm_tag(version = None, name = DEFAULT_PNPM_REPO_NAME, integrity = None, pnpm_version_from = None, include_npm = False, patches = [], patch_args = ["-p1"]):
+# NB: version defaults to the empty sentinel, mirroring the real tag attribute, so a
+# bare _fake_pnpm_tag() is equivalent to the default registration from rules_js itself.
+def _fake_pnpm_tag(version = "", name = DEFAULT_PNPM_REPO_NAME, integrity = None, pnpm_version_from = None, include_npm = False, patches = [], patch_args = ["-p1"]):
     return struct(
         name = name,
         pnpm_version = version,
@@ -129,6 +131,21 @@ def _include_npm(ctx):
         ],
     )
 
+def _include_npm_other_version_wins(ctx):
+    # include_npm is a per-repo setting, not per-version: a version-less tag asking to
+    # bundle npm must be honored even when another module's explicit version is the one
+    # selected. Otherwise the npm request would be silently dropped.
+    return _resolve_test(
+        ctx,
+        repositories = {"pnpm": {"version": "9.1.0", "integrity": "sha512-Z/WHmRapKT5c8FnCOFPVcb6vT3U8cH9AyyK+1fsVeMaq07bEEHzLO6CzW+AD62IaFkcayDbIe+tT+dVLtGEnJA==", "include_npm": True, "patches": [], "patch_args": ["-p1"]}},
+        modules = [
+            # Root brings the repo into scope and wants npm, but expresses no version.
+            _fake_mod(True, _fake_pnpm_tag(include_npm = True)),
+            # A dependency pins the version that ends up selected.
+            _fake_mod(False, _fake_pnpm_tag(version = "9.1.0")),
+        ],
+    )
+
 def _custom_name(ctx):
     return _resolve_test(
         ctx,
@@ -148,6 +165,85 @@ def _custom_name(ctx):
         ],
     )
 
+def _dep_version_with_default_registration(ctx):
+    # A non-root module explicitly requests a pnpm version while another non-root
+    # module (rules_js itself, which always registers `pnpm.pnpm(name = "pnpm")`)
+    # carries the default version.
+    #
+    # The rules_js registration comes first in BFS order ("aspect_rules_js" sorts
+    # before nearly all module names) and must not mask the explicit request,
+    # even when the explicit request is for a lower version than the default.
+    # See https://github.com/aspect-build/rules_js/pull/2349#discussion_r3362093839
+    return _resolve_test(
+        ctx,
+        repositories = {"pnpm": {"version": "9.1.0", "integrity": "sha512-Z/WHmRapKT5c8FnCOFPVcb6vT3U8cH9AyyK+1fsVeMaq07bEEHzLO6CzW+AD62IaFkcayDbIe+tT+dVLtGEnJA==", "include_npm": False, "patches": [], "patch_args": ["-p1"]}},
+        modules = [
+            _fake_mod(True),
+            # rules_js itself: a bare pnpm.pnpm(name = "pnpm") tag carrying the default version
+            _fake_mod(False, _fake_pnpm_tag()),
+            _fake_mod(False, _fake_pnpm_tag(version = "9.1.0")),
+        ],
+    )
+
+def _dep_versions_mvs(ctx):
+    # Multiple non-root modules explicitly request different pnpm versions:
+    # the highest wins, with a note. The default registration from rules_js
+    # does not participate in the selection.
+    return _resolve_test(
+        ctx,
+        repositories = {"pnpm": {"version": "9.2.0", "integrity": "sha512-mKgP0RwucJZ0d2IwQQZDKz3cZ9z1S1qMAck/aKLNXgXmghhJUioG+3YoTUGiZg1eM08u47vykYO/LnObHa+ncQ==", "include_npm": False, "patches": [], "patch_args": ["-p1"]}},
+        notes = ["""NOTE: repo 'pnpm' has multiple versions ["9.1.0", "9.2.0"]; selected 9.2.0"""],
+        modules = [
+            _fake_mod(True),
+            _fake_mod(False, _fake_pnpm_tag()),
+            _fake_mod(False, _fake_pnpm_tag(version = "9.1.0")),
+            _fake_mod(False, _fake_pnpm_tag(version = "9.2.0")),
+        ],
+    )
+
+def _root_default_dep_version(ctx):
+    # The root module registers a bare tag (no version preference, e.g. just to
+    # bring the repo into scope). A dep's explicit version wins over the default
+    # version carried by the root's bare tag.
+    return _resolve_test(
+        ctx,
+        repositories = {"pnpm": {"version": "9.1.0", "integrity": "sha512-Z/WHmRapKT5c8FnCOFPVcb6vT3U8cH9AyyK+1fsVeMaq07bEEHzLO6CzW+AD62IaFkcayDbIe+tT+dVLtGEnJA==", "include_npm": False, "patches": [], "patch_args": ["-p1"]}},
+        modules = [
+            _fake_mod(True, _fake_pnpm_tag()),
+            _fake_mod(False, _fake_pnpm_tag(version = "9.1.0")),
+        ],
+    )
+
+def _root_explicit_default_beats_dep(ctx):
+    # Explicitly requesting the default version is distinct from not requesting a
+    # version at all: it is a real request from the root module and therefore wins
+    # over a (higher) version requested by a non-root module.
+    # See https://github.com/aspect-build/rules_js/pull/2883#discussion_r3367024654
+    return _resolve_test(
+        ctx,
+        repositories = {"pnpm": {"version": DEFAULT_PNPM_VERSION, "integrity": PNPM_VERSIONS[DEFAULT_PNPM_VERSION], "include_npm": False, "patches": [], "patch_args": ["-p1"]}},
+        modules = [
+            _fake_mod(True, _fake_pnpm_tag(version = DEFAULT_PNPM_VERSION)),
+            _fake_mod(False, _fake_pnpm_tag(version = "11.0.9")),
+        ],
+    )
+
+def _root_lower_than_default(ctx):
+    # The realistic override case: the root module pins the default "pnpm" repo to
+    # a version *lower* than DEFAULT_PNPM_VERSION, while rules_js itself carries the
+    # default registration. The root's explicit (lower) pin must win; a naive
+    # "Minimal Version Selection across all registrations" would wrongly keep the
+    # higher default and silently ignore the user's pin.
+    return _resolve_test(
+        ctx,
+        repositories = {"pnpm": {"version": "9.1.0", "integrity": "sha512-Z/WHmRapKT5c8FnCOFPVcb6vT3U8cH9AyyK+1fsVeMaq07bEEHzLO6CzW+AD62IaFkcayDbIe+tT+dVLtGEnJA==", "include_npm": False, "patches": [], "patch_args": ["-p1"]}},
+        modules = [
+            _fake_mod(True, _fake_pnpm_tag(version = "9.1.0")),
+            # rules_js itself: a bare pnpm.pnpm(name = "pnpm") tag carrying DEFAULT_PNPM_VERSION
+            _fake_mod(False, _fake_pnpm_tag()),
+        ],
+    )
+
 def _integrity_conflict(ctx):
     # What happens if two modules define the same version with conflicting integrity parameters.
     # Currently we print nothing to indicate this, we trust whichever integrity wins.
@@ -228,9 +324,15 @@ def _os_cpu_constraints(ctx):
 
 basic_test = unittest.make(_basic)
 override_test = unittest.make(_override)
+dep_version_with_default_registration_test = unittest.make(_dep_version_with_default_registration)
+dep_versions_mvs_test = unittest.make(_dep_versions_mvs)
+root_default_dep_version_test = unittest.make(_root_default_dep_version)
+root_explicit_default_beats_dep_test = unittest.make(_root_explicit_default_beats_dep)
+root_lower_than_default_test = unittest.make(_root_lower_than_default)
 latest_test = unittest.make(_latest)
 custom_name_test = unittest.make(_custom_name)
 include_npm_test = unittest.make(_include_npm)
+include_npm_other_version_wins_test = unittest.make(_include_npm_other_version_wins)
 integrity_conflict_test = unittest.make(_integrity_conflict)
 from_package_json_simple_test = unittest.make(_from_package_json_simple)
 from_package_json_with_hash_test = unittest.make(_from_package_json_with_hash)
@@ -245,9 +347,15 @@ def pnpm_tests(name):
         name,
         basic_test,
         override_test,
+        dep_version_with_default_registration_test,
+        dep_versions_mvs_test,
+        root_default_dep_version_test,
+        root_explicit_default_beats_dep_test,
+        root_lower_than_default_test,
         latest_test,
         custom_name_test,
         include_npm_test,
+        include_npm_other_version_wins_test,
         integrity_conflict_test,
         from_package_json_simple_test,
         from_package_json_with_hash_test,