diff --git a/.aspect/rules/external_repository_action_cache/npm_translate_lock_LTE4Nzc1MDcwNjU= b/.aspect/rules/external_repository_action_cache/npm_translate_lock_LTE4Nzc1MDcwNjU= index 734bec640..190a6a18d 100755 --- a/.aspect/rules/external_repository_action_cache/npm_translate_lock_LTE4Nzc1MDcwNjU= +++ b/.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/meaning-of-life@1.0.0-pnpm.patch=-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 diff --git a/e2e/pnpm_workspace_deps/.aspect/rules/external_repository_action_cache/npm_translate_lock_LTE4Nzc1MDcwNjU= b/e2e/pnpm_workspace_deps/.aspect/rules/external_repository_action_cache/npm_translate_lock_LTE4Nzc1MDcwNjU= index 09937d840..905668881 100755 --- a/e2e/pnpm_workspace_deps/.aspect/rules/external_repository_action_cache/npm_translate_lock_LTE4Nzc1MDcwNjU= +++ b/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 diff --git a/e2e/pnpm_workspace_deps/.bazelignore b/e2e/pnpm_workspace_deps/.bazelignore index e45690809..79fa086c2 100644 --- a/e2e/pnpm_workspace_deps/.bazelignore +++ b/e2e/pnpm_workspace_deps/.bazelignore @@ -1,3 +1,4 @@ node_modules/ lib/node_modules +lib-dupes/node_modules tests/node_modules \ No newline at end of file diff --git a/e2e/pnpm_workspace_deps/MODULE.bazel b/e2e/pnpm_workspace_deps/MODULE.bazel index 1a6c4c971..264ddc99d 100644 --- a/e2e/pnpm_workspace_deps/MODULE.bazel +++ b/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", diff --git a/e2e/pnpm_workspace_deps/WORKSPACE b/e2e/pnpm_workspace_deps/WORKSPACE index 20fe0afb2..e43c4a5b9 100644 --- a/e2e/pnpm_workspace_deps/WORKSPACE +++ b/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", diff --git a/e2e/pnpm_workspace_deps/lib-dupes/BUILD.bazel b/e2e/pnpm_workspace_deps/lib-dupes/BUILD.bazel new file mode 100644 index 000000000..882665afd --- /dev/null +++ b/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"], +) diff --git a/e2e/pnpm_workspace_deps/lib-dupes/index.js b/e2e/pnpm_workspace_deps/lib-dupes/index.js new file mode 100644 index 000000000..b2a0c2a03 --- /dev/null +++ b/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'), +} diff --git a/e2e/pnpm_workspace_deps/lib-dupes/package.json b/e2e/pnpm_workspace_deps/lib-dupes/package.json new file mode 100644 index 000000000..dbfd9304e --- /dev/null +++ b/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/c@1.0.0" + } +} diff --git a/e2e/pnpm_workspace_deps/pnpm-lock.yaml b/e2e/pnpm_workspace_deps/pnpm-lock.yaml index 3eeda505f..6b1577d6d 100644 --- a/e2e/pnpm_workspace_deps/pnpm-lock.yaml +++ b/e2e/pnpm_workspace_deps/pnpm-lock.yaml @@ -18,14 +18,38 @@ importers: specifier: 1.0.0 version: 1.0.0 + lib-dupes: + dependencies: + '@aspect-test/c': + specifier: 2.0.0 + version: 2.0.0 + '@aspect-test/c1': + specifier: npm:@aspect-test/c@1.0.0 + version: /@aspect-test/c@1.0.0 + tests: dependencies: '@lib/test': specifier: workspace:* version: link:../lib + '@lib/test-dupes': + specifier: workspace:* + version: link:../lib-dupes packages: + /@aspect-test/c@1.0.0: + resolution: {integrity: sha512-UorLD4TFr9CWFeYbUd5etaxSo201fYEFR+rSxXytfzefX41EWCBabsXhdhvXjK6v/HRuo1y1I1NiW2P3/bKJeA==} + hasBin: true + requiresBuild: true + dev: false + + /@aspect-test/c@2.0.0: + resolution: {integrity: sha512-vRuHi/8zxZ+IRGdgdX4VoMNFZrR9UqO87yQx61IGIkjgV7QcKUeu5jfvIE3Mr0WNQeMdO1JpyTx1UUpsE73iug==} + hasBin: true + requiresBuild: true + dev: false + /@aspect-test/e@1.0.0: resolution: {integrity: sha512-GyAxHYKN650db+xnimHnL2LPz65ilmQsVhCasWA7drDNQn/rfmPiEVMzjRiS7m46scXIERaBmiJMzYDf0bIUbA==} hasBin: true diff --git a/e2e/pnpm_workspace_deps/pnpm-workspace.yaml b/e2e/pnpm_workspace_deps/pnpm-workspace.yaml index d7b2c95aa..482b6ae28 100644 --- a/e2e/pnpm_workspace_deps/pnpm-workspace.yaml +++ b/e2e/pnpm_workspace_deps/pnpm-workspace.yaml @@ -1,3 +1,4 @@ packages: - lib + - lib-dupes - tests diff --git a/e2e/pnpm_workspace_deps/tests/BUILD.bazel b/e2e/pnpm_workspace_deps/tests/BUILD.bazel index a545b5ef1..b4f22176f 100644 --- a/e2e/pnpm_workspace_deps/tests/BUILD.bazel +++ b/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", +) diff --git a/e2e/pnpm_workspace_deps/tests/package.json b/e2e/pnpm_workspace_deps/tests/package.json index b729c6057..b9c0c03cd 100644 --- a/e2e/pnpm_workspace_deps/tests/package.json +++ b/e2e/pnpm_workspace_deps/tests/package.json @@ -1,6 +1,7 @@ { "name": "tests", "dependencies": { - "@lib/test": "workspace:*" + "@lib/test": "workspace:*", + "@lib/test-dupes": "workspace:*" } } diff --git a/e2e/pnpm_workspace_deps/tests/test_pkg_dupe_deps_linked.js b/e2e/pnpm_workspace_deps/tests/test_pkg_dupe_deps_linked.js new file mode 100644 index 000000000..bd3873a82 --- /dev/null +++ b/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() +) diff --git a/npm/private/npm_package_store.bzl b/npm/private/npm_package_store.bzl index d4bc42775..785201c01 100644 --- a/npm/private/npm_package_store.bzl +++ b/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; diff --git a/npm/private/test/BUILD.bazel b/npm/private/test/BUILD.bazel index 50c679625..263ea81d5 100644 --- a/npm/private/test/BUILD.bazel +++ b/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", diff --git a/npm/private/test/npm_package/BUILD.bazel b/npm/private/test/npm_package/BUILD.bazel index 86481201b..d6663901b 100644 --- a/npm/private/test/npm_package/BUILD.bazel +++ b/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", ) diff --git a/npm/private/test/npm_package/index.js b/npm/private/test/npm_package/index.js index 4f6a8655b..ebc924939 100644 --- a/npm/private/test/npm_package/index.js +++ b/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')) diff --git a/npm/private/test/npm_package/package.json b/npm/private/test/npm_package/package.json index b14f7db0b..229de8f34 100644 --- a/npm/private/test/npm_package/package.json +++ b/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:chalk@5.1.1" } } diff --git a/npm/private/test/package.json b/npm/private/test/package.json index 5c8b773c7..33dc01a61 100644 --- a/npm/private/test/package.json +++ b/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" } } diff --git a/npm/private/test/repositories_checked.bzl b/npm/private/test/repositories_checked.bzl index 90eb9c62c..9c8f62ace 100644 --- a/npm/private/test/repositories_checked.bzl +++ b/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 = "", diff --git a/package.json b/package.json index f6ae75023..b1f7cf110 100644 --- a/package.json +++ b/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" diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 69ce104a8..a696a9d2b 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -23,6 +23,9 @@ importers: '@types/node': specifier: 16.18.11 version: registry.npmjs.org/@types/node@16.18.11 + chalk: + specifier: 5.1.1 + version: 5.1.1 inline-fixtures: specifier: 1.1.0 version: 1.1.0 @@ -303,6 +306,9 @@ importers: syncpack: specifier: git+https://github.com/JamieMason/syncpack.git#c245af8ea73ce3345d92bbda6c684092a841e262 version: github.com/JamieMason/syncpack/c245af8ea73ce3345d92bbda6c684092a841e262 + test-npm_package: + specifier: workspace:* + version: link:npm_package typescript: specifier: '*' version: 4.9.5 @@ -318,6 +324,9 @@ importers: chalk: specifier: 5.0.1 version: 5.0.1 + chalk-alt: + specifier: npm:chalk@5.1.1 + version: /chalk@5.1.1 packages: @@ -1995,6 +2004,10 @@ packages: engines: {node: ^12.17.0 || ^14.13 || >=16.0.0} dev: false + /chalk@5.1.1: + resolution: {integrity: sha512-OItMegkSDU3P7OJRWBbNRsQsL8SzgwlIGXSZRVfHCLBYrDgzYDuozwDMwvEDpiZdjr50tdOTbTzuubirtEozsg==} + engines: {node: ^12.17.0 || ^14.13 || >=16.0.0} + /charenc@0.0.2: resolution: {integrity: sha512-yrLQ/yVUFXkzg7EDQsPieE/53+0RlaWTs+wBrvW36cyilJ2SaDWfl4Yj7MtLTXleV9uEKefbAGUPv2/iWSooRA==} dev: true