From 2bf735b87455c606a9fb7ebe4738fda3fde73b8f Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Wed, 16 Oct 2019 10:37:02 -0700 Subject: [PATCH] refactor: remove dynamic_deps feature 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 | 8 ------ internal/npm_install/generate_build_file.js | 24 ---------------- internal/npm_install/generate_build_file.ts | 28 ------------------- internal/npm_install/npm_install.bzl | 21 -------------- .../test/generate_build_file.spec.js | 27 +----------------- .../test-a/bin/BUILD.bazel.golden | 2 +- .../@gregmagolan/test-a/index.bzl.golden | 4 +-- .../golden/jasmine/bin/BUILD.bazel.golden | 2 +- .../test/golden/jasmine/index.bzl.golden | 4 +-- packages/typescript/docs/install.md | 1 - 10 files changed, 7 insertions(+), 114 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index b23dce08ab..6026ac07f1 100644 --- a/WORKSPACE +++ b/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", diff --git a/internal/npm_install/generate_build_file.js b/internal/npm_install/generate_build_file.js index 39f3b60f95..f80d62038c 100644 --- a/internal/npm_install/generate_build_file.js +++ b/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; } /** diff --git a/internal/npm_install/generate_build_file.ts b/internal/npm_install/generate_build_file.ts index bca51329df..6eddaaed4d 100644 --- a/internal/npm_install/generate_build_file.ts +++ b/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; } diff --git a/internal/npm_install/npm_install.bzl b/internal/npm_install/npm_install.bzl index cec02cdb0b..9fe884dfe6 100644 --- a/internal/npm_install/npm_install.bzl +++ b/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)) diff --git a/internal/npm_install/test/generate_build_file.spec.js b/internal/npm_install/test/generate_build_file.spec.js index 841bedcaed..91560c712b 100644 --- a/internal/npm_install/test/generate_build_file.spec.js +++ b/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'}); diff --git a/internal/npm_install/test/golden/@gregmagolan/test-a/bin/BUILD.bazel.golden b/internal/npm_install/test/golden/@gregmagolan/test-a/bin/BUILD.bazel.golden index 046d3b50f8..b999b71d1e 100644 --- a/internal/npm_install/test/golden/@gregmagolan/test-a/bin/BUILD.bazel.golden +++ b/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"], ) diff --git a/internal/npm_install/test/golden/@gregmagolan/test-a/index.bzl.golden b/internal/npm_install/test/golden/@gregmagolan/test-a/index.bzl.golden index ce607d6a25..34d685e941 100644 --- a/internal/npm_install/test/golden/@gregmagolan/test-a/index.bzl.golden +++ b/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 ) diff --git a/internal/npm_install/test/golden/jasmine/bin/BUILD.bazel.golden b/internal/npm_install/test/golden/jasmine/bin/BUILD.bazel.golden index 6bd5143235..0ce9a6f06e 100644 --- a/internal/npm_install/test/golden/jasmine/bin/BUILD.bazel.golden +++ b/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"], ) diff --git a/internal/npm_install/test/golden/jasmine/index.bzl.golden b/internal/npm_install/test/golden/jasmine/index.bzl.golden index 9067db7274..fb339b6688 100644 --- a/internal/npm_install/test/golden/jasmine/index.bzl.golden +++ b/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 ) diff --git a/packages/typescript/docs/install.md b/packages/typescript/docs/install.md index ee1a6fc49b..41bdc7c1fa 100644 --- a/packages/typescript/docs/install.md +++ b/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