refactor: remove dynamic_deps feature (#1276)

This isn't the right way to declare that a rule depends on a plugin.

- It is too hard for users to understand what this means, how to
configure it correctly, and to discover that their broken tool needs a
dynamic dep.
- It is not npm-idiomatic. The dependency graph at install time is not
meant to reflect the actual dependencies that will be loaded at runtime.

In the rollup example, the `rollup.config.js` has `import
'rollup-plugin-json'`
so the corresponding BUILD file should have that package in the deps.

BREAKING CHANGE:

The dynamic_deps attribute of yarn_install and npm_install is removed,
in favor of declaring needed packages in the deps/data of the rule that
invokes the tool.

WORKSPACE

@@ -217,14 +217,6 @@ yarn_install(
 
 yarn_install(
     name = "fine_grained_goldens",
-    # exercise the dynamic_deps feature, even though it doesn't make sense for the targets to
-    # depend on zone.js or Angular core. This will just inject an extra data[] dependency into
-    # the generated binary targets. Note that we also ensure that scoped packages can be properly
-    # modified.
-    dynamic_deps = {
-        "@gregmagolan/test-a": "@angular/core",
-        "jasmine": "zone.js",
-    },
     manual_build_file_contents = """
 filegroup(
   name = "golden_files",

internal/npm_install/generate_build_file.js

@@ -68,7 +68,6 @@ package(default_visibility = ["//visibility:public"])
     const ERROR_ON_BAZEL_FILES = parseInt(args[2]);
     const LOCK_FILE_PATH = args[3];
     const INCLUDED_FILES = args[4] ? args[4].split(',') : [];
-    const DYNAMIC_DEPS = JSON.parse(args[5] || '{}');
     if (require.main === module) {
         main();
     }
@@ -106,7 +105,6 @@ package(default_visibility = ["//visibility:public"])
     module.exports = {
         main,
         printPackageBin,
-        addDynamicDependencies,
         printIndexBzl,
     };
     /**
@@ -417,27 +415,6 @@ def _maybe(repo_rule, name, **kwargs):
         }
         return false;
     }
-    function addDynamicDependencies(pkgs, dynamic_deps = DYNAMIC_DEPS) {
-        function match(name, p) {
-            const value = dynamic_deps[p._moduleName];
-            if (name === value)
-                return true;
-            // Support wildcard match
-            if (value && value.includes('*') && name.startsWith(value.substring(0, value.indexOf('*')))) {
-                return true;
-            }
-            return false;
-        }
-        pkgs.forEach(p => {
-            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}`);
-        });
-    }
     /**
      * Finds and returns an array of all packages under a given path.
      */
@@ -466,7 +443,6 @@ def _maybe(repo_rule, name, **kwargs):
             .map(f => path.posix.join(p, f))
             .filter(f => isDirectory(f));
         scopes.forEach(f => pkgs.push(...findPackages(f)));
-        addDynamicDependencies(pkgs);
         return pkgs;
     }
     /**

internal/npm_install/generate_build_file.ts

@@ -62,7 +62,6 @@ const RULE_TYPE = args[1];
 const ERROR_ON_BAZEL_FILES = parseInt(args[2]);
 const LOCK_FILE_PATH = args[3];
 const INCLUDED_FILES = args[4] ? args[4].split(',') : [];
-const DYNAMIC_DEPS = JSON.parse(args[5] || '{}');
 
 if (require.main === module) {
   main();
@@ -108,7 +107,6 @@ function main() {
 module.exports = {
   main,
   printPackageBin,
-  addDynamicDependencies,
   printIndexBzl,
 };
 
@@ -456,30 +454,6 @@ function hasRootBuildFile(pkg: Dep, rootPath: string) {
   return false;
 }
 
-
-function addDynamicDependencies(pkgs: Dep[], dynamic_deps = DYNAMIC_DEPS) {
-  function match(name: string, p: Dep) {
-    const value = dynamic_deps[p._moduleName];
-    if (name === value) return true;
-
-    // Support wildcard match
-    if (value && value.includes('*') && name.startsWith(value.substring(0, value.indexOf('*')))) {
-      return true;
-    }
-
-    return false;
-  }
-  pkgs.forEach(p => {
-    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}`);
-  });
-}
-
 /**
  * Finds and returns an array of all packages under a given path.
  */
@@ -514,8 +488,6 @@ function findPackages(p = 'node_modules') {
                      .filter(f => isDirectory(f));
   scopes.forEach(f => pkgs.push(...findPackages(f)));
 
-  addDynamicDependencies(pkgs);
-
   return pkgs;
 }
 

internal/npm_install/npm_install.bzl

@@ -66,26 +66,6 @@ If symlink_node_modules is True, this attribute is ignored since
 the dependency manager will run in the package.json location.
 """,
     ),
-    "dynamic_deps": attr.string_dict(
-        doc = """Declare implicit dependencies between npm packages.
-
-In many cases, an npm package doesn't list a dependency on another package, yet still require()s it.
-One example is plugins, where a tool like rollup can require rollup-plugin-json if the user installed it.
-Another example is the tsc_wrapped binary in @bazel/typescript which can require tsickle if its installed.
-Under Bazel, we must declare these dependencies so that they are included as inputs to the program.
-
-Note that the pattern used by many packages, which have plugins in the form pkg-plugin-someplugin, are automatically
-added as implicit dependencies. Thus for example, `rollup` will automatically get `rollup-plugin-json` included in its
-dependencies without needing to use this attribute.
-
-The keys in the dict are npm package names, and the value may be a particular package, or a prefix ending with *.
-For example, `dynamic_deps = {"@bazel/typescript": "tsickle", "karma": "my-karma-plugin-*"}`
-
-Note, this may sound like "optionalDependencies" but that field in package.json actually means real dependencies
-which are installed, but failures on installation are ignored.
-""",
-        default = {"@bazel/typescript": "tsickle"},
-    ),
     "exclude_packages": attr.string_list(
         doc = """DEPRECATED. This attribute is no longer used.""",
     ),
@@ -165,7 +145,6 @@ def _create_build_files(repository_ctx, rule_type, node, lock_file):
         "1" if error_on_build_files else "0",
         repository_ctx.path(lock_file),
         ",".join(repository_ctx.attr.included_files),
-        str(repository_ctx.attr.dynamic_deps),
     ])
     if result.return_code:
         fail("generate_build_file.js failed: \nSTDOUT:\n%s\nSTDERR:\n%s" % (result.stdout, result.stderr))

