From 61dc823a39e6682eb92c104c94eef98d0cebb816 Mon Sep 17 00:00:00 2001 From: Peter Goldberg Date: Wed, 28 Oct 2026 12:48:58 +0000 Subject: [PATCH 1/3] fix: include transitive deps in transitive_files for packages with lifecycle hooks --- npm/private/npm_package_store.bzl | 52 +++++++------------------------ 1 file changed, 12 insertions(+), 40 deletions(-) diff --git a/npm/private/npm_package_store.bzl b/npm/private/npm_package_store.bzl index dab5c1346..d6b977ce9 100644 --- a/npm/private/npm_package_store.bzl +++ b/npm/private/npm_package_store.bzl @@ -177,10 +177,7 @@ def _npm_package_store_impl(ctx): msg = "Expected src of '{}' to provide either NpmPackageInfo or JsInfo".format(ctx.label) fail(msg) else: - # ctx.attr.src can be unspecified when the rule is a npm_package_store_internal; when it is _not_ - # set, this is a terminal 3p package with ctx.attr.deps being the transitive closure of - # deps; this pattern is used to break circular dependencies between 3rd party npm deps; it - # is not used for 1st party deps + # ctx.attr.src is unspecified for a 3p package store from npm_package_store_internal package = ctx.attr.package version = ctx.attr.version @@ -361,10 +358,7 @@ deps of npm_package_store must be in the same package.""" % (ctx.label.package, else: direct_ref_deps[dep] = dep_aliases elif not ctx.attr.src: - # ctx.attr.src can be unspecified when the rule is a npm_package_store_internal; when it is _not_ - # set, this is a terminal 3p package with ctx.attr.deps being the transitive closure of - # deps; this pattern is used to break circular dependencies between 3rd party npm deps; it - # is not used for 1st party deps + # ctx.attr.src is unspecified for a 3p package store from npm_package_store_internal deps_map = {} deps_with_ref_deps = [] for dep, _dep_aliases in ctx.attr.deps.items(): @@ -375,7 +369,7 @@ deps of npm_package_store must be in the same package.""" % (ctx.label.package, deps_map[dep_info.key] = dep_info if package_key == dep_info.key: - # provide the node_modules directory for this package if found in the transitive_closure + # This dep is the package's own /pkg target package_store_directory = dep_info.package_store_directory else: # this is a ref npm_link_package, a downstream terminal npm_link_package for this npm @@ -387,15 +381,6 @@ deps of npm_package_store must be in the same package.""" % (ctx.label.package, if dep_info.ref_deps: deps_with_ref_deps.append(dep_info) - # Include the store and js info of all dependencies expected to be linked; - # because deps is the transitive closure, the flat (non-transitive) sources - # and types of each dep are sufficient - dep_js_info = dep[JsInfo] - sources_depsets.append(dep_js_info.sources) - types_depsets.append(dep_js_info.types) - npm_sources_depsets.append(dep_js_info.npm_sources) - npm_package_store_infos.append(dep_info) - for dep_info in deps_with_ref_deps: dep_package_store_name = utils.package_store_name(dep_info.key) @@ -418,28 +403,15 @@ deps of npm_package_store must be in the same package.""" % (ctx.label.package, files.append(package_store_directory) # Include the store and js info of all dependencies expected to be linked - if ctx.attr.src: - for target in ctx.attr.deps: - dep_js_info = target[JsInfo] - sources_depsets.append(dep_js_info.transitive_sources) - types_depsets.append(dep_js_info.transitive_types) - npm_sources_depsets.append(dep_js_info.npm_sources) - npm_package_store_infos.append(target[NpmPackageStoreInfo]) - - for npm_package_store_info in npm_package_store_infos: - transitive_files_depsets.append(npm_package_store_info.transitive_files) - else: - # ctx.attr.src can be unspecified when the rule is a npm_package_store_internal; when ctx.attr.src is - # _not_ set, this is a terminal 3p package with ctx.attr.deps being the transitive closure - # of deps; this pattern is used to break circular dependencies between 3rd party npm deps; - # it is not used for 1st party deps; because npm_package_store_infos is the transitive - # closure of all the entire package store deps, we can safely add just the flat `sources`, - # `types` and `files` from each of these to the transitive depsets; doing so reduces the - # size of the transitive depsets significantly and reduces analysis time and Bazel memory - # usage during analysis; the flat sources and types are already gathered while iterating - # the deps above - for npm_package_store_info in npm_package_store_infos: - transitive_files_depsets.append(npm_package_store_info.files) + for target in ctx.attr.deps: + dep_js_info = target[JsInfo] + sources_depsets.append(dep_js_info.transitive_sources) + types_depsets.append(dep_js_info.transitive_types) + npm_sources_depsets.append(dep_js_info.npm_sources) + npm_package_store_infos.append(target[NpmPackageStoreInfo]) + + for npm_package_store_info in npm_package_store_infos: + transitive_files_depsets.append(npm_package_store_info.transitive_files) sources_depset = depset(transitive = sources_depsets) types_depset = depset(transitive = types_depsets) From f7ade86e49c77ea5a868943359c3951ed6e38e63 Mon Sep 17 00:00:00 2001 From: Peter Goldberg Date: Mon, 2 Nov 2026 13:02:49 +0000 Subject: [PATCH 2/3] test: add e2e case for `npm_link_package` with a package that has a lifecycle hook --- .github/workflows/ci-workflows.yaml | 1 + e2e/npm_link_package_lifecycle/.bazelignore | 1 + e2e/npm_link_package_lifecycle/.bazelrc | 3 + e2e/npm_link_package_lifecycle/.bazelversion | 1 + e2e/npm_link_package_lifecycle/.npmrc | 4 ++ e2e/npm_link_package_lifecycle/BUILD.bazel | 18 ++++++ e2e/npm_link_package_lifecycle/MODULE.bazel | 19 ++++++ e2e/npm_link_package_lifecycle/README.md | 2 + .../lib/BUILD.bazel | 15 +++++ e2e/npm_link_package_lifecycle/lib/index.js | 2 + e2e/npm_link_package_lifecycle/package.json | 6 ++ e2e/npm_link_package_lifecycle/pnpm-lock.yaml | 61 +++++++++++++++++++ .../pnpm-workspace.yaml | 6 ++ e2e/npm_link_package_lifecycle/test.js | 5 ++ 14 files changed, 144 insertions(+) create mode 100644 e2e/npm_link_package_lifecycle/.bazelignore create mode 100644 e2e/npm_link_package_lifecycle/.bazelrc create mode 100644 e2e/npm_link_package_lifecycle/.bazelversion create mode 100644 e2e/npm_link_package_lifecycle/.npmrc create mode 100644 e2e/npm_link_package_lifecycle/BUILD.bazel create mode 100644 e2e/npm_link_package_lifecycle/MODULE.bazel create mode 100644 e2e/npm_link_package_lifecycle/README.md create mode 100644 e2e/npm_link_package_lifecycle/lib/BUILD.bazel create mode 100644 e2e/npm_link_package_lifecycle/lib/index.js create mode 100644 e2e/npm_link_package_lifecycle/package.json create mode 100644 e2e/npm_link_package_lifecycle/pnpm-lock.yaml create mode 100644 e2e/npm_link_package_lifecycle/pnpm-workspace.yaml create mode 100644 e2e/npm_link_package_lifecycle/test.js diff --git a/.github/workflows/ci-workflows.yaml b/.github/workflows/ci-workflows.yaml index 8a8dc081c..2770b1bcc 100644 --- a/.github/workflows/ci-workflows.yaml +++ b/.github/workflows/ci-workflows.yaml @@ -77,6 +77,7 @@ jobs: e2e/no_shell_toolchain e2e/npm_link_package e2e/npm_link_package-rerooted + e2e/npm_link_package_lifecycle e2e/npm_translate_lock e2e/npm_translate_lock_replace_packages e2e/npm_translate_lock_empty diff --git a/e2e/npm_link_package_lifecycle/.bazelignore b/e2e/npm_link_package_lifecycle/.bazelignore new file mode 100644 index 000000000..c2658d7d1 --- /dev/null +++ b/e2e/npm_link_package_lifecycle/.bazelignore @@ -0,0 +1 @@ +node_modules/ diff --git a/e2e/npm_link_package_lifecycle/.bazelrc b/e2e/npm_link_package_lifecycle/.bazelrc new file mode 100644 index 000000000..c36e27d30 --- /dev/null +++ b/e2e/npm_link_package_lifecycle/.bazelrc @@ -0,0 +1,3 @@ +import %workspace%/../../tools/preset.bazelrc +import %workspace%/../e2e.bazelrc + diff --git a/e2e/npm_link_package_lifecycle/.bazelversion b/e2e/npm_link_package_lifecycle/.bazelversion new file mode 100644 index 000000000..5942a0d3a --- /dev/null +++ b/e2e/npm_link_package_lifecycle/.bazelversion @@ -0,0 +1 @@ +7.7.1 diff --git a/e2e/npm_link_package_lifecycle/.npmrc b/e2e/npm_link_package_lifecycle/.npmrc new file mode 100644 index 000000000..5de6efae1 --- /dev/null +++ b/e2e/npm_link_package_lifecycle/.npmrc @@ -0,0 +1,4 @@ +# Disabling pnpm [hoisting](https://pnpm.io/npmrc#hoist) by setting `hoist=false` is recommended on +# projects using rules_js so that pnpm outside of Bazel lays out a node_modules tree similar to what +# rules_js lays out under Bazel (without a hidden node_modules/.pnpm/node_modules) +hoist=false diff --git a/e2e/npm_link_package_lifecycle/BUILD.bazel b/e2e/npm_link_package_lifecycle/BUILD.bazel new file mode 100644 index 000000000..40222f057 --- /dev/null +++ b/e2e/npm_link_package_lifecycle/BUILD.bazel @@ -0,0 +1,18 @@ +load("@aspect_rules_js//js:defs.bzl", "js_test") +load("@aspect_rules_js//npm:defs.bzl", "npm_link_package") +load("@npm//:defs.bzl", "npm_link_all_packages") + +npm_link_all_packages() + +npm_link_package( + name = "node_modules/@e2e/lib", + src = "//lib:pkg", +) + +# The indirect deps of chalk reach the runfiles only through the transitive_files of its package +# store, so this test must not depend on //:node_modules/chalk directly +js_test( + name = "test", + data = [":node_modules/@e2e/lib"], + entry_point = "test.js", +) diff --git a/e2e/npm_link_package_lifecycle/MODULE.bazel b/e2e/npm_link_package_lifecycle/MODULE.bazel new file mode 100644 index 000000000..4b4f45f59 --- /dev/null +++ b/e2e/npm_link_package_lifecycle/MODULE.bazel @@ -0,0 +1,19 @@ +bazel_dep(name = "aspect_rules_js", version = "0.0.0", dev_dependency = True) +local_path_override( + module_name = "aspect_rules_js", + path = "../..", +) + +npm = use_extension( + "@aspect_rules_js//npm:extensions.bzl", + "npm", + dev_dependency = True, +) +npm.npm_translate_lock( + name = "npm", + data = ["//:package.json"], + npmrc = "//:.npmrc", + pnpm_lock = "//:pnpm-lock.yaml", + verify_node_modules_ignored = "//:.bazelignore", +) +use_repo(npm, "npm") diff --git a/e2e/npm_link_package_lifecycle/README.md b/e2e/npm_link_package_lifecycle/README.md new file mode 100644 index 000000000..0a64fb750 --- /dev/null +++ b/e2e/npm_link_package_lifecycle/README.md @@ -0,0 +1,2 @@ +This e2e tests that a first-party `npm_package` linked with `npm_link_package` gets the indirect +deps of an npm package with a lifecycle build target in its runfiles. diff --git a/e2e/npm_link_package_lifecycle/lib/BUILD.bazel b/e2e/npm_link_package_lifecycle/lib/BUILD.bazel new file mode 100644 index 000000000..00b09b7ff --- /dev/null +++ b/e2e/npm_link_package_lifecycle/lib/BUILD.bazel @@ -0,0 +1,15 @@ +load("@aspect_rules_js//js:defs.bzl", "js_library") +load("@aspect_rules_js//npm:defs.bzl", "npm_package") + +js_library( + name = "lib", + srcs = ["index.js"], + deps = ["//:node_modules/chalk"], +) + +npm_package( + name = "pkg", + srcs = [":lib"], + package = "@e2e/lib", + visibility = ["//visibility:public"], +) diff --git a/e2e/npm_link_package_lifecycle/lib/index.js b/e2e/npm_link_package_lifecycle/lib/index.js new file mode 100644 index 000000000..ffe76b053 --- /dev/null +++ b/e2e/npm_link_package_lifecycle/lib/index.js @@ -0,0 +1,2 @@ +// chalk requires supports-color, which requires has-flag +module.exports = require('chalk') diff --git a/e2e/npm_link_package_lifecycle/package.json b/e2e/npm_link_package_lifecycle/package.json new file mode 100644 index 000000000..f52b2594f --- /dev/null +++ b/e2e/npm_link_package_lifecycle/package.json @@ -0,0 +1,6 @@ +{ + "private": true, + "dependencies": { + "chalk": "4.1.2" + } +} diff --git a/e2e/npm_link_package_lifecycle/pnpm-lock.yaml b/e2e/npm_link_package_lifecycle/pnpm-lock.yaml new file mode 100644 index 000000000..3d8895df7 --- /dev/null +++ b/e2e/npm_link_package_lifecycle/pnpm-lock.yaml @@ -0,0 +1,61 @@ +lockfileVersion: '9.0' + +settings: + autoInstallPeers: true + excludeLinksFromLockfile: false + +importers: + + .: + dependencies: + chalk: + specifier: 4.1.2 + version: 4.1.2 + +packages: + + ansi-styles@4.3.0: + resolution: {integrity: sha512-zbB9rCJAT1rbjiVDb2hqKFHNYLxgtk8NURxZ3IZwD3F6NtxbXZQCnnSi1Lkx+IDohdPlFp222wVALIheZJQSEg==} + engines: {node: '>=8'} + + chalk@4.1.2: + resolution: {integrity: sha512-oKnbhFyRIXpUuez8iBMmyEa4nbj4IOQyuhc/wy9kY7/WVPcwIO9VA668Pu8RkO7+0G76SLROeyw9CpQ061i4mA==} + engines: {node: '>=10'} + + color-convert@2.0.1: + resolution: {integrity: sha512-RRECPsj7iu/xb5oKYcsFHSppFNnsj/52OVTRKb4zP5onXwVF3zVmmToNcOfGC+CRDpfK/U584fMg38ZHCaElKQ==} + engines: {node: '>=7.0.0'} + + color-name@1.1.4: + resolution: {integrity: sha512-dOy+3AuW3a2wNbZHIuMZpTcgjGuLU/uBL/ubcZF9OXbDo8ff4O8yVp5Bf0efS8uEoYo5q4Fx7dY9OgQGXgAsQA==} + + has-flag@4.0.0: + resolution: {integrity: sha512-EykJT/Q1KjTWctppgIAgfSO0tKVuZUjhgMr17kqTumMl6Afv3EISleU7qZUzoXDFTAHTDC4NOoG/ZxU3EvlMPQ==} + engines: {node: '>=8'} + + supports-color@7.2.0: + resolution: {integrity: sha512-qpCAvRl9stuOHveKsn7HncJRvv501qIacKzQlO/+Lwxc9+0q2wLyv4Dfvt80/DPn2pqOBsJdDiogXGR9+OvwRw==} + engines: {node: '>=8'} + +snapshots: + + ansi-styles@4.3.0: + dependencies: + color-convert: 2.0.1 + + chalk@4.1.2: + dependencies: + ansi-styles: 4.3.0 + supports-color: 7.2.0 + + color-convert@2.0.1: + dependencies: + color-name: 1.1.4 + + color-name@1.1.4: {} + + has-flag@4.0.0: {} + + supports-color@7.2.0: + dependencies: + has-flag: 4.0.0 diff --git a/e2e/npm_link_package_lifecycle/pnpm-workspace.yaml b/e2e/npm_link_package_lifecycle/pnpm-workspace.yaml new file mode 100644 index 000000000..5abe0ef61 --- /dev/null +++ b/e2e/npm_link_package_lifecycle/pnpm-workspace.yaml @@ -0,0 +1,6 @@ +packages: + - '.' + +# chalk has no install scripts, but listing it here still gives it a lifecycle build target +onlyBuiltDependencies: + - 'chalk' diff --git a/e2e/npm_link_package_lifecycle/test.js b/e2e/npm_link_package_lifecycle/test.js new file mode 100644 index 000000000..830ef43a2 --- /dev/null +++ b/e2e/npm_link_package_lifecycle/test.js @@ -0,0 +1,5 @@ +const chalk = require('@e2e/lib') + +if (typeof chalk.red !== 'function') { + throw new Error('expected @e2e/lib to export chalk') +} From ed4c5fbbfeba157fe793397b70b57dd84b32abcd Mon Sep 17 00:00:00 2001 From: Peter Goldberg Date: Mon, 2 Nov 2026 13:46:19 +0000 Subject: [PATCH 3/3] test: check that package store transitive_files includes indirect deps --- npm/private/test/BUILD.bazel | 3 + npm/private/test/npm_package_store_test.bzl | 74 +++++++++++++++++++++ 2 files changed, 77 insertions(+) create mode 100644 npm/private/test/npm_package_store_test.bzl diff --git a/npm/private/test/BUILD.bazel b/npm/private/test/BUILD.bazel index 9310f69ad..98688d46c 100644 --- a/npm/private/test/BUILD.bazel +++ b/npm/private/test/BUILD.bazel @@ -7,6 +7,7 @@ load("@npm__meaning-of-life__1.0.0__links//:defs.bzl", "npm_link_imported_packag load("@rules_shell//shell:sh_test.bzl", "sh_test") load(":generated_pkg_json_test.bzl", "generated_pkg_json_test") load(":npm_auth_test.bzl", "npm_auth_failure_test_suite", "npm_auth_test_suite") +load(":npm_package_store_test.bzl", "npm_package_store_test_suite") load(":npm_package_visibility_test.bzl", "npm_package_visibility_tests") load(":npmrc_test.bzl", "npmrc_tests") load(":parse_pnpm_lock_tests.bzl", "parse_pnpm_lock_tests") @@ -47,6 +48,8 @@ npm_auth_test_suite() npm_auth_failure_test_suite(name = "npm_auth_failure_tests") +npm_package_store_test_suite(name = "test_npm_package_store") + write_source_files( name = "write_npm_translate_lock", files = { diff --git a/npm/private/test/npm_package_store_test.bzl b/npm/private/test/npm_package_store_test.bzl new file mode 100644 index 000000000..ee2d74d68 --- /dev/null +++ b/npm/private/test/npm_package_store_test.bzl @@ -0,0 +1,74 @@ +"""Analysis tests for npm_package_store""" + +load("@bazel_skylib//lib:unittest.bzl", "analysistest", "asserts") +load("//js:providers.bzl", "JsInfo") +load("//npm/private:npm_package_store_info.bzl", "NpmPackageStoreInfo") + +def _transitive_files_test_impl(ctx): + env = analysistest.begin(ctx) + target = analysistest.target_under_test(env) + + transitive_files = [f.short_path for f in target[NpmPackageStoreInfo].transitive_files.to_list()] + + indirect_dep_store_directory = ctx.attr.indirect_dep[NpmPackageStoreInfo].package_store_directory + asserts.true( + env, + indirect_dep_store_directory.short_path in transitive_files, + "expected transitive_files to contain {}".format(indirect_dep_store_directory.short_path), + ) + + # npm_sources of a third-party package store already covers its whole closure + asserts.equals( + env, + sorted([f.short_path for f in target[JsInfo].npm_sources.to_list()]), + sorted(transitive_files), + ) + return analysistest.end(env) + +_transitive_files_test = analysistest.make( + _transitive_files_test_impl, + attrs = { + "indirect_dep": attr.label( + doc = "Store target of a dep that the target under test only reaches through another dep", + mandatory = True, + providers = [NpmPackageStoreInfo], + ), + }, +) + +def npm_package_store_test_suite(name): + """Test suite for npm_package_store + + Args: + name: Target name of the test_suite target. + """ + + # chalk depends on has-flag through supports-color + _transitive_files_test( + name = "basic_transitive_files_test", + target_under_test = "//:.aspect_rules_js/node_modules/chalk@4.1.2", + indirect_dep = "//:.aspect_rules_js/node_modules/has-flag@4.0.0", + ) + + # segfault-handler has a lifecycle build target. It depends on file-uri-to-path through bindings + _transitive_files_test( + name = "lifecycle_transitive_files_test", + target_under_test = "//:.aspect_rules_js/node_modules/segfault-handler@1.3.0", + indirect_dep = "//:.aspect_rules_js/node_modules/file-uri-to-path@1.0.0", + ) + + # es5-ext is in a dependency cycle. It depends on type through esniff + _transitive_files_test( + name = "circular_transitive_files_test", + target_under_test = "//:.aspect_rules_js/node_modules/es5-ext@0.10.64", + indirect_dep = "//:.aspect_rules_js/node_modules/type@2.7.3", + ) + + native.test_suite( + name = name, + tests = [ + ":basic_transitive_files_test", + ":lifecycle_transitive_files_test", + ":circular_transitive_files_test", + ], + )