Skip to content

Commit

Permalink
fix(npm_install): dynamic_deps attribute not working for scoped packages
Browse files Browse the repository at this point in the history
  • Loading branch information
devversion authored and alexeagle committed Sep 6, 2019
1 parent aa83f90 commit bf68577
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 16 deletions.
11 changes: 8 additions & 3 deletions WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -217,9 +217,14 @@ yarn_install(

yarn_install(
name = "fine_grained_goldens",
# 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"},
# 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",
Expand Down
16 changes: 10 additions & 6 deletions internal/npm_install/generate_build_file.js
Original file line number Diff line number Diff line change
Expand Up @@ -444,9 +444,9 @@ 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;
if (name.startsWith(`${p._moduleName}-plugin-`)) return true;

const value = dynamic_deps[p._name];
const value = dynamic_deps[p._moduleName];
if (name === value) return true;

// Support wildcard match
Expand All @@ -456,8 +456,8 @@ function addDynamicDependencies(pkgs, dynamic_deps = DYNAMIC_DEPS) {

return false;
}
p._dynamicDependencies =
pkgs.filter(x => !!x._name && match(x._name)).map(dyn => `//${dyn._dir}:${dyn._name}`);
p._dynamicDependencies = pkgs.filter(x =>
!!x._moduleName && match(x._moduleName)).map(dyn => `//${dyn._dir}:${dyn._name}`);
});
}

Expand Down Expand Up @@ -485,13 +485,13 @@ 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));
scopes.forEach(f => pkgs.push(...findPackages(f)));

addDynamicDependencies(pkgs);

return pkgs;
}

Expand Down Expand Up @@ -532,6 +532,10 @@ function parsePackage(p) {
// Stash the package directory name for future use
pkg._name = pkg._dir.split('/').pop();

// Module name of the package. Unlike "_name" this represents the
// full package name (including scope name).
pkg._moduleName = pkg.name || `${pkg._dir}/${pkg._name}`;

// Keep track of whether or not this is a nested package
pkg._isNested = /\/node_modules\//.test(p);

Expand Down
24 changes: 18 additions & 6 deletions internal/npm_install/test/generate_build_file.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,24 +80,36 @@ 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'}, {_name: 'bar', _dir: 'bar'}];
addDynamicDependencies(pkgs, {'foo': 'bar'});
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'}, {_name: 'bar', _dir: 'bar'}];
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"]');
});
it('should automatically include plugins in nodejs_binary data', () => {
const pkgs =
[{_name: 'foo', bin: 'foobin', _dir: 'some_dir'}, {_name: 'foo-plugin-bar', _dir: 'bar'}];
const pkgs = [
{_name: 'foo', bin: 'foobin', _dir: 'some_dir', _moduleName: 'foo'},
{_name: 'foo-plugin-bar', _dir: 'bar', _moduleName: 'foo-plugin-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"]');
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
data = ["//@gregmagolan/test-a:test-a", "//@angular/core:core"],
)

0 comments on commit bf68577

Please sign in to comment.