feat(typescript): add JSNamedModuleInfo provider to ts_library outputs

bazel-contrib · Oct 4, 2019 · 8b43e6c · 8b43e6c
1 parent 85b1cd1
commit 8b43e6c
Show file tree

Hide file tree

Showing 28 changed files with 349 additions and 319 deletions.
diff --git a/WORKSPACE b/WORKSPACE
@@ -67,11 +67,10 @@ yarn_install(
     name = "npm",
     # 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 NodeModuleInfo and NodeModuleSources
-    # but that rule is not yet in the public API and we have not yet dropped support
-    # for filegroup based node_modules target.
+    # 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 NodeModuleInfo but that rule is not yet in the public API and we
+    # have not yet dropped support for filegroup based node_modules target.
     manual_build_file_contents = """
 filegroup(
   name = "node_modules_filegroup",

diff --git a/internal/common/devmode_js_sources.bzl b/internal/common/devmode_js_sources.bzl
@@ -17,7 +17,7 @@
 Outputs a manifest file with the sources listed.
 """
 
-load(":sources_aspect.bzl", "sources_aspect")
+load("@build_bazel_rules_nodejs//:providers.bzl", "JSNamedModuleInfo")
 
 # Avoid using non-normalized paths (workspace/../other_workspace/path)
 def _to_manifest_path(ctx, file):
@@ -27,26 +27,30 @@ def _to_manifest_path(ctx, file):
         return ctx.workspace_name + "/" + file.short_path
 
 def _devmode_js_sources_impl(ctx):
-    files = depset()
-
-    for d in ctx.attr.deps:
-        if hasattr(d, "node_sources"):
-            files = depset(transitive = [files, d.node_sources])
-        elif hasattr(d, "files"):
-            files = depset(transitive = [files, d.files])
+    sources_depsets = []
+    for dep in ctx.attr.deps:
+        if JSNamedModuleInfo in dep:
+            sources_depsets.append(dep[JSNamedModuleInfo].sources)
+        if hasattr(dep, "files"):
+            sources_depsets.append(dep.files)
+    sources = depset(transitive = sources_depsets)
 
     ctx.actions.write(ctx.outputs.manifest, "".join([
         _to_manifest_path(ctx, f) + "\n"
-        for f in files.to_list()
+        for f in sources.to_list()
+        if f.path.endswith(".js") or f.path.endswith(".mjs")
     ]))
-    return [DefaultInfo(files = files)]
+
+    return [DefaultInfo(
+        files = sources,
+        runfiles = ctx.runfiles(transitive_files = sources),
+    )]
 
 devmode_js_sources = rule(
     implementation = _devmode_js_sources_impl,
     attrs = {
         "deps": attr.label_list(
             allow_files = True,
-            aspects = [sources_aspect],
         ),
     },
     outputs = {

diff --git a/internal/common/node_module_info.bzl b/internal/common/node_module_info.bzl
@@ -12,63 +12,40 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-"""NodeModuleInfo & NodeModuleSources providers and apsect to collect node_modules from deps.
+"""NodeModuleInfo providers and apsect to collect node_modules from deps.
 """
 
-# NodeModuleInfo provider is only provided by targets that are npm dependencies by the
-# `node_module_library` rule. This provider is currently used by different rules to filter out
-# npm dependencies such as
-# ```
-# [d for d in ctx.attr.deps if not NodeModuleInfo in d]
-# ```
-# in `packages/typescript/internal/build_defs.bzl` or
-# ```
-# hasattr(target, "files") and not NodeModuleInfo in target:
-# ```
-# in `internal/common/sources_aspect.bzl`.
-# Similar filtering is done in downstream repositories such as angular/angular so this provider
-# needs to go through a deprecation period before it can be phased out.
-NodeModuleInfo = provider(
-    doc = "Provides information about npm dependencies installed with yarn_install and npm_install rules",
-    fields = {
-        "workspace": "The workspace name that the npm dependencies are provided from",
-    },
-)
-
-# NodeModuleSources provider is provided by targets that are npm dependencies by the
+# NodeModuleInfo provider is provided by targets that are npm dependencies by the
 # `node_module_library` rule as well as other targets that have direct or transitive deps on
-# `node_module_library` targets via the `collect_node_modules_aspect` below.
-# TODO: rename to NodeModuleSourcesInfo so name doesn't trigger name-conventions warning
-# buildozer: disable=name-conventions
-NodeModuleSources = provider(
-    doc = "Provides sources for npm dependencies installed with yarn_install and npm_install rules",
+# `node_module_library` targets via the `node_modules_aspect` below.
+NodeModuleInfo = provider(
+    doc = "Provides information about npm dependencies",
     fields = {
-        "scripts": "Source files that are javascript named-UMD or named-AMD modules for use in rules such as ts_devserver",
-        "sources": "Source files that are npm dependencies",
+        "sources": "Source files that are direct & transitive npm depedendencies",
         "workspace": "The workspace name that the npm dependencies are provided from",
     },
 )
 
-def _collect_node_modules_aspect_impl(target, ctx):
-    nm_wksp = None
-
-    if NodeModuleSources in target:
-        return []
+def _node_modules_aspect_impl(target, ctx):
+    providers = []
 
-    if hasattr(ctx.rule.attr, "deps"):
+    # provide NodeModuleInfo if it is not already provided there are NodeModuleInfo deps
+    if not NodeModuleInfo in target:
         sources = depset()
-        for dep in ctx.rule.attr.deps:
-            if NodeModuleSources in dep:
-                if nm_wksp and dep[NodeModuleSources].workspace != nm_wksp:
-                    fail("All npm dependencies need to come from a single workspace. Found '%s' and '%s'." % (nm_wksp, dep[NodeModuleSources].workspace))
-                nm_wksp = dep[NodeModuleSources].workspace
-                sources = depset(transitive = [dep[NodeModuleSources].sources, sources])
-        if sources:
-            return [NodeModuleSources(sources = sources, workspace = nm_wksp)]
-
-    return []
-
-collect_node_modules_aspect = aspect(
-    implementation = _collect_node_modules_aspect_impl,
+        nm_wksp = None
+        if hasattr(ctx.rule.attr, "deps"):
+            for dep in ctx.rule.attr.deps:
+                if NodeModuleInfo in dep:
+                    if nm_wksp and dep[NodeModuleInfo].workspace != nm_wksp:
+                        fail("All npm dependencies need to come from a single workspace. Found '%s' and '%s'." % (nm_wksp, dep[NodeModuleInfo].workspace))
+                    nm_wksp = dep[NodeModuleInfo].workspace
+                    sources = depset(transitive = [dep[NodeModuleInfo].sources, sources])
+            if nm_wksp:
+                providers.extend([NodeModuleInfo(sources = sources, workspace = nm_wksp)])
+
+    return providers
+
+node_modules_aspect = aspect(
+    _node_modules_aspect_impl,
     attr_aspects = ["deps"],
 )
diff --git a/internal/common/sources_aspect.bzl b/internal/common/sources_aspect.bzl
diff --git a/internal/linker/link_node_modules.bzl b/internal/linker/link_node_modules.bzl
@@ -10,7 +10,7 @@ linker, which uses the mappings to link a node_modules directory for
 runtimes to locate all the first-party packages.
 """
 
-load("@build_bazel_rules_nodejs//internal/common:node_module_info.bzl", "NodeModuleSources")
+load("@build_bazel_rules_nodejs//internal/common:node_module_info.bzl", "NodeModuleInfo")
 
 def _debug(vars, *args):
     if "VERBOSE_LOGS" in vars.keys():
@@ -42,8 +42,8 @@ def register_node_modules_linker(ctx, args, inputs):
     # Look through data/deps attributes to find...
     for dep in getattr(ctx.attr, "data", []) + getattr(ctx.attr, "deps", []):
         # ...the root directory for the third-party node_modules; we'll symlink the local "node_modules" to it
-        if NodeModuleSources in dep:
-            possible_root = "/".join([dep[NodeModuleSources].workspace, "node_modules"])
+        if NodeModuleInfo in dep:
+            possible_root = "/".join([dep[NodeModuleInfo].workspace, "node_modules"])
             if not node_modules_root:
                 node_modules_root = possible_root
             elif node_modules_root != possible_root:

diff --git a/internal/node.bzl b/internal/node.bzl
@@ -23,7 +23,5 @@ load(
     "//internal/common:expand_into_runfiles.bzl",
     _expand_location_into_runfiles = "expand_location_into_runfiles",
 )
-load("//internal/common:sources_aspect.bzl", _sources_aspect = "sources_aspect")
 
-sources_aspect = _sources_aspect
 expand_location_into_runfiles = _expand_location_into_runfiles
diff --git a/internal/node/node.bzl b/internal/node/node.bzl
@@ -20,10 +20,11 @@ They support module mapping: any targets in the transitive dependencies with
 a `module_name` attribute can be `require`d by that name.
 """
 
-load("@build_bazel_rules_nodejs//internal/common:node_module_info.bzl", "NodeModuleSources", "collect_node_modules_aspect")
+load("@build_bazel_rules_nodejs//:declaration_provider.bzl", "DeclarationInfo")
+load("@build_bazel_rules_nodejs//:providers.bzl", "JSNamedModuleInfo")
+load("@build_bazel_rules_nodejs//internal/common:node_module_info.bzl", "NodeModuleInfo", "node_modules_aspect")
 load("//internal/common:expand_into_runfiles.bzl", "expand_location_into_runfiles")
 load("//internal/common:module_mappings.bzl", "module_mappings_runtime_aspect")
-load("//internal/common:sources_aspect.bzl", "sources_aspect")
 load("//internal/common:windows_utils.bzl", "create_windows_native_launcher_script", "is_windows")
 
 def _trim_package_node_modules(package_name):
@@ -48,8 +49,8 @@ def _compute_node_modules_root(ctx):
     """
     node_modules_root = None
     if ctx.attr.node_modules:
-        if NodeModuleSources in ctx.attr.node_modules:
-            node_modules_root = "/".join([ctx.attr.node_modules[NodeModuleSources].workspace, "node_modules"])
+        if NodeModuleInfo in ctx.attr.node_modules:
+            node_modules_root = "/".join([ctx.attr.node_modules[NodeModuleInfo].workspace, "node_modules"])
         elif ctx.files.node_modules:
             # ctx.files.node_modules is not an empty list
             workspace = ctx.attr.node_modules.label.workspace_root.split("/")[1] if ctx.attr.node_modules.label.workspace_root else ctx.workspace_name
@@ -59,8 +60,8 @@ def _compute_node_modules_root(ctx):
                 "node_modules",
             ] if f])
     for d in ctx.attr.data:
