From a47410ecaa131eb85d76131984fb2da5f8dfd9a5 Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Sun, 18 Aug 2019 10:40:42 -0700 Subject: [PATCH] feat(builtin): introduce dynamic dependencies concept This lets our generated nodejs_binary targets for npm packages express dynamic runtime-only dependencies This is needed for example so that rollup binary can require the rollup-plugin-* packages --- WORKSPACE | 3 ++ internal/npm_install/generate_build_file.js | 41 ++++++++++++++++--- internal/npm_install/npm_install.bzl | 14 +++++++ .../test/generate_build_file.spec.js | 25 ++++++++++- .../golden/jasmine/bin/BUILD.bazel.golden | 2 +- packages/typescript/docs/install.md | 6 ++- 6 files changed, 81 insertions(+), 10 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index 36bf67647a..cb42319762 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -219,6 +219,9 @@ yarn_install( yarn_install( name = "npm_install_test", + # exercise the dynamic_deps feature, even though it doesn't make sense for a real jasmine binary to depend on zone.js + # This will just inject an extra data[] dependency into the jasmine_bin generated target. + dynamic_deps = {"jasmine": "zone.js"}, manual_build_file_contents = """ filegroup( name = "test_files", diff --git a/internal/npm_install/generate_build_file.js b/internal/npm_install/generate_build_file.js index fef4adfb19..c3a81e7a58 100644 --- a/internal/npm_install/generate_build_file.js +++ b/internal/npm_install/generate_build_file.js @@ -58,6 +58,7 @@ const RULE_TYPE = args[1]; const ERROR_ON_BAZEL_FILES = parseInt(args[2]); const LOCK_FILE_LABEL = args[3]; const INCLUDED_FILES = args[4] ? args[4].split(',') : []; +const DYNAMIC_DEPS = JSON.parse(args[5] || '{}'); if (require.main === module) { main(); @@ -103,6 +104,7 @@ function main() { module.exports = { main, printPackageBin, + addDynamicDependencies, }; /** @@ -436,6 +438,27 @@ function hasRootBuildFile(pkg, rootPath) { return false; } +function addDynamicDependencies(pkgs, dynamic_deps = DYNAMIC_DEPS) { + pkgs.forEach(p => { + function match(name) { + // Automatically include dynamic dependency on plugins of the form pkg-plugin-foo + if (name.startsWith(`${p._name}-plugin-`)) return true; + + const value = dynamic_deps[p._name]; + if (name === value) return true; + + // Support wildcard match + if (value && value.includes('*') && name.startsWith(value.substring(0, value.indexOf('*')))) { + return true; + } + + return false; + } + p._dynamicDependencies = + pkgs.filter(x => !!x._name && match(x._name)).map(dyn => `//${dyn._dir}:${dyn._name}`); + }); +} + /** * Finds and returns an array of all packages under a given path. */ @@ -460,6 +483,8 @@ function findPackages(p = 'node_modules') { packages.forEach( f => pkgs.push(parsePackage(f), ...findPackages(path.posix.join(f, 'node_modules')))); + addDynamicDependencies(pkgs); + const scopes = listing.filter(f => f.startsWith('@')) .map(f => path.posix.join(p, f)) .filter(f => isDirectory(f)); @@ -582,10 +607,10 @@ function cleanupEntryPointPath(p) { } /** - * Cleans up the given path + * Cleans up the given path * Then tries to resolve the path into a file and warns if in DEBUG and the file dosen't exist - * @param {any} pkg - * @param {string} path + * @param {any} pkg + * @param {string} path * @returns {string | undefined} */ function findEntryFile(pkg, path) { @@ -631,9 +656,9 @@ function resolveMainFile(pkg, mainFileName) { } /** - * Tries to resolve the mainFile from a given pkg + * Tries to resolve the mainFile from a given pkg * This uses seveal mainFileNames in priority to find a correct usable file - * @param {any} pkg + * @param {any} pkg * @returns {string | undefined} */ function resolvePkgMainFile(pkg) { @@ -953,6 +978,10 @@ function printPackageBin(pkg) { result = `load("@build_bazel_rules_nodejs//:defs.bzl", "nodejs_binary") `; + const data = [`//${pkg._dir}:${pkg._name}`]; + if (pkg._dynamicDependencies) { + data.push(...pkg._dynamicDependencies); + } for (const [name, path] of executables.entries()) { // Handle additionalAttributes of format: @@ -977,7 +1006,7 @@ nodejs_binary( name = "${name}", entry_point = "//:node_modules/${pkg._dir}/${path}", install_source_map_support = False, - data = ["//${pkg._dir}:${pkg._name}"],${additionalAttributes} + data = [${data.map(p => `"${p}"`).join(', ')}],${additionalAttributes} ) `; diff --git a/internal/npm_install/npm_install.bzl b/internal/npm_install/npm_install.bzl index 95f6d2ca7c..075a593753 100644 --- a/internal/npm_install/npm_install.bzl +++ b/internal/npm_install/npm_install.bzl @@ -65,6 +65,19 @@ COMMON_ATTRIBUTES = dict(dict(), **{ 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. + + 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. + + We must declare these dependencies so that Bazel includes the maybe-require()d package in the inputs to the program.""", + default = {"@bazel/typescript": "tsickle"}, + ), "exclude_packages": attr.string_list( doc = """DEPRECATED. This attribute is no longer used.""", ), @@ -141,6 +154,7 @@ def _create_build_files(repository_ctx, rule_type, node, lock_file): "1" if error_on_build_files else "0", str(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 581283fe02..65a4ecdee7 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} = require('../generate_build_file'); +const {printPackageBin, addDynamicDependencies} = require('../generate_build_file'); describe('build file generator', () => { describe('integration test', () => { @@ -77,4 +77,27 @@ describe('build file generator', () => { .toContain('nodejs_binary('); }); }); + + describe('dynamic dependencies', () => { + it('should include requested dynamic dependencies in nodejs_binary data', () => { + const pkgs = [{_name: 'foo', bin: 'foobin', _dir: 'some_dir'}, {_name: 'bar', _dir: 'bar'}]; + addDynamicDependencies(pkgs, {'foo': 'bar'}); + expect(pkgs[0]._dynamicDependencies).toEqual(['//bar:bar']); + expect(printPackageBin(pkgs[0])).toContain('data = ["//some_dir:foo", "//bar:bar"]'); + }); + it('should support wildcard', () => { + const pkgs = [{_name: 'foo', bin: 'foobin', _dir: 'some_dir'}, {_name: 'bar', _dir: 'bar'}]; + addDynamicDependencies(pkgs, {'foo': 'b*'}); + expect(pkgs[0]._dynamicDependencies).toEqual(['//bar:bar']); + expect(printPackageBin(pkgs[0])).toContain('data = ["//some_dir:foo", "//bar:bar"]'); + }); + it('should automatically include plugins in nodejs_binary data', () => { + const pkgs = + [{_name: 'foo', bin: 'foobin', _dir: 'some_dir'}, {_name: 'foo-plugin-bar', _dir: 'bar'}]; + addDynamicDependencies(pkgs, {}); + expect(pkgs[0]._dynamicDependencies).toEqual(['//bar:foo-plugin-bar']); + expect(printPackageBin(pkgs[0])) + .toContain('data = ["//some_dir:foo", "//bar:foo-plugin-bar"]'); + }); + }); }); \ No newline at end of file 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 8b43e86d58..7676d8ee0c 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"], + data = ["//jasmine:jasmine", "//zone.js:zone.js"], ) diff --git a/packages/typescript/docs/install.md b/packages/typescript/docs/install.md index 3e537e5ba2..0815ba7e76 100644 --- a/packages/typescript/docs/install.md +++ b/packages/typescript/docs/install.md @@ -104,7 +104,7 @@ managing npm dependencies with Bazel. ## Customizing the TypeScript compiler binary -An example is needing to increase the NodeJS heap size used for compilations. +An example use case is needing to increase the NodeJS heap size used for compilations. Similar to above, you declare your own binary for running tsc_wrapped, e.g.: @@ -127,7 +127,9 @@ nodejs_binary( then refer to that target in the `compiler` attribute of your `ts_library` rule. -Another use case is to use [tsickle], which is an optional dependency of `tsc_wrapped`. In case it needs `"@npm//tsickle"` added to the `data` attribute above. +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