fix: multiple versions of a package using aliases in pnpm workspaces …

…crashes

Fix #1110

.aspect/rules/external_repository_action_cache/npm_translate_lock_LTE4Nzc1MDcwNjU=

@@ -2,9 +2,9 @@
 # Input hashes for repository rule npm_translate_lock(name = "npm", pnpm_lock = "//:pnpm-lock.yaml").
 # This file should be checked into version control along with the pnpm-lock.yaml file.
 .npmrc=-2065072158
-pnpm-lock.yaml=-2123305961
+pnpm-lock.yaml=-1324340320
 examples/npm_deps/patches/[email protected]=-442666336
-package.json=-1057425123
+package.json=965846805
 pnpm-workspace.yaml=1536402859
 examples/js_binary/package.json=-41174383
 examples/macro/package.json=857146175
@@ -17,8 +17,8 @@ js/private/coverage/bundle/package.json=-1543718929
 js/private/image/package.json=-266007263
 js/private/test/image/package.json=1295393035
 js/private/worker/src/package.json=-715289696
-npm/private/test/package.json=1920219401
+npm/private/test/package.json=1424344949
 npm/private/test/vendored/lodash-4.17.21.tgz=-1206623349
-npm/private/test/npm_package/package.json=-1377103079
+npm/private/test/npm_package/package.json=-1991705133
 npm/private/test/vendored/is-odd/package.json=1041695223
 npm/private/test/vendored/semver-max/package.json=578664053

e2e/pnpm_workspace_deps/.aspect/rules/external_repository_action_cache/npm_translate_lock_LTE4Nzc1MDcwNjU=

@@ -1,7 +1,9 @@
+# @generated
 # Input hashes for repository rule npm_translate_lock(name = "npm", pnpm_lock = "//:pnpm-lock.yaml").
 # This file should be checked into version control along with the pnpm-lock.yaml file.
-pnpm-lock.yaml=-1808949182
+pnpm-lock.yaml=-1199344695
 package.json=959251505
-pnpm-workspace.yaml=1047324817
+pnpm-workspace.yaml=-2026278039
 lib/package.json=-275126300
-tests/package.json=-641014960
+lib-dupes/package.json=-725174835
+tests/package.json=-1421585247

e2e/pnpm_workspace_deps/.bazelignore

@@ -1,3 +1,4 @@
 node_modules/
 lib/node_modules
+lib-dupes/node_modules
 tests/node_modules

e2e/pnpm_workspace_deps/MODULE.bazel

@@ -17,6 +17,7 @@ npm.npm_translate_lock(
         "//:package.json",
         "//:pnpm-workspace.yaml",
         "//lib:package.json",
+        "//lib-dupes:package.json",
         "//tests:package.json",
     ],
     pnpm_lock = "//:pnpm-lock.yaml",

e2e/pnpm_workspace_deps/WORKSPACE

@@ -22,6 +22,7 @@ npm_translate_lock(
         "//:package.json",
         "//:pnpm-workspace.yaml",
         "//lib:package.json",
+        "//lib-dupes:package.json",
         "//tests:package.json",
     ],
     pnpm_lock = "//:pnpm-lock.yaml",

e2e/pnpm_workspace_deps/lib-dupes/BUILD.bazel

@@ -0,0 +1,27 @@
+load("@aspect_rules_js//npm:defs.bzl", "npm_package")
+load("@aspect_rules_js//js:defs.bzl", "js_library")
+load("@npm//:defs.bzl", "npm_link_all_packages")
+
+npm_link_all_packages(name = "node_modules")
+
+# A library with 2 aliases to one simple dependency (package with no dependencies)
+js_library(
+    name = "js_lib_dupes",
+    srcs = ["index.js"],
+    data = [
+        # Two versions of the same package (via aliases), see https://github.com/aspect-build/rules_js/issues/1110
+        ":node_modules/@aspect-test/c",
+        ":node_modules/@aspect-test/c1",
+    ],
+    visibility = ["//visibility:public"],
+)
+
+# Exposed via npm_package()
+npm_package(
+    name = "lib-dupes",
+    srcs = [
+        "package.json",
+        ":js_lib_dupes",
+    ],
+    visibility = ["//visibility:public"],
+)

e2e/pnpm_workspace_deps/lib-dupes/index.js

@@ -0,0 +1,5 @@
+// Two versions of the same package referenced throught aliases, see https://github.com/aspect-build/rules_js/issues/1110
+module.exports = {
+    importDep: () => require('@aspect-test/c'),
+    importDupeDep: () => require('@aspect-test/c1'),
+}

