From 060cc1c96e2454dd0690f6033b7babf0cdc6c9fd Mon Sep 17 00:00:00 2001 From: Greg Magolan Date: Tue, 2 Apr 2024 22:30:28 -0400 Subject: [PATCH] refactor: add include_sources and include_transitive_declarations to js_filegroup rules and gather_files_from_js_providers + gather_runfiles helpers (#1585) --- docs/js_filegroup.md | 8 ++-- js/private/js_filegroup.bzl | 19 ++++++-- js/private/js_helpers.bzl | 45 +++++++++++++++---- js/private/js_run_devserver.bzl | 2 + .../test/create_launcher/custom_test.bzl | 2 + 5 files changed, 61 insertions(+), 15 deletions(-) diff --git a/docs/js_filegroup.md b/docs/js_filegroup.md index 69aa99322..d075d21b0 100644 --- a/docs/js_filegroup.md +++ b/docs/js_filegroup.md @@ -7,8 +7,8 @@ Helper rule to gather files from JsInfo providers of targets and provide them as ## js_filegroup 
-js_filegroup(name, srcs, include_declarations, include_npm_linked_packages,
-             include_transitive_sources)
+js_filegroup(name, srcs, include_declarations, include_npm_linked_packages, include_sources,
+             include_transitive_declarations, include_transitive_sources)
Gathers files from the JsInfo providers from targets in srcs and provides them as default outputs. @@ -22,8 +22,10 @@ This helper rule is used by the `js_run_binary` macro. | :------------- | :------------- | :------------- | :------------- | :------------- | | name | A unique name for this target. | Name | required | | | srcs | List of targets to gather files from. | List of labels | optional | `[]` | -| include_declarations | When True, `declarations` and `transitive_declarations` from `JsInfo` providers in srcs targets are included in the default outputs of the target.

Defaults to false since declarations are generally not needed at runtime and introducing them could slow down developer round trip time due to having to generate typings on source file changes. | Boolean | optional | `False` | +| include_declarations | When True, `declarations` from `JsInfo` providers in srcs targets are included in the default outputs of the target.

Defaults to False since declarations are generally not needed at runtime and introducing them could slow down developer round trip time due to having to generate typings on source file changes. | Boolean | optional | `False` | | include_npm_linked_packages | When True, files in `npm_linked_packages` and `transitive_npm_linked_packages` from `JsInfo` providers in srcs targets are included in the default outputs of the target.

`transitive_files` from `NpmPackageStoreInfo` providers in data targets are also included in the default outputs of the target. | Boolean | optional | `True` | +| include_sources | When True, `sources` from `JsInfo` providers in `srcs` targets are included in the default outputs of the target. | Boolean | optional | `True` | +| include_transitive_declarations | When True, `transitive_declarations` from `JsInfo` providers in srcs targets are included in the default outputs of the target.

Defaults to False since declarations are generally not needed at runtime and introducing them could slow down developer round trip time due to having to generate typings on source file changes. | Boolean | optional | `False` | | include_transitive_sources | When True, `transitive_sources` from `JsInfo` providers in `srcs` targets are included in the default outputs of the target. | Boolean | optional | `True` | diff --git a/js/private/js_filegroup.bzl b/js/private/js_filegroup.bzl index 1870d1ecc..9c6df1426 100644 --- a/js/private/js_filegroup.bzl +++ b/js/private/js_filegroup.bzl @@ -10,13 +10,13 @@ This helper rule is used by the `js_run_binary` macro. def _js_filegroup_impl(ctx): return DefaultInfo(files = _gather_files_from_js_providers( targets = ctx.attr.srcs, + include_sources = ctx.attr.include_sources, include_transitive_sources = ctx.attr.include_transitive_sources, include_declarations = ctx.attr.include_declarations, + include_transitive_declarations = ctx.attr.include_transitive_declarations, include_npm_linked_packages = ctx.attr.include_npm_linked_packages, )) -# TODO(2.0): Add an include_direct_sources & include_direct_declarations & include_direct_npm_linked_packages and -# rename include_declarations to include_transitive declarations & include_npm_linked_packages to include_transitive_npm_linked_packages. js_filegroup = rule( doc = _DOC, implementation = _js_filegroup_impl, @@ -25,14 +25,25 @@ js_filegroup = rule( doc = """List of targets to gather files from.""", allow_files = True, ), + "include_sources": attr.bool( + doc = """When True, `sources` from `JsInfo` providers in `srcs` targets are included in the default outputs of the target.""", + default = True, + ), "include_transitive_sources": attr.bool( doc = """When True, `transitive_sources` from `JsInfo` providers in `srcs` targets are included in the default outputs of the target.""", default = True, ), "include_declarations": attr.bool( - doc = """When True, `declarations` and `transitive_declarations` from `JsInfo` providers in srcs targets are included in the default outputs of the target. + doc = """When True, `declarations` from `JsInfo` providers in srcs targets are included in the default outputs of the target. + + Defaults to False since declarations are generally not needed at runtime and introducing them could slow down developer round trip + time due to having to generate typings on source file changes.""", + default = False, + ), + "include_transitive_declarations": attr.bool( + doc = """When True, `transitive_declarations` from `JsInfo` providers in srcs targets are included in the default outputs of the target. - Defaults to false since declarations are generally not needed at runtime and introducing them could slow down developer round trip + Defaults to False since declarations are generally not needed at runtime and introducing them could slow down developer round trip time due to having to generate typings on source file changes.""", default = False, ), diff --git a/js/private/js_helpers.bzl b/js/private/js_helpers.bzl index 862ea9033..0e766ae3e 100644 --- a/js/private/js_helpers.bzl +++ b/js/private/js_helpers.bzl @@ -174,7 +174,19 @@ this option is not needed. return copy_file_to_bin_action(ctx, file) -def gather_runfiles(ctx, sources, data, deps, data_files = [], copy_data_files_to_bin = False, no_copy_to_bin = [], include_transitive_sources = True, include_declarations = False, include_npm_linked_packages = True): +def gather_runfiles( + ctx, + sources, + data, + deps, + data_files = [], + copy_data_files_to_bin = False, + no_copy_to_bin = [], + include_sources = True, + include_transitive_sources = True, + include_declarations = False, + include_transitive_declarations = False, + include_npm_linked_packages = True): """Creates a runfiles object containing files in `sources`, default outputs from `data` and transitive runfiles from `data` & `deps`. As a defense in depth against `data` & `deps` targets not supplying all required runfiles, also @@ -210,10 +222,14 @@ def gather_runfiles(ctx, sources, data, deps, data_files = [], copy_data_files_t file such as a file in an external repository. In most cases, this option is not needed. See `copy_data_files_to_bin` docstring for more info. + include_sources: see js_filegroup documentation + include_transitive_sources: see js_filegroup documentation include_declarations: see js_filegroup documentation + include_transitive_declarations: see js_filegroup documentation + include_npm_linked_packages: see js_filegroup documentation Returns: @@ -231,12 +247,13 @@ def gather_runfiles(ctx, sources, data, deps, data_files = [], copy_data_files_t for target in data ]) - # Gather the transitive sources & transitive npm linked packages from the JsInfo & - # NpmPackageStoreInfo providers of data & deps targets. + # Gather files from JsInfo providers of data & deps transitive_files_depsets.append(gather_files_from_js_providers( targets = data + deps, + include_sources = include_sources, include_transitive_sources = include_transitive_sources, include_declarations = include_declarations, + include_transitive_declarations = include_transitive_declarations, include_npm_linked_packages = include_npm_linked_packages, )) @@ -296,25 +313,31 @@ def envs_for_log_level(log_level): def gather_files_from_js_providers( targets, + include_sources, include_transitive_sources, include_declarations, + include_transitive_declarations, include_npm_linked_packages): """Gathers files from JsInfo and NpmPackageStoreInfo providers. Args: targets: list of target to gather from + include_sources: see js_filegroup documentation include_transitive_sources: see js_filegroup documentation include_declarations: see js_filegroup documentation + include_transitive_declarations: see js_filegroup documentation include_npm_linked_packages: see js_filegroup documentation Returns: A depset of files """ - files_depsets = [ - target[JsInfo].sources - for target in targets - if JsInfo in target and hasattr(target[JsInfo], "sources") - ] + files_depsets = [] + if include_sources: + files_depsets = [ + target[JsInfo].sources + for target in targets + if JsInfo in target and hasattr(target[JsInfo], "sources") + ] if include_transitive_sources: files_depsets.extend([ target[JsInfo].transitive_sources @@ -322,6 +345,12 @@ def gather_files_from_js_providers( if JsInfo in target and hasattr(target[JsInfo], "transitive_sources") ]) if include_declarations: + files_depsets.extend([ + target[JsInfo].declarations + for target in targets + if JsInfo in target and hasattr(target[JsInfo], "declarations") + ]) + if include_transitive_declarations: files_depsets.extend([ target[JsInfo].transitive_declarations for target in targets diff --git a/js/private/js_run_devserver.bzl b/js/private/js_run_devserver.bzl index 00950c5df..433fd2579 100644 --- a/js/private/js_run_devserver.bzl +++ b/js/private/js_run_devserver.bzl @@ -42,8 +42,10 @@ def _js_run_devserver_impl(ctx): transitive_runfiles = [_gather_files_from_js_providers( targets = ctx.attr.data, + include_sources = True, include_transitive_sources = ctx.attr.include_transitive_sources, include_declarations = ctx.attr.include_declarations, + include_transitive_declarations = ctx.attr.include_declarations, include_npm_linked_packages = ctx.attr.include_npm_linked_packages, )] diff --git a/js/private/test/create_launcher/custom_test.bzl b/js/private/test/create_launcher/custom_test.bzl index c92e8934c..e9179a3f2 100644 --- a/js/private/test/create_launcher/custom_test.bzl +++ b/js/private/test/create_launcher/custom_test.bzl @@ -21,8 +21,10 @@ def _custom_test_impl(ctx): files = ctx.files.data, transitive_files = js_lib_helpers.gather_files_from_js_providers( targets = ctx.attr.data, + include_sources = True, include_transitive_sources = ctx.attr.include_transitive_sources, include_declarations = ctx.attr.include_declarations, + include_transitive_declarations = ctx.attr.include_declarations, include_npm_linked_packages = ctx.attr.include_npm_linked_packages, ), ).merge(launcher.runfiles).merge_all([