Consolidate node module rules & providers & support transitive npm de…

…ps via collect_node_modules_aspect

BUILD.bazel

@@ -54,7 +54,6 @@ npm_package(
         "//internal/js_library:package_contents",
         # TODO(alexeagle): distribute separately as @bazel/rollup
         "//internal/rollup:package_contents",
-        "//internal/ng_apf_library:package_contents",
         "//internal/node:package_contents",
         "//internal/npm_install:package_contents",
         "//internal/npm_package:package_contents",

internal/common/node_module_info.bzl

@@ -12,22 +12,40 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-"""NodeModuleInfo provider and apsect to collect node_modules from deps.
+"""NodeModuleInfo & NodeModuleSources providers and apsect to collect node_modules from deps.
 """
 
 NodeModuleInfo = provider(
-    doc = "This provider contains information about npm dependencies installed with yarn_install and npm_install rules",
+    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(
+    doc = "Provides sources for npm dependencies installed with yarn_install and npm_install rules",
+    fields = {
+        "sources": "Source files that are npm dependencies",
+        "workspace": "The workspace name that the npm dependencies are provided from",
+    },
+)
+
 def _collect_node_modules_aspect_impl(target, ctx):
     nm_wksp = None
 
-    if hasattr(ctx.rule.attr, "tags") and "NODE_MODULE_MARKER" in ctx.rule.attr.tags:
-        nm_wksp = target.label.workspace_root.split("/")[1] if target.label.workspace_root else ctx.workspace_name
-        return [NodeModuleInfo(workspace = nm_wksp)]
+    if NodeModuleSources in target:
+        return []
+
+    if hasattr(ctx.rule.attr, "deps"):
+        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 []
 

internal/common/providers.bzl

@@ -12,6 +12,6 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-# Provider for "node-module-like" rules such as `ng_apf_library`.
+# Provider for "node-module-like" rules such as `node_module_library`.
 # It tells downstream rules how to consume Javascript files in the NPM package.
 ScriptsProvider = provider(fields = ["scripts"])

internal/common/sources_aspect.bzl

@@ -15,6 +15,7 @@
 """Aspect to collect es5 js sources and scripts from deps.
 """
 
+load("@build_bazel_rules_nodejs//internal/common:node_module_info.bzl", "NodeModuleInfo")
 load("@build_bazel_rules_nodejs//internal/common:providers.bzl", "ScriptsProvider")
 
 def _sources_aspect_impl(target, ctx):
@@ -26,7 +27,7 @@ def _sources_aspect_impl(target, ctx):
     node_sources = depset()
 
     # dev_scripts is a collection of "scripts" from "node-module-like" targets
-    # such as `ng_apf_library`. Note that nothing is collected from the default
+    # such as `node_module_library`. Note that nothing is collected from the default
     # filegroup target for generic node modules because it does not have the
     # `scripts` provider nor does it have the `deps` attribute.
     dev_scripts = depset()
@@ -37,9 +38,8 @@ def _sources_aspect_impl(target, ctx):
         node_sources = depset(transitive = [node_sources, target.typescript.es5_sources])
     elif ScriptsProvider in target:
         dev_scripts = depset(transitive = [dev_scripts, target[ScriptsProvider].scripts])
-    elif hasattr(target, "files") and "NODE_MODULE_MARKER" not in ctx.rule.attr.tags:
-        # Sources from npm fine grained deps which are tagged with NODE_MODULE_MARKER
-        # should not be included
+    elif hasattr(target, "files") and not NodeModuleInfo in target:
+        # Sources from npm fine grained deps should not be included
         node_sources = depset(
             [f for f in target.files if f.path.endswith(".js")],
             transitive = [node_sources],

internal/node/node.bzl

@@ -20,9 +20,9 @@ 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("//internal/common:expand_into_runfiles.bzl", "expand_location_into_runfiles")
 load("//internal/common:module_mappings.bzl", "module_mappings_runtime_aspect")
-load("//internal/common:node_module_info.bzl", "NodeModuleInfo", "collect_node_modules_aspect")
 load("//internal/common:sources_aspect.bzl", "sources_aspect")
 
 def _trim_package_node_modules(package_name):
@@ -36,18 +36,15 @@ def _trim_package_node_modules(package_name):
         segments += [n]
     return "/".join(segments)
 
-def _write_loader_script(ctx):
-    # Generates the JavaScript snippet of module roots mappings, with each entry
-    # in the form:
-    #   {module_name: /^mod_name\b/, module_root: 'path/to/mod_name'}
-    module_mappings = []
-    for d in ctx.attr.data:
-        if hasattr(d, "runfiles_module_mappings"):
-            for [mn, mr] in d.runfiles_module_mappings.items():
-                escaped = mn.replace("/", "\/").replace(".", "\.")
-                mapping = "{module_name: /^%s\\b/, module_root: '%s'}" % (escaped, mr)
-                module_mappings.append(mapping)
+def _compute_node_modules_root(ctx):
+    """Computes the node_modules root from the node_modules and deps attributes.
 
+    Args:
+      ctx: the skylark execution context
+
+    Returns:
+      The node_modules root as a string
+    """
     node_modules_root = None
     if ctx.files.node_modules:
         # ctx.files.node_modules is not an empty list
@@ -58,8 +55,8 @@ def _write_loader_script(ctx):
             "node_modules",
         ] if f])
     for d in ctx.attr.data:
-        if NodeModuleInfo in d:
-            possible_root = "/".join([d[NodeModuleInfo].workspace, "node_modules"])
+        if NodeModuleSources in d:
+            possible_root = "/".join([d[NodeModuleSources].workspace, "node_modules"])
             if not node_modules_root:
                 node_modules_root = possible_root
             elif node_modules_root != possible_root:
@@ -73,6 +70,21 @@ def _write_loader_script(ctx):
             ctx.attr.node_modules.label.package,
             "node_modules",
         ] if f])
+    return node_modules_root
+
+def _write_loader_script(ctx):
+    # Generates the JavaScript snippet of module roots mappings, with each entry
+    # in the form:
+    #   {module_name: /^mod_name\b/, module_root: 'path/to/mod_name'}
+    module_mappings = []
+    for d in ctx.attr.data:
+        if hasattr(d, "runfiles_module_mappings"):
+            for [mn, mr] in d.runfiles_module_mappings.items():
+                escaped = mn.replace("/", "\/").replace(".", "\.")
+                mapping = "{module_name: /^%s\\b/, module_root: '%s'}" % (escaped, mr)
+                module_mappings.append(mapping)
+
+    node_modules_root = _compute_node_modules_root(ctx)
 
     ctx.actions.expand_template(
         template = ctx.file._loader_template,
@@ -101,7 +113,13 @@ def _short_path_to_manifest_path(ctx, short_path):
 
 def _nodejs_binary_impl(ctx):
     node = ctx.file.node
-    node_modules = ctx.files.node_modules
+    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.
+    for d in ctx.attr.data:
+        if NodeModuleSources in d:
+            node_modules = depset(transitive = [node_modules, d[NodeModuleSources].sources])
 
     # Using a depset will allow us to avoid flattening files and sources
     # inside this loop. This should reduce the performances hits,
@@ -151,7 +169,7 @@ def _nodejs_binary_impl(ctx):
         is_executable = True,
     )
 
-    runfiles = depset([node, ctx.outputs.loader, ctx.file._repository_args] + node_modules + ctx.files._node_runfiles, transitive = [sources])
+    runfiles = depset([node, ctx.outputs.loader, ctx.file._repository_args] + ctx.files._node_runfiles, transitive = [sources, node_modules])
 
     return [DefaultInfo(
         executable = ctx.outputs.script,
@@ -160,13 +178,13 @@ def _nodejs_binary_impl(ctx):
             files = [
                         node,
                         ctx.outputs.loader,
-                    ] + ctx.files._source_map_support_files + node_modules +
+                    ] + ctx.files._source_map_support_files +
 
                     # We need this call to the list of Files.
                     # Calling the .to_list() method may have some perfs hits,
                     # so we should be running this method only once per rule.
                     # see: https://docs.bazel.build/versions/master/skylark/depsets.html#performance
-                    sources.to_list(),
+                    node_modules.to_list() + sources.to_list(),
             collect_data = True,
         ),
     )]

internal/npm_install/BUILD.bazel

@@ -13,7 +13,6 @@ filegroup(
     name = "generate_build_file",
     srcs = [
         "generate_build_file.js",
-        "//internal/ng_apf_library:ng_apf_library.js",
     ],
     visibility = ["//internal:__subpackages__"],
 )