e2e/pnpm_workspace_deps/lib-dupes/package.json

@@ -0,0 +1,8 @@
+{
+    "name": "@lib/test-dupes",
+    "private": true,
+    "dependencies": {
+        "@aspect-test/c": "2.0.0",
+        "@aspect-test/c1": "npm:@aspect-test/[email protected]"
+    }
+}

e2e/pnpm_workspace_deps/pnpm-workspace.yaml

@@ -1,3 +1,4 @@
 packages:
     - lib
+    - lib-dupes
     - tests

e2e/pnpm_workspace_deps/tests/BUILD.bazel

@@ -20,3 +20,11 @@ js_test(
     ],
     entry_point = "test_pkg_deps_linked.js",
 )
+
+js_test(
+    name = "dupes",
+    data = [
+        ":node_modules/@lib/test-dupes",
+    ],
+    entry_point = "test_pkg_dupe_deps_linked.js",
+)

e2e/pnpm_workspace_deps/tests/package.json

@@ -1,6 +1,7 @@
 {
     "name": "tests",
     "dependencies": {
-        "@lib/test": "workspace:*"
+        "@lib/test": "workspace:*",
+        "@lib/test-dupes": "workspace:*"
     }
 }

e2e/pnpm_workspace_deps/tests/test_pkg_dupe_deps_linked.js

@@ -0,0 +1,7 @@
+const testLibDupes = require('@lib/test-dupes')
+
+console.log('loaded dependency', testLibDupes.importDep().id())
+console.log(
+    'loaded duplicate aliased dependency',
+    testLibDupes.importDupeDep().id()
+)

npm/private/npm_package_store.bzl

@@ -178,7 +178,7 @@ def _impl(ctx):
     transitive_files = []
     direct_ref_deps = {}
 
-    npm_package_store_deps = ctx.attr.src[NpmPackageInfo].npm_package_store_deps.to_list() if ctx.attr.src else []
+    npm_package_store_deps = []
 
     if ctx.attr.src:
         # output the package as a TreeArtifact to its virtual store location
@@ -206,14 +206,7 @@ def _impl(ctx):
                 verbose = ctx.attr.verbose,
             )
 
-        for store in npm_package_store_deps:
-            dep_package = store.package
-            dep_virtual_store_directory = store.virtual_store_directory
-
-            # "node_modules/{virtual_store_root}/{virtual_store_name}/node_modules/{package}"
-            dep_symlink_path = paths.join("node_modules", utils.virtual_store_root, virtual_store_name, "node_modules", dep_package)
-            transitive_files.extend(utils.make_symlink(ctx, dep_symlink_path, dep_virtual_store_directory))
-
+        linked_virtual_store_directories = []
         for dep, _dep_aliases in ctx.attr.deps.items():
             # symlink the package's direct deps to its virtual store location
             if dep[NpmPackageStoreInfo].root_package != ctx.label.package:
