feat: add package_name to ts_library

BREAKING CHANGES:

module_name will no longer turn on linking for the ts_library target; instead package_name must now be specified to enable linking. package_path may also be specified to control the link location.

This PR includes a breaking change to simplify module_mappings_aspect in internal/linker/link_node_modules.bzl now that a ts_library special case is no longer needed.

NB: the alternative would be to remove linking support from ts_library entirely as a js_library or npm_link could be used instead, however, given that ts_library doesn't expose its .js output as the default output this would require users to use a filegroup with an output selector to maintain the same functionality as ts_library currently has with module_name. This PR keeps the BREAKING CHANGE to just adding a package_name attribute to make the ts_library js outputs linkable.

docs/Built-ins.md

@@ -1193,15 +1193,19 @@ Defaults to `@build_bazel_rules_nodejs//internal:node_context_data`
 
 <h4 id="pkg_npm-package_name">package_name</h4>
 
-(*String*): Optional package_name that this npm package may be imported as.
+(*String*): The package name that the linker will link this npm package as.
+
+If package_path is set, the linker will link this package under <package_path>/node_modules/<package_name>.
+If package_path is not set the this will be the root node_modules of the workspace.
 
 Defaults to `""`
 
 <h4 id="pkg_npm-package_path">package_path</h4>
 
-(*String*): The directory in the workspace to link to.
-If set, link this pkg_npm to the node_modules under the package path specified.
-If unset, the default is to link to the node_modules root of the workspace.
+(*String*): The package path in the workspace that the linker will link this npm package to.
+
+If package_path is set, the linker will link this package under <package_path>/node_modules/<package_name>.
+If package_path is not set the this will be the root node_modules of the workspace.
 
 Defaults to `""`
 

docs/Providers.md

@@ -168,7 +168,7 @@ Historical note: this was the typescript.es5_sources output.
 **USAGE**
 
 <pre>
-LinkablePackageInfo(<a href="#LinkablePackageInfo-files">files</a>, <a href="#LinkablePackageInfo-package_name">package_name</a>, <a href="#LinkablePackageInfo-package_path">package_path</a>, <a href="#LinkablePackageInfo-path">path</a>, <a href="#LinkablePackageInfo-_tslibrary">_tslibrary</a>)
+LinkablePackageInfo(<a href="#LinkablePackageInfo-files">files</a>, <a href="#LinkablePackageInfo-package_name">package_name</a>, <a href="#LinkablePackageInfo-package_path">package_path</a>, <a href="#LinkablePackageInfo-path">path</a>)
 </pre>
 
 The LinkablePackageInfo provider provides information to the linker for linking pkg_npm built packages
@@ -202,9 +202,6 @@ or a source file path such as,
 
 `path/to/package` or
 `external/<external_wksp>/path/to/package` 
-<h4 id="LinkablePackageInfo-_tslibrary">_tslibrary</h4>
-
- For internal use only 
 
 
 ## NodeContextInfo

docs/TypeScript.md

