fix(builtin): bugs in 0.38 found while rolling out to angular repo

bazel-contrib · Oct 3, 2019 · d2262c8 · d2262c8
1 parent 1ecb527
commit d2262c8
Show file tree

Hide file tree

Showing 5 changed files with 36 additions and 13 deletions.
diff --git a/internal/linker/link_node_modules.bzl b/internal/linker/link_node_modules.bzl
@@ -122,19 +122,21 @@ def get_module_mappings(label, attrs, vars, rule_kind, srcs = [], workspace_name
     elif label.workspace_root:
         mr = "%s/%s" % (label.workspace_root, mr)
 
-    if mn in mappings and mappings[mn] != mr:
-        fail(("duplicate module mapping at %s: %s maps to both %s and %s" %
-              (label, mn, mappings[mn], mr)), "deps")
-    _debug(vars, "target %s adding module mapping %s: %s" % (label, mn, mr))
-
     # since our module mapping is currently based on attribute names,
     # allow a special one to instruct the linker that the package has no output
     # directory and is therefore meant to be used as sources.
     # TODO: This belongs in a different mechanism like a package.json field.
     if getattr(attrs, "module_from_src", False):
-        mappings[mn] = ["src", mr]
+        mr = ["src", mr]
     else:
-        mappings[mn] = ["bin", mr]
+        mr = ["bin", mr]
+
+    if mn in mappings and mappings[mn] != mr:
+        fail(("duplicate module mapping at %s: %s maps to both %s and %s" %
+              (label, mn, mappings[mn], mr)), "deps")
+    _debug(vars, "target %s adding module mapping %s: %s" % (label, mn, mr))
+
+    mappings[mn] = mr
     return mappings
 
 # When building a mapping for use at runtime, we need paths to be relative to

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
@@ -473,8 +473,13 @@ function addDynamicDependencies(pkgs: Dep[], dynamic_deps = DYNAMIC_DEPS) {
     return false;
   }
   pkgs.forEach(p => {
-    p._dynamicDependencies = pkgs.filter(x => !!x._moduleName && match(x._moduleName, p))
-                                 .map(dyn => `//${dyn._dir}:${dyn._name}`);
+    p._dynamicDependencies =
+        pkgs.filter(
+                // Filter entries like
+                // "_dir":"check-side-effects/node_modules/rollup-plugin-node-resolve"
+                x => !x._dir.includes('/node_modules/') && !!x._moduleName &&
+                    match(x._moduleName, p))
+            .map(dyn => `//${dyn._dir}:${dyn._name}`);
   });
 }
 

diff --git a/internal/npm_install/test/BUILD.bazel b/internal/npm_install/test/BUILD.bazel
@@ -13,7 +13,7 @@ jasmine_node_test(
     data = [
         ":check.js",
         ":goldens",
-        "//internal/npm_install:generate_build_file",
+        "//internal/npm_install:compile_generate_build_file",
         "@fine_grained_goldens//:golden_files",
         "@npm//unidiff",
     ],

diff --git a/internal/npm_install/test/generate_build_file.spec.js b/internal/npm_install/test/generate_build_file.spec.js
@@ -111,6 +111,17 @@ describe('build file generator', () => {
       expect(printPackageBin(pkgs[0]))
           .toContain('data = ["//some_dir:foo", "//bar:foo-plugin-bar"]');
     });
+    it('should not include nested packages', () => {
+      const pkgs = [
+        {_name: 'foo', bin: 'foobin', _dir: 'some_dir', _moduleName: 'foo'}, {
+          _name: 'foo-plugin-bar',
+          _dir: 'top-level/node_modules/bar',
+          _moduleName: 'foo-plugin-bar'
+        }
+      ];
+      addDynamicDependencies(pkgs, {});
+      expect(pkgs[0]._dynamicDependencies).toEqual([]);
+    });
   });
 
   describe('index.bzl files', () => {