@@ -224,16 +217,29 @@ deps of npm_package_store must be in the same package.""" % (ctx.label.package,
             dep_aliases = _dep_aliases.split(",") if _dep_aliases else [dep_package]
             dep_virtual_store_directory = dep[NpmPackageStoreInfo].virtual_store_directory
             if dep_virtual_store_directory:
+                linked_virtual_store_directories.append(dep_virtual_store_directory)
                 for dep_alias in dep_aliases:
                     # "node_modules/{virtual_store_root}/{virtual_store_name}/node_modules/{package}"
                     dep_symlink_path = paths.join("node_modules", utils.virtual_store_root, virtual_store_name, "node_modules", dep_alias)
                     transitive_files.extend(utils.make_symlink(ctx, dep_symlink_path, dep_virtual_store_directory))
             else:
                 # this is a ref npm_link_package, a downstream terminal npm_link_package
-                # for this npm depedency will create the dep symlinks for this dep;
+                # for this npm dependency will create the dep symlinks for this dep;
                 # this pattern is used to break circular dependencies between 3rd
                 # party npm deps; it is not recommended for 1st party deps
                 direct_ref_deps[dep] = dep_aliases
+
+        for store in ctx.attr.src[NpmPackageInfo].npm_package_store_deps.to_list():
+            dep_package = store.package
+            dep_virtual_store_directory = store.virtual_store_directory
+
+            # only link npm package store deps from NpmPackageInfo if they have _not_ already been linked directly
+            # from deps; fixes https://github.com/aspect-build/rules_js/issues/1110.
+            if dep_virtual_store_directory not in linked_virtual_store_directories:
+                # "node_modules/{virtual_store_root}/{virtual_store_name}/node_modules/{package}"
+                dep_symlink_path = paths.join("node_modules", utils.virtual_store_root, virtual_store_name, "node_modules", dep_package)
+                transitive_files.extend(utils.make_symlink(ctx, dep_symlink_path, dep_virtual_store_directory))
+                npm_package_store_deps.append(store)
     else:
         # if ctx.attr.src is _not_ set and ctx.attr.deps is, this is a terminal
         # package with deps being the transitive closure of deps;

npm/private/test/BUILD.bazel

@@ -62,6 +62,7 @@ build_test(
         ":node_modules/json-stable-stringify",
         ":node_modules/lodash",  # lodash is brought in as a vendored .tgz file: vendored/lodash-4.17.21.tgz
         ":node_modules/node-gyp",
+        ":node_modules/test-npm_package",
         ":node_modules/plotly.js",
         ":node_modules/protoc-gen-grpc",
         ":node_modules/regl",

npm/private/test/npm_package/BUILD.bazel

@@ -13,19 +13,22 @@ js_library(
         ":package.json",
     ],
     deps = [
-        # node_modules deps should be excluded from the contents of the npm_package
+        # 1. node_modules deps should be excluded from the contents of the npm_package
+        # 2. include 2 versions of chalk to test cases such as https://github.com/aspect-build/rules_js/issues/1110
         ":node_modules/chalk",
+        "//:node_modules/chalk",
     ],
 )
 
 npm_package(
-    name = "pkg_a",
+    name = "npm_package",
     srcs = [":lib_a"],
     include_runfiles = False,
+    visibility = ["//visibility:public"],
 )
 
 copy_to_directory(
-    name = "expected_pkg_a",
+    name = "expected_npm_package",
     srcs = [
         "index.js",
         ":package.json",
@@ -34,6 +37,6 @@ copy_to_directory(
 
 diff_test(
     name = "test",
-    file1 = ":pkg_a",
-    file2 = ":expected_pkg_a",
+    file1 = ":npm_package",
+    file2 = ":expected_npm_package",
 )

npm/private/test/npm_package/index.js

@@ -1,2 +1,5 @@
+// Import two versions (via aliases) of the same package, see https://github.com/aspect-build/rules_js/issues/1110
 const chalk = require('chalk')
-console.log(chalk.italic.green('42'))
+const chalkAlt = require('chalk-alt')
+
+console.log(chalk.italic.green('Answer:'), chalkAlt.bold.red('42'))

npm/private/test/npm_package/package.json

@@ -1,6 +1,8 @@
 {
+    "name": "test-npm_package",
     "private": true,
     "dependencies": {
-        "chalk": "5.0.1"
+        "chalk": "5.0.1",
+        "chalk-alt": "npm:[email protected]"
     }
 }

npm/private/test/package.json

@@ -23,6 +23,7 @@
         "syncpack": "git+https://github.com/JamieMason/syncpack.git#c245af8ea73ce3345d92bbda6c684092a841e262",
         "typescript": "*",
         "unused": "latest",
+        "test-npm_package": "workspace:*",
         "webpack-bundle-analyzer": "4.5.0"
     }
 }

npm/private/test/repositories_checked.bzl

@@ -6308,6 +6308,25 @@ def npm_repositories():
         },
     )
 
+    npm_import(
+        name = "npm__chalk__5.1.1",
+        root_package = "",
+        link_workspace = "",
+        link_packages = {
+            "": ["chalk"],
+            "npm/private/test/npm_package": ["chalk-alt"],
+        },
+        package = "chalk",
+        version = "5.1.1",
+        url = "https://registry.npmjs.org/chalk/-/chalk-5.1.1.tgz",
+        npm_translate_lock_repo = "npm",
+        generate_bzl_library_targets = True,
+        integrity = "sha512-OItMegkSDU3P7OJRWBbNRsQsL8SzgwlIGXSZRVfHCLBYrDgzYDuozwDMwvEDpiZdjr50tdOTbTzuubirtEozsg==",
+        transitive_closure = {
+            "chalk": ["5.1.1"],
+        },
+    )
+
     npm_import(
         name = "npm__charenc__0.0.2",
         root_package = "",

package.json

@@ -2,6 +2,7 @@
     "private": true,
     "devDependencies": {
         "@types/node": "16.18.11",
+        "chalk": "5.1.1",
         "inline-fixtures": "1.1.0",
         "jsonpath-plus": "7.2.0",
         "typescript": "4.9.5"