@@ -185,8 +185,9 @@ Defaults to `[]`
 <pre>
 ts_library(<a href="#ts_library-name">name</a>, <a href="#ts_library-angular_assets">angular_assets</a>, <a href="#ts_library-compiler">compiler</a>, <a href="#ts_library-data">data</a>, <a href="#ts_library-deps">deps</a>, <a href="#ts_library-devmode_module">devmode_module</a>, <a href="#ts_library-devmode_target">devmode_target</a>,
            <a href="#ts_library-expected_diagnostics">expected_diagnostics</a>, <a href="#ts_library-generate_externs">generate_externs</a>, <a href="#ts_library-internal_testing_type_check_dependencies">internal_testing_type_check_dependencies</a>,
-           <a href="#ts_library-link_workspace_root">link_workspace_root</a>, <a href="#ts_library-module_name">module_name</a>, <a href="#ts_library-module_root">module_root</a>, <a href="#ts_library-prodmode_module">prodmode_module</a>, <a href="#ts_library-prodmode_target">prodmode_target</a>, <a href="#ts_library-runtime">runtime</a>,
-           <a href="#ts_library-runtime_deps">runtime_deps</a>, <a href="#ts_library-srcs">srcs</a>, <a href="#ts_library-supports_workers">supports_workers</a>, <a href="#ts_library-tsconfig">tsconfig</a>, <a href="#ts_library-tsickle_typed">tsickle_typed</a>, <a href="#ts_library-use_angular_plugin">use_angular_plugin</a>)
+           <a href="#ts_library-link_workspace_root">link_workspace_root</a>, <a href="#ts_library-module_name">module_name</a>, <a href="#ts_library-module_root">module_root</a>, <a href="#ts_library-package_name">package_name</a>, <a href="#ts_library-package_path">package_path</a>, <a href="#ts_library-prodmode_module">prodmode_module</a>,
+           <a href="#ts_library-prodmode_target">prodmode_target</a>, <a href="#ts_library-runtime">runtime</a>, <a href="#ts_library-runtime_deps">runtime_deps</a>, <a href="#ts_library-srcs">srcs</a>, <a href="#ts_library-supports_workers">supports_workers</a>, <a href="#ts_library-tsconfig">tsconfig</a>, <a href="#ts_library-tsickle_typed">tsickle_typed</a>,
+           <a href="#ts_library-use_angular_plugin">use_angular_plugin</a>)
 </pre>
 
 type-check and compile a set of TypeScript sources to JavaScript.
@@ -338,7 +339,8 @@ Defaults to `False`
 <h4 id="ts_library-link_workspace_root">link_workspace_root</h4>
 
 (*Boolean*): Link the workspace root to the bin_dir to support absolute requires like 'my_wksp/path/to/file'.
-    If source files need to be required then they can be copied to the bin_dir with copy_to_bin.
+
+If source files need to be required then they can be copied to the bin_dir with copy_to_bin.
 
 Defaults to `False`
 
@@ -354,6 +356,24 @@ Defaults to `""`
 
 Defaults to `""`
 
+<h4 id="ts_library-package_name">package_name</h4>
+
+(*String*): The package name that the linker will link this ts_library output as.
+
+If package_path is set, the linker will link this package under <package_path>/node_modules/<package_name>.
+If package_path is not set the this will be the root node_modules of the workspace.
+
+Defaults to `""`
+
+<h4 id="ts_library-package_path">package_path</h4>
+
+(*String*): The package path in the workspace that the linker will link this ts_library output to.
+
+If package_path is set, the linker will link this package under <package_path>/node_modules/<package_name>.
+If package_path is not set the this will be the root node_modules of the workspace.
+
+Defaults to `""`
+
 <h4 id="ts_library-prodmode_module">prodmode_module</h4>
 
 (*String*): Set the typescript `module` compiler option for prodmode output.

examples/angular/src/lib/shorten/BUILD.bazel

@@ -6,6 +6,7 @@ package(default_visibility = ["//:__subpackages__"])
 # with a main field to show how to create a first-party npm lib with a package.json
 ts_library(
     name = "shorten",
+    package_name = "@bazel/shorten",
     srcs = ["index.ts"],
     module_name = "@bazel/shorten",
 )

examples/angular_view_engine/patches/@angular+bazel+9.0.5.patch

@@ -811,7 +811,7 @@ index 32b640a..a787bbb 100755
  node_modules_aspect = _node_modules_aspect
 
 diff --git a/node_modules/@angular/bazel/src/ng_module.bzl b/node_modules/@angular/bazel/src/ng_module.bzl
-index a38f6a8..436d83b 100755
+index a38f6a8..0bd569d 100755
 --- a/node_modules/@angular/bazel/src/ng_module.bzl
 +++ b/node_modules/@angular/bazel/src/ng_module.bzl
 @@ -13,6 +13,7 @@ load(
@@ -826,18 +826,27 @@ index a38f6a8..436d83b 100755
          # once it is no longer needed.
      ])
 