-        if NodeModuleSources in d:
-            possible_root = "/".join([d[NodeModuleSources].workspace, "node_modules"])
+        if NodeModuleInfo in d:
+            possible_root = "/".join([d[NodeModuleInfo].workspace, "node_modules"])
             if not node_modules_root:
                 node_modules_root = possible_root
             elif node_modules_root != possible_root:
@@ -130,19 +131,23 @@ def _nodejs_binary_impl(ctx):
     node_modules = depset(ctx.files.node_modules)
 
     # Also include files from npm fine grained deps as inputs.
-    # These deps are identified by the NodeModuleSources provider.
+    # These deps are identified by the NodeModuleInfo provider.
     for d in ctx.attr.data:
-        if NodeModuleSources in d:
-            node_modules = depset(transitive = [node_modules, d[NodeModuleSources].sources])
+        if NodeModuleInfo in d:
+            node_modules = depset(transitive = [node_modules, d[NodeModuleInfo].sources])
 
     # Using a depset will allow us to avoid flattening files and sources
     # inside this loop. This should reduce the performances hits,
     # since we don't need to call .to_list()
     sources = depset()
 
     for d in ctx.attr.data:
-        if hasattr(d, "node_sources"):
-            sources = depset(transitive = [sources, d.node_sources])
+        if DeclarationInfo in d:
+            sources = depset(transitive = [sources, d[DeclarationInfo].transitive_declarations])
+
+        # TODO: switch to JSModuleInfo when it is available
+        if JSNamedModuleInfo in d:
+            sources = depset(transitive = [sources, d[JSNamedModuleInfo].sources])
         if hasattr(d, "files"):
             sources = depset(transitive = [sources, d.files])
 
