fix: @npm//foobar:foobar__files target no longer includes nested node…

…_modules (#1390) * fix: @npm//foobar:foobar__files target no longer includes nested node_modules * refactor: make npm targets `foo__nested_node_modules`, `foo__all_files` & `foo__contents` private
bazel-contrib · Nov 26, 2019 · a13f2b6 · a13f2b6
1 parent bfb4b10
commit a13f2b6
Show file tree

Hide file tree

Showing 12 changed files with 205 additions and 75 deletions.
diff --git a/internal/npm_install/generate_build_file.js b/internal/npm_install/generate_build_file.js
@@ -172,10 +172,10 @@ for installation instructions.`);
 `;
             });
         });
-        let srcsStarlark = '';
+        let filesStarlark = '';
         if (pkgs.length) {
-            const list = pkgs.map(pkg => `"//${pkg._dir}:${pkg._name}__files",`).join('\n        ');
-            srcsStarlark = `
+            const list = pkgs.map(pkg => `"//${pkg._dir}:${pkg._name}__all_files",`).join('\n        ');
+            filesStarlark = `
     # direct sources listed for strict deps support
     srcs = [
         ${list}
@@ -201,7 +201,7 @@ ${exportsStarlark}])
 # there are many files in target.
 # See https://github.com/bazelbuild/bazel/issues/5153.
 node_module_library(
-    name = "node_modules",${srcsStarlark}${depsStarlark}
+    name = "node_modules",${filesStarlark}${depsStarlark}
 )
 
 `;
@@ -775,6 +775,8 @@ def _maybe(repo_rule, name, **kwargs):
      */
     function printPackage(pkg) {
         const sources = filterFiles(pkg._files, INCLUDED_FILES);
+        const files = sources.filter((f) => !f.startsWith('node_modules/'));
+        const nestedNodeModules = sources.filter((f) => f.startsWith('node_modules/'));
         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?
@@ -788,12 +790,21 @@ def _maybe(repo_rule, name, **kwargs):
         ${namedSources.map((f) => `"//:node_modules/${pkg._dir}/${f}",`).join('\n        ')}
     ],`;
         }