-+    if ctx.attr.module_name:
++    if ctx.attr.package_name:
 +        path = "/".join([p for p in [ctx.bin_dir.path, ctx.label.workspace_root, ctx.label.package] if p])
 +        ts_providers["providers"].append(LinkablePackageInfo(
-+            package_name = ctx.attr.module_name,
++            package_name = ctx.attr.package_name,
++            package_path = ctx.attr.package_path,
 +            path = path,
 +            files = ts_providers["typescript"]["es5_sources"],
-+            _tslibrary = True,
 +        ))
 +
      return ts_providers_dict_to_struct(ts_providers)
 
  local_deps_aspects = [node_modules_aspect, _collect_summaries_aspect]
+@@ -750,6 +760,8 @@ NG_MODULE_RULE_ATTRS = dict(dict(COMMON_ATTRIBUTES, **NG_MODULE_ATTRIBUTES), **{
+         default = Label("@npm//typescript:typescript__typings"),
+     ),
+     "entry_point": attr.label(allow_single_file = True),
++    "package_name": attr.string(),
++    "package_path": attr.string(),
+
+     # Default is %{name}_public_index
+     # The suffix points to the generated "bundle index" files that users import from
 diff --git a/node_modules/@angular/bazel/src/ng_package/ng_package.bzl b/node_modules/@angular/bazel/src/ng_package/ng_package.bzl
 index db33ad3..0079446 100755
 --- a/node_modules/@angular/bazel/src/ng_package/ng_package.bzl

examples/angular_view_engine/src/lib/shorten/BUILD.bazel

@@ -6,6 +6,7 @@ package(default_visibility = ["//:__subpackages__"])
 # with a main field to show how to create a first-party npm lib with a package.json
 ts_library(
     name = "shorten",
+    package_name = "@bazel/shorten",
     srcs = ["index.ts"],
     module_name = "@bazel/shorten",
 )

examples/angular_view_engine/src/lib/typography/BUILD.bazel

@@ -4,6 +4,7 @@ package(default_visibility = ["//:__subpackages__"])
 
 ng_module(
     name = "typography",
+    package_name = "@examples/angular/typography",
     srcs = glob(["*.ts"]),
     module_name = "@examples/angular/typography",
     tsconfig = "//src:tsconfig.json",

internal/js_library/js_library.bzl

@@ -56,8 +56,18 @@ _ATTRS = {
         They will be copied into the package bin folder if needed.""",
         allow_files = True,
     ),
-    "package_name": attr.string(),
-    "package_path": attr.string(),
+    "package_name": attr.string(
+        doc = """The package name that the linker will link this js_library as.
+
+If package_path is set, the linker will link this package under <package_path>/node_modules/<package_name>.
+If package_path is not set the this will be the root node_modules of the workspace.""",
+    ),
+    "package_path": attr.string(
+        doc = """The package path in the workspace that the linker will link this js_library to.
+
+If package_path is set, the linker will link this package under <package_path>/node_modules/<package_name>.
+If package_path is not set the this will be the root node_modules of the workspace.""",
+    ),
     "srcs": attr.label_list(allow_files = True),
     "strip_prefix": attr.string(
         doc = "Path components to strip from the start of the package import path",

internal/linker/link_node_modules.bzl

@@ -39,29 +39,17 @@ def add_arg(args, arg):
 def _link_mapping(label, mappings, k, v):
     # Check that two package name mapping do not map to two different source paths
     package_name = k.split(":")[0]
-    source_path = v[1]
-
-    # Special case for ts_library module_name for legacy behavior and for AMD name work-around
-    # Mapping is tagged as "__tslibrary__".
-    # See longer comment below in _get_module_mappings for more details.
-    if v[0] != "__tslibrary__":
-        for iter_key, iter_values in mappings.items():
-            # Map key is of format "package_name:package_path"
-            # Map values are of format [deprecated, source_path]
-            iter_package_name = iter_key.split(":")[0]
-            iter_source_path = iter_values[1]
-            if iter_values[0] != "__tslibrary__" and package_name == iter_package_name and source_path != iter_source_path:
-                fail("conflicting mapping at '%s': '%s' and '%s' map to conflicting %s and %s" % (label, k, iter_key, source_path, iter_source_path))
-
-    # Allow __tslibrary__ special case to be overridden other matching mappings
-    if k in mappings and mappings[k] != v:
-        if mappings[k][0] == "__tslibrary__":
-            return True
-        elif v[0] == "__tslibrary__":
-            return False
-        fail(("conflicting mapping at '%s': '%s' maps to both %s and %s" % (label, k, mappings[k], v)), "deps")
-    else:
-        return True
+    link_path = v
+
+    for iter_key, iter_values in mappings.items():
+        # Map key is of format "package_name:package_path"
+        # Map values are of format [deprecated, link_path]
+        iter_package_name = iter_key.split(":")[0]
+        iter_source_path = iter_values
+        if package_name == iter_package_name and link_path != iter_source_path:
+            fail("conflicting mapping at '%s': '%s' and '%s' map to conflicting %s and %s" % (label, k, iter_key, link_path, iter_source_path))
+
+    return True
 
 def write_node_modules_manifest(ctx, extra_data = [], mnemonic = None, link_workspace_root = False):
     """Writes a manifest file read by the linker, containing info about resolving runtime dependencies
@@ -74,7 +62,7 @@ def write_node_modules_manifest(ctx, extra_data = [], mnemonic = None, link_work
             If source files need to be required then they can be copied to the bin_dir with copy_to_bin.
     """
 
-    mappings = {ctx.workspace_name: ["__link__", ctx.bin_dir.path]} if link_workspace_root else {}
+    mappings = {ctx.workspace_name: ctx.bin_dir.path} if link_workspace_root else {}
     node_modules_roots = {}
 
     # Look through data/deps attributes to find the root directories for the third-party node_modules;
@@ -104,7 +92,7 @@ def write_node_modules_manifest(ctx, extra_data = [], mnemonic = None, link_work
     # Convert mappings to a module sets (modules per package package_path)
     # {
     #     "package_path": {
-    #             "package_name": "source_path",
+    #             "package_name": "link_path",
     #             ...
     #     },
     #     ...
@@ -114,10 +102,10 @@ def write_node_modules_manifest(ctx, extra_data = [], mnemonic = None, link_work
         map_key_split = k.split(":")
         package_name = map_key_split[0]
         package_path = map_key_split[1] if len(map_key_split) > 1 else ""
-        source_path = v[1]
+        link_path = v
         if package_path not in module_sets:
             module_sets[package_path] = {}
-        module_sets[package_path][package_name] = source_path
+        module_sets[package_path][package_name] = link_path
 
     # Write the result to a file, and use the magic node option --bazel_node_modules_manifest
     # The launcher.sh will peel off this argument and pass it to the linker rather than the program.
@@ -135,19 +123,15 @@ def write_node_modules_manifest(ctx, extra_data = [], mnemonic = None, link_work
     return modules_manifest
 
 def _get_module_mappings(target, ctx):
-    """Returns the module_mappings from the given attrs.
-
-    Collects a {module_name - module_root} hash from all transitive dependencies,
-    checking for collisions. If a module has a non-empty `module_root` attribute,
-    all sources underneath it are treated as if they were rooted at a folder
-    `module_name`.
+    """Gathers module mappings from LinkablePackageInfo which maps "package_name:package_path" to link_path.
 
     Args:
       target: target
       ctx: ctx
 
     Returns:
-      The module mappings
+        Returns module mappings of shape:
+        { "package_name:package_path": link_path, ... }
     """
     mappings = {}
 
@@ -165,50 +149,27 @@ def _get_module_mappings(target, ctx):
         _debug(ctx.var, "No LinkablePackageInfo for", target.label)
         return mappings
 
+    linkable_package_info = target[LinkablePackageInfo]
+
     # LinkablePackageInfo may be provided without a package_name so check for that case as well
-    if not target[LinkablePackageInfo].package_name:
+    if not linkable_package_info.package_name:
         # No mappings contributed here, short-circuit with the transitive ones we collected
         _debug(ctx.var, "No package_name in LinkablePackageInfo for", target.label)
         return mappings
 
-    linkable_package_info = target[LinkablePackageInfo]
+    package_path = linkable_package_info.package_path if hasattr(linkable_package_info, "package_path") else ""
+    map_key = "%s:%s" % (linkable_package_info.package_name, package_path)
+    map_value = linkable_package_info.path
 
-    if hasattr(linkable_package_info, "package_path") and linkable_package_info.package_path:
-        mn = "%s:%s" % (linkable_package_info.package_name, linkable_package_info.package_path)
-    else:
-        # legacy (root linked) style mapping
-        # TODO(4.0): remove this else condition and always use "%s:%s" style
-        mn = linkable_package_info.package_name
-    mr = ["__link__", linkable_package_info.path]
-
-    # Special case for ts_library module_name for legacy behavior and for AMD name work-around
-    # Tag the mapping as "__tslibrary__" so it can be overridden by any other mapping if found.
-    #
-    # In short, ts_library module_name attribute results in both setting the AMD name (which
-    # desired and necessary in devmode which outputs UMD) and in making a linkable mapping. Because
-    # of this, you can get in the situation where a ts_library module_name and a downstream pkg_name
-    # package_name conflict and result in duplicate mappings. This work-around will make this
-    # situation work however it is not a recommended pattern since a ts_library can be a dep of a
-    # pkg_npm but not vice-verse at the moment since ts_library cannot handle directory artifacts as
-    # deps.
-    #
-    # TODO(4.0): In a future major release, ts_library will get a package_name attribute to enable the linker
-    # and the __tslibrary__ special case can be factored out.
-    # This is planned for 4.0: https://github.com/bazelbuild/rules_nodejs/issues/2450.
-    if hasattr(linkable_package_info, "_tslibrary") and linkable_package_info._tslibrary:
-        mr[0] = "__tslibrary__"
-
-    if _link_mapping(target.label, mappings, mn, mr):
-        _debug(ctx.var, "target %s adding module mapping %s: %s" % (target.label, mn, mr))
-        mappings[mn] = mr
+    if _link_mapping(target.label, mappings, map_key, map_value):
+        _debug(ctx.var, "target %s adding module mapping %s: %s" % (target.label, map_key, map_value))
+        mappings[map_key] = map_value
 
     # Returns mappings of shape:
     # {
-    #   "package_name": [legacy_tslibary_usage, source_path],
-    #   "package_name:package_path": [legacy_tslibary_usage, source_path],
+    #   "package_name:package_path": link_path,
     #   ...
     # }
-    # TODO(4.0): simplify to { "package_name:package_path": source_path, ... } once __tslibrary__ is no longer needed
     return mappings
 
 def _module_mappings_aspect_impl(target, ctx):

internal/pkg_npm/pkg_npm.bzl

@@ -103,12 +103,16 @@ PKG_NPM_ATTRS = dict(NODE_CONTEXT_ATTRS, **{
         allow_files = True,
     ),
     "package_name": attr.string(
-        doc = """Optional package_name that this npm package may be imported as.""",
+        doc = """The package name that the linker will link this npm package as.
+
+If package_path is set, the linker will link this package under <package_path>/node_modules/<package_name>.
+If package_path is not set the this will be the root node_modules of the workspace.""",
     ),
     "package_path": attr.string(
-        doc = """The directory in the workspace to link to.
-If set, link this pkg_npm to the node_modules under the package path specified.
-If unset, the default is to link to the node_modules root of the workspace.""",
+        doc = """The package path in the workspace that the linker will link this npm package to.
+
+If package_path is set, the linker will link this package under <package_path>/node_modules/<package_name>.
+If package_path is not set the this will be the root node_modules of the workspace.""",
     ),
     "srcs": attr.label_list(
         doc = """Files inside this directory which are simply copied into the package.""",

internal/providers/linkable_package_info.bzl

@@ -40,9 +40,5 @@ or a source file path such as,
 `path/to/package` or
 `external/<external_wksp>/path/to/package`
 """,
-        # TODO(4.0): In a future major release, ts_library will get a package_name attribute to enable the linker
-        # and the _tslibrary special case can be removed.
-        # This is planned for 4.0: https://github.com/bazelbuild/rules_nodejs/issues/2450.
-        "_tslibrary": "For internal use only",
     },
 )