@@ -262,7 +267,7 @@ _NODEJS_EXECUTABLE_ATTRS = {
     "data": attr.label_list(
         doc = """Runtime dependencies which may be loaded during execution.""",
         allow_files = True,
-        aspects = [sources_aspect, module_mappings_runtime_aspect, collect_node_modules_aspect],
+        aspects = [node_modules_aspect, module_mappings_runtime_aspect],
     ),
     "default_env_vars": attr.string_list(
         doc = """Default environment variables that are added to `configuration_env_vars`.

diff --git a/internal/node/npm_package_bin.bzl b/internal/node/npm_package_bin.bzl
@@ -1,13 +1,14 @@
 "A generic rule to run a tool that appears in node_modules/.bin"
 
+load("@build_bazel_rules_nodejs//internal/common:node_module_info.bzl", "NodeModuleInfo", "node_modules_aspect")
 load("@build_bazel_rules_nodejs//internal/linker:link_node_modules.bzl", "module_mappings_aspect", "register_node_modules_linker")
 
 # Note: this API is chosen to match nodejs_binary
 # so that we can generate macros that act as either an output-producing tool or an executable
 _ATTRS = {
     "outs": attr.output_list(),
     "args": attr.string_list(mandatory = True),
-    "data": attr.label_list(allow_files = True, aspects = [module_mappings_aspect]),
+    "data": attr.label_list(allow_files = True, aspects = [module_mappings_aspect, node_modules_aspect]),
     "output_dir": attr.bool(),
     "tool": attr.label(
         executable = True,
@@ -23,14 +24,23 @@ def _expand_location(ctx, s):
     s = s.replace("$@", "/".join([ctx.bin_dir.path, ctx.label.package, ctx.attr.name]))
     return ctx.expand_location(s, targets = ctx.attr.data)
 
+def _inputs(ctx):
+    # Also include files from npm fine grained deps as inputs.
+    # These deps are identified by the NodeModuleInfo provider.
+    inputs_depsets = []
+    for d in ctx.attr.data:
+        if NodeModuleInfo in d:
+            inputs_depsets.append(d[NodeModuleInfo].sources)
+    return depset(ctx.files.data, transitive = inputs_depsets).to_list()
+
 def _impl(ctx):
     if ctx.attr.output_dir and ctx.attr.outs:
         fail("Only one of output_dir and outs may be specified")
     if not ctx.attr.output_dir and not ctx.attr.outs:
         fail("One of output_dir and outs must be specified")
 
     args = ctx.actions.args()
-    inputs = ctx.files.data[:]
+    inputs = _inputs(ctx)
     outputs = []
     if ctx.attr.output_dir:
         outputs = [ctx.actions.declare_directory(ctx.attr.name)]

diff --git a/internal/npm_install/generate_build_file.js b/internal/npm_install/generate_build_file.js
diff --git a/internal/npm_install/generate_build_file.ts b/internal/npm_install/generate_build_file.ts
@@ -888,15 +888,15 @@ function printPackage(pkg: Dep) {
   const dtsSources = filterFiles(pkg._files, ['.d.ts']);
   // TODO(gmagolan): add UMD & AMD scripts to scripts even if not an APF package _but_ only if they
   // are named?
-  const scripts = getNgApfScripts(pkg);
+  const namedSources = getNgApfScripts(pkg);
   const deps = [pkg].concat(pkg._dependencies.filter(dep => dep !== pkg && !dep._isNested));
 
-  let scriptStarlark = '';
-  if (scripts.length) {
-    scriptStarlark = `
+  let namedSourcesStarlark = '';
+  if (namedSources.length) {
+    namedSourcesStarlark = `
     # subset of srcs that are javascript named-UMD or named-AMD scripts
-    scripts = [
-        ${scripts.map((f: string) => `"//:node_modules/${pkg._dir}/${f}",`).join('\n        ')}
+    named_sources = [
+        ${namedSources.map((f: string) => `"//:node_modules/${pkg._dir}/${f}",`).join('\n        ')}
     ],`;
   }
 
@@ -948,7 +948,7 @@ node_module_library(
 # circular dependencies errors
 node_module_library(
     name = "${pkg._name}__contents",
-    srcs = [":${pkg._name}__files"],${scriptStarlark}
+    srcs = [":${pkg._name}__files"],${namedSourcesStarlark}
 )
 
 # ${pkg._name}__typings is the subset of ${pkg._name}__contents that are declarations