fix: do not put sources+types into NpmPackageStoreInfo.files (#1850)

Co-authored-by: Greg Magolan <[email protected]>

examples/linked_consumer/BUILD.bazel

@@ -1,4 +1,6 @@
-load("@aspect_rules_js//js:defs.bzl", "js_test")
+load("@aspect_bazel_lib//lib:copy_to_directory.bzl", "copy_to_directory")
+load("@aspect_bazel_lib//lib:diff_test.bzl", "diff_test")
+load("@aspect_rules_js//js:defs.bzl", "js_info_files", "js_test")
 load("@npm//:defs.bzl", "npm_link_all_packages")
 
 npm_link_all_packages(name = "node_modules")
@@ -22,3 +24,77 @@ js_test(
     ],
     entry_point = "test_pkg_deps_linked.js",
 )
+
+# Test that sources & test can be pulled from a linked js_library the same
+# as they can be pulled out of an unlinked js_library
+js_info_files(
+    name = "unlinked_sources",
+    srcs = ["//examples/linked_lib:lib"],
+    include_npm_sources = False,
+    include_sources = True,
+    include_transitive_sources = True,
+    include_transitive_types = False,
+    include_types = False,
+)
+
+js_info_files(
+    name = "linked_sources",
+    srcs = [":node_modules/@lib/test2"],
+    include_npm_sources = False,
+    include_sources = True,
+    include_transitive_sources = True,
+    include_transitive_types = False,
+    include_types = False,
+)
+
+copy_to_directory(
+    name = "unlinked_sources_dir",
+    srcs = [":unlinked_sources"],
+)
+
+copy_to_directory(
+    name = "linked_sources_dir",
+    srcs = [":linked_sources"],
+)
+
+diff_test(
+    name = "sources_test",
+    file1 = ":unlinked_sources_dir",
+    file2 = ":linked_sources_dir",
+)
+
+js_info_files(
+    name = "linked_types",
+    srcs = [":node_modules/@lib/test2"],
+    include_npm_sources = False,
+    include_sources = False,
+    include_transitive_sources = False,
+    include_transitive_types = True,
+    include_types = True,
+)
+
+js_info_files(
+    name = "unlinked_types",
+    srcs = ["//examples/linked_lib:lib"],
+    include_npm_sources = False,
+    include_sources = False,
+    include_transitive_sources = False,
+    include_transitive_types = True,
+    include_types = True,
+)
+
+copy_to_directory(
+    name = "unlinked_types_dir",
+    srcs = [":unlinked_types"],
+)
+
+copy_to_directory(
+    name = "linked_types_dir",
+    srcs = [":linked_types"],
+)
+
+diff_test(
+    name = "types_test",
+    file1 = ":unlinked_types_dir",
+    file2 = ":linked_types_dir",
+)

examples/linked_lib/BUILD.bazel

