From 002b638e7699ca5d631a96b29fcda9cbf5ab4e5d Mon Sep 17 00:00:00 2001 From: Jason Bedard Date: Thu, 25 Jul 2024 15:46:53 -0700 Subject: [PATCH] fix: do not put sources+types into NpmPackageStoreInfo.files (#1850) Co-authored-by: Greg Magolan --- examples/linked_consumer/BUILD.bazel | 78 +++++++++++++++++++++++++- examples/linked_lib/BUILD.bazel | 2 + examples/linked_lib/one.d.ts | 1 + examples/linked_lib/one.js | 4 ++ npm/private/npm_link_package_store.bzl | 32 +++++++++-- npm/private/npm_package_store.bzl | 36 ++++++++---- 6 files changed, 136 insertions(+), 17 deletions(-) create mode 100644 examples/linked_lib/one.d.ts create mode 100644 examples/linked_lib/one.js diff --git a/examples/linked_consumer/BUILD.bazel b/examples/linked_consumer/BUILD.bazel index 5d0c39515..662d75705 100644 --- a/examples/linked_consumer/BUILD.bazel +++ b/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", +) diff --git a/examples/linked_lib/BUILD.bazel b/examples/linked_lib/BUILD.bazel index 70b9dd1e7..82045c6ca 100644 --- a/examples/linked_lib/BUILD.bazel +++ b/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"], diff --git a/examples/linked_lib/one.d.ts b/examples/linked_lib/one.d.ts new file mode 100644 index 000000000..b9dc8dd63 --- /dev/null +++ b/examples/linked_lib/one.d.ts @@ -0,0 +1 @@ +export declare const one = 1 diff --git a/examples/linked_lib/one.js b/examples/linked_lib/one.js new file mode 100644 index 000000000..e4d42eea3 --- /dev/null +++ b/examples/linked_lib/one.js @@ -0,0 +1,4 @@ +'use strict' +exports.__esModule = true +exports.one = void 0 +exports.one = 1 diff --git a/npm/private/npm_link_package_store.bzl b/npm/private/npm_link_package_store.bzl index a1798a498..71c9801d3 100644 --- a/npm/private/npm_link_package_store.bzl +++ b/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(), ), diff --git a/npm/private/npm_package_store.bzl b/npm/private/npm_package_store.bzl index 862c0f746..8b3e2a508 100644 --- a/npm/private/npm_package_store.bzl +++ b/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,