fix: include transitive deps in transitive_files for packages with lifecycle hooks

.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

e2e/npm_link_package_lifecycle/.bazelignore

@@ -0,0 +1 @@
+node_modules/

e2e/npm_link_package_lifecycle/.bazelrc

@@ -0,0 +1,3 @@
+import %workspace%/../../tools/preset.bazelrc
+import %workspace%/../e2e.bazelrc
+

e2e/npm_link_package_lifecycle/.bazelversion

@@ -0,0 +1 @@
+7.7.1

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

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",
+)

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")

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.

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"],
+)

e2e/npm_link_package_lifecycle/lib/index.js

@@ -0,0 +1,2 @@
+// chalk requires supports-color, which requires has-flag
+module.exports = require('chalk')

e2e/npm_link_package_lifecycle/package.json

@@ -0,0 +1,6 @@
+{
+    "private": true,
+    "dependencies": {
+        "chalk": "4.1.2"
+    }
+}

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'

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')
+}

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)

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 = {

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",
+        ],
+    )