@@ -9,6 +9,8 @@ js_library(
     name = "pkg",
     srcs = [
         "index.js",
+        "one.d.ts",
+        "one.js",
         "package.json",
     ],
     visibility = ["//visibility:public"],

examples/linked_lib/one.d.ts

@@ -0,0 +1 @@
+export declare const one = 1

examples/linked_lib/one.js

@@ -0,0 +1,4 @@
+'use strict'
+exports.__esModule = true
+exports.one = void 0
+exports.one = 1

npm/private/npm_link_package_store.bzl

@@ -22,7 +22,7 @@ https://github.com/npm/rfcs/blob/main/accepted/0042-isolated-mode.md.
 _ATTRS = {
     "src": attr.label(
         doc = """The npm_package_store target to link as a direct dependency.""",
-        providers = [NpmPackageStoreInfo],
+        providers = [NpmPackageStoreInfo, JsInfo],
         mandatory = True,
     ),
     "package": attr.string(
@@ -61,6 +61,7 @@ exec node "$basedir/{bin_path}" "$@"
 
 def _npm_link_package_store_impl(ctx):
     store_info = ctx.attr.src[NpmPackageStoreInfo]
+    store_js_info = ctx.attr.src[JsInfo]
 
     package_store_directory = store_info.package_store_directory
     if not package_store_directory:
@@ -91,11 +92,28 @@ def _npm_link_package_store_impl(ctx):
         )
         files.append(bin_file)
 
-    files_depset = depset(files, transitive = [store_info.files])
-
-    transitive_files_depset = depset(files, transitive = [store_info.transitive_files])
+    # All files required to run the package if consumed as `DefaultInfo`
+    files_depset = depset(files, transitive = [
+        store_info.files,
+        store_js_info.npm_sources,
+        store_js_info.sources,
+    ])
+    transitive_files_depset = depset(files, transitive = [
+        store_info.transitive_files,
+        store_js_info.npm_sources,
+        store_js_info.transitive_sources,
+    ])
+
+    # Additional npm_sources required to to run the package, in addition to other
+    # data included in JsInfo provider.
+    npm_sources = depset(files, transitive = [
+        store_info.transitive_files,
+        store_js_info.npm_sources,
+    ])
 
     providers = [
+        # Provide default info to allow consuming the package via `data` of rules
+        # not aware of JsInfo such as `sh_binary` etc.
         DefaultInfo(
             # Only provide direct files in DefaultInfo files
             files = files_depset,
@@ -105,7 +123,11 @@ def _npm_link_package_store_impl(ctx):
         ),
         js_info(
             target = ctx.label,
-            npm_sources = transitive_files_depset,
+            sources = store_js_info.sources,
+            transitive_sources = store_js_info.transitive_sources,
+            types = store_js_info.types,
+            transitive_types = store_js_info.transitive_types,
+            npm_sources = npm_sources,
             # only propagate non-dev npm dependencies to use as direct dependencies when linking downstream npm_package targets with npm_link_package
             npm_package_store_infos = depset([store_info]) if not store_info.dev else depset(),
         ),

npm/private/npm_package_store.bzl

@@ -105,7 +105,7 @@ _ATTRS = {
         > In contrast, Bazel makes it possible to make builds hermetic, which means that
         > all dependencies of a program must be declared when running in Bazel's sandbox.
         """,
-        providers = [NpmPackageStoreInfo],
+        providers = [NpmPackageStoreInfo, JsInfo],
     ),
     "package": attr.string(
         doc = """The package name to link to.
@@ -180,9 +180,17 @@ def _npm_package_store_impl(ctx):
     package_store_name = utils.package_store_name(package, version)
     package_store_directory = None
 
+    # files required to create the package store entry
     files = []
     transitive_files_depsets = []
+
+    # JsInfo of the package and all deps required to run
+    js_infos = []
+
+    # NpmPackageStoreInfo of the package and deps
     npm_package_store_infos = []
+
+    # Direct references to all dependencies
     direct_ref_deps = {}
 
     # the path to the package store location for this package
@@ -277,6 +285,8 @@ deps of npm_package_store must be in the same package.""" % (ctx.label.package,
                 # "node_modules/{package_store_root}/{package_store_name}/node_modules/{package}"
                 dep_symlink_path = "node_modules/{}/{}/node_modules/{}".format(utils.package_store_root, package_store_name, dep_info.package)
                 files.append(utils.make_symlink(ctx, dep_symlink_path, dep_package_store_directory.path))
+
+                # Include the store info of all linked dependencies
                 npm_package_store_infos.append(dep_info)
     elif ctx.attr.src and JsInfo in ctx.attr.src:
         jsinfo = ctx.attr.src[JsInfo]
@@ -287,10 +297,8 @@ deps of npm_package_store must be in the same package.""" % (ctx.label.package,
         else:
             symlink_path = "{}/{}".format(ctx.label.package or ".", package_store_directory_path)
 
-        transitive_files_depsets.append(jsinfo.npm_sources)
-        transitive_files_depsets.append(jsinfo.transitive_sources)
-        transitive_files_depsets.append(jsinfo.transitive_types)
-        npm_package_store_infos.extend(jsinfo.npm_package_store_infos.to_list())
+        # The package JsInfo including all direct and transitive sources, store info etc.
+        js_infos.append(jsinfo)
 
         if jsinfo.target.workspace_name:
             target_path = "{}/external/{}/{}".format(ctx.bin_dir.path, jsinfo.target.workspace_name, jsinfo.target.package)
@@ -346,10 +354,14 @@ deps of npm_package_store must be in the same package.""" % (ctx.label.package,
     if package_store_directory:
         files.append(package_store_directory)
 
+    # Include the store and js info of all dependencies expected to be linked
     for target in ctx.attr.deps:
+        js_infos.append(target[JsInfo])
         npm_package_store_infos.append(target[NpmPackageStoreInfo])
 
     if ctx.attr.src:
+        sources_depset = depset(transitive = [jsinfo.transitive_sources for jsinfo in js_infos])
+        types_depset = depset(transitive = [jsinfo.transitive_types for jsinfo in js_infos])
         for npm_package_store_info in npm_package_store_infos:
             transitive_files_depsets.append(npm_package_store_info.transitive_files)
     else:
@@ -360,21 +372,23 @@ deps of npm_package_store must be in the same package.""" % (ctx.label.package,
         # closure of all the entire package store deps, we can safely add just `files` from each of
         # these to `transitive_files_depset`; doing so reduces the size of `transitive_files_depset`
         # significantly and reduces analysis time and Bazel memory usage during analysis
+        sources_depset = depset(transitive = [jsinfo.sources for jsinfo in js_infos])
+        types_depset = depset(transitive = [jsinfo.types for jsinfo in js_infos])
         for npm_package_store_info in npm_package_store_infos:
             transitive_files_depsets.append(npm_package_store_info.files)
 
+    npm_sources = depset(files, transitive = [jsinfo.npm_sources for jsinfo in js_infos])
     transitive_files_depset = depset(files, transitive = transitive_files_depsets)
-
     files_depset = depset(files)
 
     providers = [
         js_info(
             target = ctx.label,
-            npm_sources = transitive_files_depset,
-        ),
-        DefaultInfo(
-            files = files_depset,
-            runfiles = ctx.runfiles(transitive_files = transitive_files_depset),
+            npm_sources = npm_sources,
+            sources = sources_depset,
+            transitive_sources = sources_depset,
+            types = types_depset,
+            transitive_types = types_depset,
         ),
         NpmPackageStoreInfo(
             root_package = ctx.label.package,