-        let srcsStarlark = '';
-        if (sources.length) {
-            srcsStarlark = `
+        let filesStarlark = '';
+        if (files.length) {
+            filesStarlark = `
     # ${pkg._dir} package files (and files in nested node_modules)
     srcs = [
-        ${sources.map((f) => `"//:node_modules/${pkg._dir}/${f}",`).join('\n        ')}
+        ${files.map((f) => `"//:node_modules/${pkg._dir}/${f}",`).join('\n        ')}
+    ],`;
+        }
+        let nestedNodeModulesStarlark = '';
+        if (nestedNodeModules.length) {
+            nestedNodeModulesStarlark = `
+    # ${pkg._dir} package files (and files in nested node_modules)
+    srcs = [
+        ${nestedNodeModules.map((f) => `"//:node_modules/${pkg._dir}/${f}",`)
+                .join('\n        ')}
     ],`;
         }
         let depsStarlark = '';
@@ -819,20 +830,32 @@ def _maybe(repo_rule, name, **kwargs):
 ${printJson(pkg)}
 
 filegroup(
-    name = "${pkg._name}__files",${srcsStarlark}
+    name = "${pkg._name}__files",${filesStarlark}
+)
+
+filegroup(
+    name = "${pkg._name}__nested_node_modules",${nestedNodeModulesStarlark}
+    visibility = ["//visibility:private"],
+)
+
+filegroup(
+    name = "${pkg._name}__all_files",
+    srcs = [":${pkg._name}__files", ":${pkg._name}__nested_node_modules"],
+    visibility = ["//:__subpackages__"],
 )
 
 node_module_library(
     name = "${pkg._name}",
     # direct sources listed for strict deps support
-    srcs = [":${pkg._name}__files"],${depsStarlark}
+    srcs = [":${pkg._name}__all_files"],${depsStarlark}
 )
 
 # ${pkg._name}__contents target is used as dep for main targets to prevent
 # circular dependencies errors
 node_module_library(
     name = "${pkg._name}__contents",
-    srcs = [":${pkg._name}__files"],${namedSourcesStarlark}
+    srcs = [":${pkg._name}__all_files"],${namedSourcesStarlark}
+    visibility = ["//:__subpackages__"],
 )
 
 # ${pkg._name}__typings is the subset of ${pkg._name}__contents that are declarations
@@ -987,10 +1010,10 @@ def ${name.replace(/-/g, '_')}_test(**kwargs):
         });
         // filter out duplicate deps
         deps = [...pkgs, ...new Set(deps)];
-        let srcsStarlark = '';
+        let filesStarlark = '';
         if (deps.length) {
-            const list = deps.map(dep => `"//${dep._dir}:${dep._name}__files",`).join('\n        ');
-            srcsStarlark = `
+            const list = deps.map(dep => `"//${dep._dir}:${dep._name}__all_files",`).join('\n        ');
+            filesStarlark = `
     # direct sources listed for strict deps support
     srcs = [
         ${list}
@@ -1009,7 +1032,7 @@ def ${name.replace(/-/g, '_')}_test(**kwargs):
 
 # Generated target for npm scope ${scope}
 node_module_library(
-    name = "${scope}",${srcsStarlark}${depsStarlark}
+    name = "${scope}",${filesStarlark}${depsStarlark}
 )
 
 `;

diff --git a/internal/npm_install/generate_build_file.ts b/internal/npm_install/generate_build_file.ts
@@ -176,10 +176,10 @@ function generateRootBuildFile(pkgs: Dep[]) {
 `;
                })});
 
-  let srcsStarlark = '';
+  let filesStarlark = '';
   if (pkgs.length) {
-    const list = pkgs.map(pkg => `"//${pkg._dir}:${pkg._name}__files",`).join('\n        ');
-    srcsStarlark = `
+    const list = pkgs.map(pkg => `"//${pkg._dir}:${pkg._name}__all_files",`).join('\n        ');
+    filesStarlark = `
     # direct sources listed for strict deps support
     srcs = [
         ${list}
@@ -207,7 +207,7 @@ ${exportsStarlark}])
 # there are many files in target.
 # See https://github.com/bazelbuild/bazel/issues/5153.
 node_module_library(
-    name = "node_modules",${srcsStarlark}${depsStarlark}
+    name = "node_modules",${filesStarlark}${depsStarlark}
 )
 
 `
@@ -859,6 +859,8 @@ function findFile(pkg: Dep, m: string) {
  */
 function printPackage(pkg: Dep) {
   const sources = filterFiles(pkg._files, INCLUDED_FILES);
+  const files = sources.filter((f: string) => !f.startsWith('node_modules/'));
+  const nestedNodeModules = sources.filter((f: string) => f.startsWith('node_modules/'));
   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?
@@ -874,12 +876,23 @@ function printPackage(pkg: Dep) {
     ],`;
   }
 
-  let srcsStarlark = '';
-  if (sources.length) {
-    srcsStarlark = `
+  let filesStarlark = '';
+  if (files.length) {
+    filesStarlark = `
     # ${pkg._dir} package files (and files in nested node_modules)
     srcs = [
-        ${sources.map((f: string) => `"//:node_modules/${pkg._dir}/${f}",`).join('\n        ')}
+        ${files.map((f: string) => `"//:node_modules/${pkg._dir}/${f}",`).join('\n        ')}
+    ],`;
+  }
+
+  let nestedNodeModulesStarlark = '';
+  if (nestedNodeModules.length) {
+    nestedNodeModulesStarlark = `
+    # ${pkg._dir} package files (and files in nested node_modules)
+    srcs = [
+        ${
+        nestedNodeModules.map((f: string) => `"//:node_modules/${pkg._dir}/${f}",`)
+            .join('\n        ')}
     ],`;
   }
 
@@ -909,20 +922,32 @@ function printPackage(pkg: Dep) {
 ${printJson(pkg)}
 
 filegroup(
-    name = "${pkg._name}__files",${srcsStarlark}
+    name = "${pkg._name}__files",${filesStarlark}
+)
+
+filegroup(
+    name = "${pkg._name}__nested_node_modules",${nestedNodeModulesStarlark}
+    visibility = ["//visibility:private"],
+)
+
+filegroup(
+    name = "${pkg._name}__all_files",
+    srcs = [":${pkg._name}__files", ":${pkg._name}__nested_node_modules"],
+    visibility = ["//:__subpackages__"],
 )
 
 node_module_library(
     name = "${pkg._name}",
     # direct sources listed for strict deps support
-    srcs = [":${pkg._name}__files"],${depsStarlark}
+    srcs = [":${pkg._name}__all_files"],${depsStarlark}
 )
 
 # ${pkg._name}__contents target is used as dep for main targets to prevent
 # circular dependencies errors
 node_module_library(
     name = "${pkg._name}__contents",
-    srcs = [":${pkg._name}__files"],${namedSourcesStarlark}
+    srcs = [":${pkg._name}__all_files"],${namedSourcesStarlark}
+    visibility = ["//:__subpackages__"],
 )
 
 # ${pkg._name}__typings is the subset of ${pkg._name}__contents that are declarations
@@ -1099,10 +1124,10 @@ function printScope(scope: string, pkgs: Dep[]) {
   // filter out duplicate deps
   deps = [...pkgs, ...new Set(deps)];
 
-  let srcsStarlark = '';
+  let filesStarlark = '';
   if (deps.length) {
-    const list = deps.map(dep => `"//${dep._dir}:${dep._name}__files",`).join('\n        ');
-    srcsStarlark = `
+    const list = deps.map(dep => `"//${dep._dir}:${dep._name}__all_files",`).join('\n        ');
+    filesStarlark = `
     # direct sources listed for strict deps support
     srcs = [
         ${list}
@@ -1123,7 +1148,7 @@ function printScope(scope: string, pkgs: Dep[]) {
 
 # Generated target for npm scope ${scope}
 node_module_library(
-    name = "${scope}",${srcsStarlark}${depsStarlark}
+    name = "${scope}",${filesStarlark}${depsStarlark}
 )
 
 `;

diff --git a/internal/npm_install/test/golden/@angular/core/BUILD.bazel.golden b/internal/npm_install/test/golden/@angular/core/BUILD.bazel.golden
@@ -664,9 +664,18 @@ filegroup(
         "//:node_modules/@angular/core/testing/testing.metadata.json",
     ],
 )
+filegroup(
+    name = "core__nested_node_modules",
+    visibility = ["//visibility:private"],
+)
+filegroup(
+    name = "core__all_files",
+    srcs = [":core__files", ":core__nested_node_modules"],
+    visibility = ["//:__subpackages__"],
+)
 node_module_library(
     name = "core",
-    srcs = [":core__files"],
+    srcs = [":core__all_files"],
     deps = [
         "//@angular/core:core__contents",
         "//tslib:tslib__contents",
@@ -676,11 +685,12 @@ node_module_library(
 )
 node_module_library(
     name = "core__contents",
-    srcs = [":core__files"],
+    srcs = [":core__all_files"],
     named_module_srcs = [
         "//:node_modules/@angular/core/bundles/core-testing.umd.js",
         "//:node_modules/@angular/core/bundles/core.umd.js",
     ],
+    visibility = ["//:__subpackages__"],
 )
 node_module_library(
     name = "core__typings",

diff --git a/internal/npm_install/test/golden/@gregmagolan/BUILD.bazel.golden b/internal/npm_install/test/golden/@gregmagolan/BUILD.bazel.golden
@@ -4,8 +4,8 @@ load("@build_bazel_rules_nodejs//internal/npm_install:node_module_library.bzl",
 node_module_library(
     name = "@gregmagolan",
     srcs = [
-        "//@gregmagolan/test-a:test-a__files",
-        "//@gregmagolan/test-b:test-b__files",
+        "//@gregmagolan/test-a:test-a__all_files",
+        "//@gregmagolan/test-b:test-b__all_files",
     ],
     deps = [
         "//@gregmagolan/test-a:test-a__contents",

diff --git a/internal/npm_install/test/golden/@gregmagolan/test-a/BUILD.bazel.golden b/internal/npm_install/test/golden/@gregmagolan/test-a/BUILD.bazel.golden
@@ -9,16 +9,26 @@ filegroup(
         "//:node_modules/@gregmagolan/test-a/package.json",
     ],
 )
+filegroup(
+    name = "test-a__nested_node_modules",
+    visibility = ["//visibility:private"],
+)
+filegroup(
+    name = "test-a__all_files",
+    srcs = [":test-a__files", ":test-a__nested_node_modules"],
+    visibility = ["//:__subpackages__"],
+)
 node_module_library(
     name = "test-a",
-    srcs = [":test-a__files"],
+    srcs = [":test-a__all_files"],
     deps = [
         "//@gregmagolan/test-a:test-a__contents",
     ],
 )
 node_module_library(
     name = "test-a__contents",
-    srcs = [":test-a__files"],
+    srcs = [":test-a__all_files"],
+    visibility = ["//:__subpackages__"],
 )
 node_module_library(
     name = "test-a__typings",

diff --git a/internal/npm_install/test/golden/@gregmagolan/test-b/BUILD.bazel.golden b/internal/npm_install/test/golden/@gregmagolan/test-b/BUILD.bazel.golden
@@ -5,21 +5,33 @@ filegroup(
     name = "test-b__files",
     srcs = [
         "//:node_modules/@gregmagolan/test-b/main.js",
+        "//:node_modules/@gregmagolan/test-b/package.json",
+    ],
+)
+filegroup(
+    name = "test-b__nested_node_modules",
+    srcs = [
         "//:node_modules/@gregmagolan/test-b/node_modules/@gregmagolan/test-a/main.js",
         "//:node_modules/@gregmagolan/test-b/node_modules/@gregmagolan/test-a/package.json",
-        "//:node_modules/@gregmagolan/test-b/package.json",
     ],
+    visibility = ["//visibility:private"],
+)
+filegroup(
+    name = "test-b__all_files",
+    srcs = [":test-b__files", ":test-b__nested_node_modules"],
+    visibility = ["//:__subpackages__"],
 )
 node_module_library(
     name = "test-b",
-    srcs = [":test-b__files"],
+    srcs = [":test-b__all_files"],
     deps = [
         "//@gregmagolan/test-b:test-b__contents",
     ],
 )
 node_module_library(
     name = "test-b__contents",
-    srcs = [":test-b__files"],
+    srcs = [":test-b__all_files"],
+    visibility = ["//:__subpackages__"],
 )
 node_module_library(
     name = "test-b__typings",