From f8c53075d4d2d02820042840a52edc5449cd746f Mon Sep 17 00:00:00 2001 From: Greg Magolan Date: Mon, 27 May 2024 23:33:48 -0700 Subject: [PATCH] feat: add include_npm_sources to npm_package --- docs/npm_package.md | 26 ++++++---- npm/private/npm_package.bzl | 65 ++++++++++-------------- npm/private/test/npm_package/BUILD.bazel | 26 +++++++++- 3 files changed, 68 insertions(+), 49 deletions(-) diff --git a/docs/npm_package.md b/docs/npm_package.md index 49cd71c2c..ac02bfd5a 100644 --- a/docs/npm_package.md +++ b/docs/npm_package.md @@ -17,7 +17,7 @@ npm_package(name, sr include_external_repositories, include_srcs_packages, exclude_srcs_packages, include_srcs_patterns, exclude_srcs_patterns, replace_prefixes, allow_overwrites, include_sources, include_types, include_transitive_sources, include_transitive_types, - include_runfiles, hardlink, publishable, verbose, kwargs) + include_npm_sources, include_runfiles, hardlink, publishable, verbose, kwargs) A macro that packages sources into a directory (a tree artifact) and provides an `NpmPackageInfo`. @@ -58,18 +58,23 @@ for more information on supported globbing patterns. `npm_package` makes use of `copy_to_directory` (https://docs.aspect.build/rules/aspect_bazel_lib/docs/copy_to_directory) under the hood, adopting its API and its copy action using composition. However, unlike `copy_to_directory`, -`npm_package` includes `transitive_sources` and `transitive_types` files from `JsInfo` providers in srcs +`npm_package` includes direct and transitive sources and types files from `JsInfo` providers in srcs by default. The behavior of including sources and types from `JsInfo` can be configured -using the `include_sources`, `include_transitive_sources`, `include_types`, `include_transitive_types` -attributes. +using the `include_sources`, `include_transitive_sources`, `include_types`, `include_transitive_types`. The two `include*_types` options may cause type-check actions to run, which slows down your development round-trip. You can pass the Bazel option `--@aspect_rules_js//npm:exclude_types_from_npm_packages` to override these two attributes for an individual `bazel` invocation, avoiding the type-check. -`npm_package` also includes default runfiles from `srcs` by default which `copy_to_directory` does not. This behavior -can be configured with the `include_runfiles` attribute. +As of rules_js 2.0, the recommended solution for avoiding eager type-checking when linking +1p deps is to link `js_library` or any `JsInfo` producing targets directly without the +indirection of going through an `npm_package` target (see https://github.com/aspect-build/rules_js/pull/1646 +for more details). + +`npm_package` can also include npm packages sources and default runfiles from `srcs` which `copy_to_directory` does not. +These behaviors can be configured with the `include_npm_sourfes` and `include_runfiles` attributes +respectively. The default `include_srcs_packages`, `[".", "./**"]`, prevents files from outside of the target's package and subpackages from being included. @@ -101,10 +106,11 @@ To stamp the current git tag as the "version" in the package.json file, see | exclude_srcs_patterns | List of paths (with glob support) to exclude from output directory.

Files in srcs are not copied to the output directory if their output directory path, after applying `root_paths`, matches one of the patterns specified.

Forward slashes (`/`) should be used as path separators.

Files that do not have matching output directory paths are subject to subsequent filters and transformations to determine if they are copied and what their path in the output directory will be.

Globs are supported (see rule docstring above). | `["**/node_modules/**"]` | | replace_prefixes | Map of paths prefixes (with glob support) to replace in the output directory path when copying files.

If the output directory path for a file starts with or fully matches a a key in the dict then the matching portion of the output directory path is replaced with the dict value for that key. The final path segment matched can be a partial match of that segment and only the matching portion will be replaced. If there are multiple keys that match, the longest match wins.

Forward slashes (`/`) should be used as path separators.

Replace prefix transformation are the final step in the list of filters and transformations. The final output path of a file being copied into the output directory is determined at this step.

Globs are supported (see rule docstring above). | `{}` | | allow_overwrites | If True, allow files to be overwritten if the same output file is copied to twice.

The order of srcs matters as the last copy of a particular file will win when overwriting. Performance of `npm_package` will be slightly degraded when allow_overwrites is True since copies cannot be parallelized out as they are calculated. Instead all copy paths must be calculated before any copies can be started. | `False` | -| include_sources | When True, `sources` from `JsInfo` providers in `data` targets are included in the list of available files to copy. | `True` | -| include_types | When True, `types` from `JsInfo` providers in `data` targets are included in the list of available files to copy. | `True` | -| include_transitive_sources | When True, `transitive_sources` from `JsInfo` providers in `data` targets are included in the list of available files to copy. | `True` | -| include_transitive_types | When True, `transitive_types` from `JsInfo` providers in `data` targets are included in the list of available files to copy. | `True` | +| include_sources | When True, `sources` from `JsInfo` providers in `srcs` targets are included in the list of available files to copy. | `True` | +| include_types | When True, `types` from `JsInfo` providers in `srcs` targets are included in the list of available files to copy. | `True` | +| include_transitive_sources | When True, `transitive_sources` from `JsInfo` providers in `srcs` targets are included in the list of available files to copy. | `True` | +| include_transitive_types | When True, `transitive_types` from `JsInfo` providers in `srcs` targets are included in the list of available files to copy. | `True` | +| include_npm_sources | When True, `npm_sources` from `JsInfo` providers in `srcs` targets are included in the list of available files to copy. | `False` | | include_runfiles | When True, default runfiles from `srcs` targets are included in the list of available files to copy.

This may be needed in a few cases:

- to work-around issues with rules that don't provide everything needed in sources, transitive_sources, types & transitive_types - to depend on the runfiles targets that don't use JsInfo

NB: The default value will be flipped to False in the next major release as runfiles are not needed in the general case and adding them to the list of files available to copy can add noticeable overhead to the analysis phase in a large repository with many npm_package targets. | `False` | | hardlink | Controls when to use hardlinks to files instead of making copies.

Creating hardlinks is much faster than making copies of files with the caveat that hardlinks share file permissions with their source.

Since Bazel removes write permissions on files in the output tree after an action completes, hardlinks to source files are not recommended since write permissions will be inadvertently removed from sources files.

- `auto`: hardlinks are used for generated files already in the output tree - `off`: all files are copied - `on`: hardlinks are used for all files (not recommended) | `"auto"` | | publishable | When True, enable generation of `{name}.publish` target | `False` | diff --git a/npm/private/npm_package.bzl b/npm/private/npm_package.bzl index 6b487afcb..e32d7896f 100644 --- a/npm/private/npm_package.bzl +++ b/npm/private/npm_package.bzl @@ -39,41 +39,21 @@ _NPM_PACKAGE_FILES_ATTRS = { "include_sources": attr.bool(), "include_transitive_types": attr.bool(), "include_transitive_sources": attr.bool(), + "include_npm_sources": attr.bool(), "srcs": attr.label_list(allow_files = True), } def _npm_package_files_impl(ctx): files_depsets = [] - if ctx.attr.include_transitive_sources: - # include all transitive sources (this includes direct sources) - files_depsets.extend([ - target[JsInfo].transitive_sources - for target in ctx.attr.srcs - if JsInfo in target and hasattr(target[JsInfo], "transitive_sources") - ]) - elif ctx.attr.include_sources: - # include only direct sources - files_depsets.extend([ - target[JsInfo].sources - for target in ctx.attr.srcs - if JsInfo in target and hasattr(target[JsInfo], "sources") - ]) - - if ctx.attr.include_transitive_types: - # include all transitive types (this includes direct types) - files_depsets.extend([ - target[JsInfo].transitive_types - for target in ctx.attr.srcs - if JsInfo in target and hasattr(target[JsInfo], "transitive_types") - ]) - elif ctx.attr.include_types: - # include only direct types - files_depsets.extend([ - target[JsInfo].types - for target in ctx.attr.srcs - if JsInfo in target and hasattr(target[JsInfo], "types") - ]) + files_depsets.append(js_lib_helpers.gather_files_from_js_infos( + ctx.attr.srcs, + include_sources = ctx.attr.include_sources, + include_types = ctx.attr.include_types, + include_transitive_sources = ctx.attr.include_types, + include_transitive_types = ctx.attr.include_transitive_types, + include_npm_sources = ctx.attr.include_npm_sources, + )) if ctx.attr.include_runfiles: # include default runfiles from srcs @@ -171,6 +151,7 @@ def npm_package( include_types = True, include_transitive_sources = True, include_transitive_types = True, + include_npm_sources = False, include_runfiles = False, hardlink = "auto", publishable = False, @@ -214,18 +195,23 @@ def npm_package( `npm_package` makes use of `copy_to_directory` (https://docs.aspect.build/rules/aspect_bazel_lib/docs/copy_to_directory) under the hood, adopting its API and its copy action using composition. However, unlike `copy_to_directory`, - `npm_package` includes `transitive_sources` and `transitive_types` files from `JsInfo` providers in srcs + `npm_package` includes direct and transitive sources and types files from `JsInfo` providers in srcs by default. The behavior of including sources and types from `JsInfo` can be configured - using the `include_sources`, `include_transitive_sources`, `include_types`, `include_transitive_types` - attributes. + using the `include_sources`, `include_transitive_sources`, `include_types`, `include_transitive_types`. The two `include*_types` options may cause type-check actions to run, which slows down your development round-trip. You can pass the Bazel option `--@aspect_rules_js//npm:exclude_types_from_npm_packages` to override these two attributes for an individual `bazel` invocation, avoiding the type-check. - `npm_package` also includes default runfiles from `srcs` by default which `copy_to_directory` does not. This behavior - can be configured with the `include_runfiles` attribute. + As of rules_js 2.0, the recommended solution for avoiding eager type-checking when linking + 1p deps is to link `js_library` or any `JsInfo` producing targets directly without the + indirection of going through an `npm_package` target (see https://github.com/aspect-build/rules_js/pull/1646 + for more details). + + `npm_package` can also include npm packages sources and default runfiles from `srcs` which `copy_to_directory` does not. + These behaviors can be configured with the `include_npm_sourfes` and `include_runfiles` attributes + respectively. The default `include_srcs_packages`, `[".", "./**"]`, prevents files from outside of the target's package and subpackages from being included. @@ -404,13 +390,15 @@ def npm_package( since copies cannot be parallelized out as they are calculated. Instead all copy paths must be calculated before any copies can be started. - include_sources: When True, `sources` from `JsInfo` providers in `data` targets are included in the list of available files to copy. + include_sources: When True, `sources` from `JsInfo` providers in `srcs` targets are included in the list of available files to copy. + + include_types: When True, `types` from `JsInfo` providers in `srcs` targets are included in the list of available files to copy. - include_types: When True, `types` from `JsInfo` providers in `data` targets are included in the list of available files to copy. + include_transitive_sources: When True, `transitive_sources` from `JsInfo` providers in `srcs` targets are included in the list of available files to copy. - include_transitive_sources: When True, `transitive_sources` from `JsInfo` providers in `data` targets are included in the list of available files to copy. + include_transitive_types: When True, `transitive_types` from `JsInfo` providers in `srcs` targets are included in the list of available files to copy. - include_transitive_types: When True, `transitive_types` from `JsInfo` providers in `data` targets are included in the list of available files to copy. + include_npm_sources: When True, `npm_sources` from `JsInfo` providers in `srcs` targets are included in the list of available files to copy. include_runfiles: When True, default runfiles from `srcs` targets are included in the list of available files to copy. @@ -458,6 +446,7 @@ def npm_package( Label("@aspect_rules_js//npm:exclude_types_from_npm_packages_flag"): False, "//conditions:default": include_transitive_types, }), + include_npm_sources = include_npm_sources, include_runfiles = include_runfiles, # Always tag the target manual since we should only build it when the final target is built. tags = kwargs.get("tags", []) + ["manual"], diff --git a/npm/private/test/npm_package/BUILD.bazel b/npm/private/test/npm_package/BUILD.bazel index 1971dcc38..c671cc78a 100644 --- a/npm/private/test/npm_package/BUILD.bazel +++ b/npm/private/test/npm_package/BUILD.bazel @@ -35,7 +35,31 @@ copy_to_directory( ) diff_test( - name = "test", + name = "test_pkg", + file1 = ":pkg", + file2 = ":expected_pkg", +) + +npm_package( + name = "pkg_with_node_modules", + srcs = [":lib_a"], + exclude_srcs_patterns = [], + include_npm_sources = True, + visibility = ["//visibility:public"], +) + +copy_to_directory( + name = "expected_pkg_with_node_modules", + srcs = [ + "index.js", + ":node_modules/chalk", + ":node_modules/chalk/dir", + ":package.json", + ], +) + +diff_test( + name = "test_pkg_with_node_modules", file1 = ":pkg", file2 = ":expected_pkg", )