internal/npm_install/test/generate_build_file.spec.js

@@ -1,5 +1,5 @@
 const {check, files} = require('./check');
-const {printPackageBin, printIndexBzl, addDynamicDependencies} = require('../generate_build_file');
+const {printPackageBin, printIndexBzl} = require('../generate_build_file');
 
 describe('build file generator', () => {
   describe('integration test', () => {
@@ -78,31 +78,6 @@ describe('build file generator', () => {
     });
   });
 
-  describe('dynamic dependencies', () => {
-    it('should include requested dynamic dependencies in nodejs_binary data', () => {
-      const pkgs = [
-        {_name: 'foo', bin: 'foobin', _dir: 'some_dir', _moduleName: 'foo'},
-        {_name: 'bar', _dir: 'bar', _moduleName: 'bar'},
-        {_name: 'typescript', bin: 'tsc_wrapped', _dir: 'a', _moduleName: '@bazel/typescript'},
-        {_name: 'tsickle', _dir: 'b', _moduleName: 'tsickle'},
-      ];
-      addDynamicDependencies(pkgs, {'foo': 'bar', '@bazel/typescript': 'tsickle'});
-      expect(pkgs[0]._dynamicDependencies).toEqual(['//bar:bar']);
-      expect(pkgs[2]._dynamicDependencies).toEqual(['//b:tsickle']);
-      expect(printPackageBin(pkgs[0])).toContain('data = ["//some_dir:foo", "//bar:bar"]');
-      expect(printPackageBin(pkgs[2])).toContain('data = ["//a:typescript", "//b:tsickle"]');
-    });
-    it('should support wildcard', () => {
-      const pkgs = [
-        {_name: 'foo', bin: 'foobin', _dir: 'some_dir', _moduleName: 'foo'},
-        {_name: 'bar', _dir: 'bar', _moduleName: 'bar'}
-      ];
-      addDynamicDependencies(pkgs, {'foo': 'b*'});
-      expect(pkgs[0]._dynamicDependencies).toEqual(['//bar:bar']);
-      expect(printPackageBin(pkgs[0])).toContain('data = ["//some_dir:foo", "//bar:bar"]');
-    });
-  });
-
   describe('index.bzl files', () => {
     it('should encode npm binaries to be valid macro names', () => {
       const bzl = printIndexBzl({_dir: 'http-server', bin: 'http-server'});

internal/npm_install/test/golden/@gregmagolan/test-a/bin/BUILD.bazel.golden

@@ -5,5 +5,5 @@ nodejs_binary(
     name = "test",
     entry_point = "//:node_modules/@gregmagolan/test-a/@bin/test.js",
     install_source_map_support = False,
-    data = ["//@gregmagolan/test-a:test-a", "//@angular/core:core"],
+    data = ["//@gregmagolan/test-a:test-a"],
 )

internal/npm_install/test/golden/@gregmagolan/test-a/index.bzl.golden

@@ -7,13 +7,13 @@ def test(**kwargs):
         nodejs_binary(
             entry_point = "@fine_grained_goldens//:node_modules/@gregmagolan/test-a/@bin/test.js",
             install_source_map_support = False,
-            data = ["@fine_grained_goldens//@gregmagolan/test-a:test-a", "//@angular/core:core"] + kwargs.pop("data", []),
+            data = ["@fine_grained_goldens//@gregmagolan/test-a:test-a"] + kwargs.pop("data", []),
             **kwargs
         )
 def test_test(**kwargs):
     nodejs_test(
       entry_point = "@fine_grained_goldens//:node_modules/@gregmagolan/test-a/@bin/test.js",
       install_source_map_support = False,
-      data = ["@fine_grained_goldens//@gregmagolan/test-a:test-a", "//@angular/core:core"] + kwargs.pop("data", []),
+      data = ["@fine_grained_goldens//@gregmagolan/test-a:test-a"] + kwargs.pop("data", []),
       **kwargs
     )

internal/npm_install/test/golden/jasmine/bin/BUILD.bazel.golden

@@ -5,5 +5,5 @@ nodejs_binary(
     name = "jasmine",
     entry_point = "//:node_modules/jasmine/bin/jasmine.js",
     install_source_map_support = False,
-    data = ["//jasmine:jasmine", "//zone.js:zone.js"],
+    data = ["//jasmine:jasmine"],
 )

internal/npm_install/test/golden/jasmine/index.bzl.golden

@@ -7,13 +7,13 @@ def jasmine(**kwargs):
         nodejs_binary(
             entry_point = "@fine_grained_goldens//:node_modules/jasmine/bin/jasmine.js",
             install_source_map_support = False,
-            data = ["@fine_grained_goldens//jasmine:jasmine", "//zone.js:zone.js"] + kwargs.pop("data", []),
+            data = ["@fine_grained_goldens//jasmine:jasmine"] + kwargs.pop("data", []),
             **kwargs
         )
 def jasmine_test(**kwargs):
     nodejs_test(
       entry_point = "@fine_grained_goldens//:node_modules/jasmine/bin/jasmine.js",
       install_source_map_support = False,
-      data = ["@fine_grained_goldens//jasmine:jasmine", "//zone.js:zone.js"] + kwargs.pop("data", []),
+      data = ["@fine_grained_goldens//jasmine:jasmine"] + kwargs.pop("data", []),
       **kwargs
     )

packages/typescript/docs/install.md

@@ -168,7 +168,6 @@ then refer to that target in the `compiler` attribute of your `ts_library` rule.
 
 Note that `nodejs_binary` targets generated by `npm_install`/`yarn_install` can include data dependencies
 on packages which aren't declared as dependencies. For example, if you use [tsickle] to generate Closure Compiler-compatible JS, then it needs to be a `data` dependency of `tsc_wrapped` so that it can be loaded at runtime.
-See the `dynamic_deps` attribute of `npm_install`/`yarn_install` to include more such runtime dependencies in the generated `nodejs_binary` targets, rather than needing to define a custom one.
 
 [tsickle]: https://github.com/angular/tsickle