feat(builtin): new js_library rule (#2109)

Removes `node_module_library` private API and renames it to `js_library`

the latter will be promoted to a public API in a followup change

Author: mistic

WORKSPACE

@@ -51,9 +51,9 @@ yarn_install(
     # The @npm//:node_modules_filegroup generated by manual_build_file_contents
     # is used in the //packages/typescript/test/reference_types_directive:tsconfig_types
     # test. For now we're still supporting node_modules as a filegroup tho this may
-    # change in the future. The default generated //:node_modules target is a node_module_library
-    # rule which provides NpmPackageInfo but that rule is not yet in the public API and we
-    # have not yet dropped support for filegroup based node_modules target.
+    # change in the future. The default generated //:node_modules target is a js_library
+    # rule which provides NpmPackageInfo but we have not yet dropped support for
+    # filegroup based node_modules target.
     manual_build_file_contents = """
 filegroup(
   name = "node_modules_filegroup",

internal/js_library/js_library.bzl

@@ -12,13 +12,27 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-"""js_library allows defining a set of javascript sources and assigning a package_name.
+"""js_library can be used to expose and share any library package.
 
-DO NOT USE - this is not fully designed, and exists only to enable testing within this repo.
+DO NOT USE - this is not fully designed yet and it is a work in progress.
 """
 
-load("//:providers.bzl", "LinkablePackageInfo", "declaration_info", "js_module_info")
-load("//third_party/github.com/bazelbuild/bazel-skylib:rules/private/copy_file_private.bzl", "copy_bash", "copy_cmd")
+load(
+    "//:providers.bzl",
+    "DeclarationInfo",
+    "JSModuleInfo",
+    "JSNamedModuleInfo",
+    "LinkablePackageInfo",
+    "NpmPackageInfo",
+    "declaration_info",
+    "js_module_info",
+    "js_named_module_info",
+)
+load(
+    "//third_party/github.com/bazelbuild/bazel-skylib:rules/private/copy_file_private.bzl",
+    "copy_bash",
+    "copy_cmd",
+)
 
 _AMD_NAMES_DOC = """Mapping from require module names to global variables.
 This allows devmode JS sources to load unnamed UMD bundles from third-party libraries."""
@@ -50,90 +64,201 @@ def write_amd_names_shim(actions, amd_names_shim, targets):
     actions.write(amd_names_shim, amd_names_shim_content)
 
 def _impl(ctx):
-    files = []
+    input_files = ctx.files.srcs + ctx.files.named_module_srcs
+    all_files = []
     typings = []
     js_files = []
+    named_module_files = []
+    include_npm_package_info = False
 
-    for src in ctx.files.srcs:
-        if src.is_source and not src.path.startswith("external/"):
-            dst = ctx.actions.declare_file(src.basename, sibling = src)
+    for idx, f in enumerate(input_files):
+        file = f
+
+        # copy files into bin if needed
+        if file.is_source and not file.path.startswith("external/"):
+            dst = ctx.actions.declare_file(file.basename, sibling = file)
             if ctx.attr.is_windows:
-                copy_cmd(ctx, src, dst)
-            else:
-                copy_bash(ctx, src, dst)
-            if dst.basename.endswith(".d.ts"):
-                typings.append(dst)
+                copy_cmd(ctx, file, dst)
             else:
-                files.append(dst)
-        elif src.basename.endswith(".d.ts"):
-            typings.append(src)
-        else:
-            files.append(src)
+                copy_bash(ctx, file, dst)
+
+            # re-assign file to the one now copied into the bin folder
+            file = dst
+
+        # register js files
+        if file.basename.endswith(".js") or file.basename.endswith(".js.map") or file.basename.endswith(".json"):
+            js_files.append(file)
+
+        # register typings
+        if (
+            (
+                file.path.endswith(".d.ts") or
+                file.path.endswith(".d.ts.map") or
+                # package.json may be required to resolve "typings" key
+                file.path.endswith("/package.json")
+            ) and
+            # exclude eg. external/npm/node_modules/protobufjs/node_modules/@types/node/index.d.ts
+            # these would be duplicates of the typings provided directly in another dependency.
+            # also exclude all /node_modules/typescript/lib/lib.*.d.ts files as these are determined by
+            # the tsconfig "lib" attribute
+            len(file.path.split("/node_modules/")) < 3 and file.path.find("/node_modules/typescript/lib/lib.") == -1
+        ):
+            typings.append(file)
 
-    for p in files:
-        if p.basename.endswith(".js") or p.basename.endswith(".js.map") or p.basename.endswith(".json"):
-            js_files.append(p)
+        # auto detect if it entirely an npm package
+        #
+        # NOTE: it probably can be removed once we support node_modules from more than
+        # a single workspace
+        if file.is_source and file.path.startswith("external/"):
+            # We cannot always expose the NpmPackageInfo as the linker
+            # only allow us to reference node modules from a single workspace at a time.
+            # Here we are automatically decide if we should or not including that provider
+            # by running through the sources and check if we have a src coming from an external
+            # workspace which indicates we should include the provider.
+            include_npm_package_info = True
 
-    files_depset = depset(files)
+        # ctx.files.named_module_srcs are merged after ctx.files.srcs
+        if idx >= len(ctx.files.srcs):
+            named_module_files.append(file)
+
+        # every single file on bin should be added here
+        all_files.append(file)
+
+    files_depset = depset(all_files)
+    js_files_depset = depset(js_files)
+    named_module_files_depset = depset(named_module_files)
+    typings_depset = depset(typings)
+
+    files_depsets = [files_depset]
+    npm_sources_depsets = [files_depset]
+    direct_sources_depsets = [files_depset]
+    direct_named_module_sources_depsets = [named_module_files_depset]
+    typings_depsets = [typings_depset]
+    js_files_depsets = [js_files_depset]
+
+    for dep in ctx.attr.deps:
+        if NpmPackageInfo in dep:
+            npm_sources_depsets.append(dep[NpmPackageInfo].sources)
+        else:
+            if JSModuleInfo in dep:
+                js_files_depsets.append(dep[JSModuleInfo].direct_sources)
+                direct_sources_depsets.append(dep[JSModuleInfo].direct_sources)
+            if JSNamedModuleInfo in dep:
+                direct_named_module_sources_depsets.append(dep[JSNamedModuleInfo].direct_sources)
+                direct_sources_depsets.append(dep[JSNamedModuleInfo].direct_sources)
+            if DeclarationInfo in dep:
+                typings_depsets.append(dep[DeclarationInfo].declarations)
+                direct_sources_depsets.append(dep[DeclarationInfo].declarations)
+            if DefaultInfo in dep:
+                files_depsets.append(dep[DefaultInfo].files)
 
     providers = [
         DefaultInfo(
-            files = files_depset,
-            runfiles = ctx.runfiles(files = ctx.files.srcs),
+            files = depset(transitive = files_depsets),
+            runfiles = ctx.runfiles(
+                files = all_files,
+                transitive_files = depset(transitive = files_depsets),
+            ),
         ),
         AmdNamesInfo(names = ctx.attr.amd_names),
-        js_module_info(depset(js_files)),
+        js_module_info(
+            sources = depset(transitive = js_files_depsets),
+            deps = ctx.attr.deps,
+        ),
+        js_named_module_info(
+            sources = depset(transitive = direct_named_module_sources_depsets),
+            deps = ctx.attr.deps,
+        ),
     ]
 
     if ctx.attr.package_name:
         path = "/".join([p for p in [ctx.bin_dir.path, ctx.label.workspace_root, ctx.label.package] if p])
         providers.append(LinkablePackageInfo(
             package_name = ctx.attr.package_name,
             path = path,
-            files = files_depset,
+            files = depset(transitive = direct_sources_depsets),
+        ))
+
+    if include_npm_package_info:
+        workspace_name = ctx.label.workspace_name if ctx.label.workspace_name else ctx.workspace_name
+        providers.append(NpmPackageInfo(
+            direct_sources = depset(transitive = direct_sources_depsets),
+            sources = depset(transitive = npm_sources_depsets),
+            workspace = workspace_name,
         ))
 
     # Don't provide DeclarationInfo if there are no typings to provide.
     # Improves error messaging downstream if DeclarationInfo is required.
     if len(typings):
-        providers.append(declaration_info(depset(typings)))
+        providers.append(declaration_info(
+            declarations = depset(transitive = typings_depsets),
+            deps = ctx.attr.deps,
+        ))
 
     return providers
 
 _js_library = rule(
     implementation = _impl,
     attrs = {
-        "amd_names": attr.string_dict(doc = _AMD_NAMES_DOC),
-        "is_windows": attr.bool(mandatory = True, doc = "Automatically set by macro"),
+        "amd_names": attr.string_dict(
+            doc = _AMD_NAMES_DOC,
+        ),
+        "deps": attr.label_list(
+            doc = """Transitive dependencies of the package.
+            It should include fine grained npm dependencies from the sources
+            or other targets we want to include in the library but also propagate their own deps.""",
+        ),
+        "is_windows": attr.bool(
+            doc = "Automatically set by macro",
+            mandatory = True,
+        ),
         # module_name for legacy ts_library module_mapping support
+        # which is still being used in a couple of tests
         # TODO: remove once legacy module_mapping is removed
-        "module_name": attr.string(),
-        "package_name": attr.string(),
+        "module_name": attr.string(
+            doc = "Internal use only. It will be removed soon.",
+        ),
+        "named_module_srcs": attr.label_list(
+            doc = """A subset of srcs that are javascript named-UMD or
+            named-AMD for use in rules such as ts_devserver.
+            They will be copied into the package bin folder if needed.""",
+            allow_files = True,
+        ),
+        "package_name": attr.string(
+            doc = """Optional package_name that this package may be imported as.""",
+        ),
         "srcs": attr.label_list(
+            doc = """The list of files that comprise the package.
+            They will be copied into the package bin folder if needed.""",
             allow_files = True,
-            mandatory = True,
         ),
     },
+    doc = "Defines a js_library package",
 )
 
 def js_library(
         name,
-        srcs,
+        srcs = [],
         amd_names = {},
         package_name = None,
+        deps = [],
+        named_module_srcs = [],
         **kwargs):
-    """Internal use only. May be published to the public API in a future release."""
+    """Internal use only yet. It will be released into a public API in a future release."""
     module_name = kwargs.pop("module_name", None)
     if module_name:
         fail("use package_name instead of module_name in target //%s:%s" % (native.package_name(), name))
     if kwargs.pop("is_windows", None):
         fail("is_windows is set by the js_library macro and should not be set explicitely")
     _js_library(
         name = name,
-        srcs = srcs,
         amd_names = amd_names,
+        srcs = srcs,
+        named_module_srcs = named_module_srcs,
+        deps = deps,
         package_name = package_name,
         # module_name for legacy ts_library module_mapping support
+        # which is still being used in a couple of tests
         # TODO: remove once legacy module_mapping is removed
         module_name = package_name,
         is_windows = select({

internal/node/test/lib2/BUILD.bazel

@@ -13,8 +13,10 @@ js_library(
     name = "lib2",
     package_name = "lib2",
     srcs = [
-        "main.js",
         "package.json",
         "src/some.js",
     ],
+    deps = [
+        ":tsconfig",
+    ],
 )

internal/npm_install/generate_build_file.ts

@@ -17,7 +17,7 @@
 /**
  * @fileoverview This script generates BUILD.bazel files by analyzing
  * the node_modules folder layed out by yarn or npm. It generates
- * fine grained Bazel `node_module_library` targets for each root npm package
+ * fine grained Bazel `js_library` targets for each root npm package
  * and all files for that package and its transitive deps are included
  * in the target. For example, `@<workspace>//jasmine` would
  * include all files in the jasmine npm package and all of its
@@ -28,7 +28,7 @@
  * target will be generated for the `jasmine` binary in the `jasmine`
  * npm package.
  *
- * Additionally, a `@<workspace>//:node_modules` `node_module_library`
+ * Additionally, a `@<workspace>//:node_modules` `js_library`
  * is generated that includes all packages under node_modules
  * as well as the .bin folder.
  *
@@ -158,16 +158,16 @@ function generateRootBuildFile(pkgs: Dep[]) {
                })});
 
   let buildFile = BUILD_FILE_HEADER +
-      `load("@build_bazel_rules_nodejs//internal/npm_install:node_module_library.bzl", "node_module_library")
+      `load("@build_bazel_rules_nodejs//internal/js_library:js_library.bzl", "js_library")
 
 exports_files([
 ${exportsStarlark}])
 
-# The node_modules directory in one catch-all node_module_library.
+# The node_modules directory in one catch-all js_library.
 # NB: Using this target may have bad performance implications if
 # there are many files in target.
 # See https://github.com/bazelbuild/bazel/issues/5153.
-node_module_library(
+js_library(
     name = "node_modules",${pkgFilesStarlark}${depsStarlark}
 )
 
@@ -861,7 +861,7 @@ function findFile(pkg: Dep, m: string) {
 }
 
 /**
- * Given a pkg, return the skylark `node_module_library` targets for the package.
+ * Given a pkg, return the skylark `js_library` targets for the package.
  */
 function printPackage(pkg: Dep) {
   function starlarkFiles(attr: string, files: string[], comment: string = '') {
@@ -919,8 +919,7 @@ function printPackage(pkg: Dep) {
   const depsStarlark =
       deps.map(dep => `"//${dep._dir}:${dep._name}__contents",`).join('\n        ');
 
-  let result =
-      `load("@build_bazel_rules_nodejs//internal/npm_install:node_module_library.bzl", "node_module_library")
+  let result = `load("@build_bazel_rules_nodejs//internal/js_library:js_library.bzl", "js_library")
 
 # Generated targets for npm package "${pkg._dir}"
 ${printJson(pkg)}
@@ -955,7 +954,7 @@ filegroup(
 )
 
 # The primary target for this package for use in rule deps
-node_module_library(
+js_library(
     name = "${pkg._name}",
     # direct sources listed for strict deps support
     srcs = [":${pkg._name}__files"],
@@ -967,14 +966,14 @@ node_module_library(
 )
 
 # Target is used as dep for main targets to prevent circular dependencies errors
-node_module_library(
+js_library(
     name = "${pkg._name}__contents",
     srcs = [":${pkg._name}__files", ":${pkg._name}__nested_node_modules"],${namedSourcesStarlark}
     visibility = ["//:__subpackages__"],
 )
 
 # Typings files that are part of the npm package not including nested node_modules
-node_module_library(
+js_library(
     name = "${pkg._name}__typings",${dtsStarlark}
 )
 
@@ -1137,7 +1136,7 @@ type Dep = {
 }
 
 /**
- * Given a scope, return the skylark `node_module_library` target for the scope.
+ * Given a scope, return the skylark `js_library` target for the scope.
  */
 function printScope(scope: string, pkgs: Dep[]) {
   pkgs = pkgs.filter(pkg => !pkg._isNested && pkg._dir.startsWith(`${scope}/`));
@@ -1168,10 +1167,10 @@ function printScope(scope: string, pkgs: Dep[]) {
     ],`;
   }
 
-  return `load("@build_bazel_rules_nodejs//internal/npm_install:node_module_library.bzl", "node_module_library")
+  return `load("@build_bazel_rules_nodejs//internal/js_library:js_library.bzl", "js_library")
 
 # Generated target for npm scope ${scope}
-node_module_library(
+js_library(
     name = "${scope}",${pkgFilesStarlark}${depsStarlark}
 )