From 3a35786d8e78627f9e0783fbf45e1c960eaccdf8 Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Wed, 9 May 2018 06:42:24 -0700 Subject: [PATCH] Fixes for npm_package: (#198) * Fixes for npm_package: - Allow source files to appear under deps, like .d.ts srcs of a ts_library - Include ts_library transitive_declarations - Include data attributes of ts_library Fixes #196 Fixes #197 * Add tests for npm_package fixes --- internal/common/typescript_mock_lib.bzl | 18 ++++++++++++++---- internal/npm_package/npm_package.bzl | 15 ++++++++++++++- internal/npm_package/packager.js | 8 +++++++- internal/npm_package/test/BUILD.bazel | 15 ++++++++++++++- internal/npm_package/test/data.json | 1 + internal/npm_package/test/foo.d.ts | 1 + internal/npm_package/test/foo.js | 1 + internal/npm_package/test/npm_package.spec.js | 6 ++++++ 8 files changed, 58 insertions(+), 7 deletions(-) create mode 100644 internal/npm_package/test/data.json create mode 100644 internal/npm_package/test/foo.d.ts create mode 100644 internal/npm_package/test/foo.js diff --git a/internal/common/typescript_mock_lib.bzl b/internal/common/typescript_mock_lib.bzl index 83f3943631..5805766db9 100644 --- a/internal/common/typescript_mock_lib.bzl +++ b/internal/common/typescript_mock_lib.bzl @@ -14,19 +14,29 @@ """A mock typescript_lib rule. -Allows testing that node_jasmine_test will work with ts_library from -rules_typescript without introducing a circular dependency. +Allows testing that jasmine_node_test will work with ts_library from +rules_typescript without introducing a circular dependency between +rules_nodejs and rules_typescript repositories. """ def _mock_typescript_lib(ctx): es5_sources = depset() + transitive_decls = depset() for s in ctx.attr.srcs: - es5_sources = depset(transitive=[es5_sources, s.files]) - return struct(typescript = struct(es5_sources = es5_sources)) + es5_sources = depset([f for f in s.files if f.path.endswith(".js")], transitive = [es5_sources]) + transitive_decls = depset([f for f in s.files if f.path.endswith(".d.ts")], transitive = [transitive_decls]) + return struct( + runfiles = ctx.runfiles(collect_default=True, collect_data = True), + typescript = struct( + es5_sources = es5_sources, + transitive_declarations = transitive_decls + ), + ) mock_typescript_lib = rule( implementation = _mock_typescript_lib, attrs = { "srcs": attr.label_list(allow_files = True), + "data": attr.label_list(allow_files = True, cfg = "data"), } ) diff --git a/internal/npm_package/npm_package.bzl b/internal/npm_package/npm_package.bzl index 9164c2f3ae..ff85c9d73b 100644 --- a/internal/npm_package/npm_package.bzl +++ b/internal/npm_package/npm_package.bzl @@ -58,7 +58,20 @@ def create_package(ctx, devmode_sources, nested_packages): def _npm_package(ctx): files = depset() for d in ctx.attr.deps: - files = depset(transitive = [files, d.files, d.node_sources]) + transitive = [ + files, + # Collect whatever is in the "data" + d.data_runfiles.files, + # For JavaScript-producing rules, gather up the devmode Node.js sources + d.node_sources, + ] + + # ts_library doesn't include .d.ts outputs in the runfiles + # see comment in rules_typescript/internal/common/compilation.bzl + if hasattr(d, "typescript"): + transitive.append(d.typescript.transitive_declarations) + + files = depset(transitive = transitive) package_dir = create_package(ctx, files.to_list(), ctx.files.packages) diff --git a/internal/npm_package/packager.js b/internal/npm_package/packager.js index 7903b14dcd..9330e1837e 100644 --- a/internal/npm_package/packager.js +++ b/internal/npm_package/packager.js @@ -81,7 +81,13 @@ function main(args) { } else if (!path.relative(genDir, f).startsWith('..')) { rootDir = genDir; } else { - throw new Error(`dependency ${f} is not under bazel-bin or bazel-genfiles`); + // It might be nice to enforce here that deps don't contain sources + // since those belong in srcs above. + // The `deps` attribute should typically be outputs of other rules. + // However, things like .d.ts sources of a ts_library or data attributes + // of ts_library will result in source files that appear in the deps + // so we have to allow this. + rootDir = '.'; } return path.join(outDir, path.relative(path.join(rootDir, baseDir), f)); } diff --git a/internal/npm_package/test/BUILD.bazel b/internal/npm_package/test/BUILD.bazel index 2093e0f0fe..7d5fc918ea 100644 --- a/internal/npm_package/test/BUILD.bazel +++ b/internal/npm_package/test/BUILD.bazel @@ -1,4 +1,5 @@ load("//:defs.bzl", "npm_package", "jasmine_node_test") +load("//internal/common:typescript_mock_lib.bzl", "mock_typescript_lib") genrule( name = "produces_files", @@ -6,6 +7,15 @@ genrule( cmd = "echo \"a_dep content\" > $@", ) +mock_typescript_lib( + name = "ts_library", + srcs = [ + "foo.d.ts", + "foo.js", + ], + data = ["data.json"], +) + npm_package( name = "dependent_pkg", srcs = ["dependent_file"], @@ -16,7 +26,10 @@ npm_package( srcs = ["some_file"], packages = [":dependent_pkg"], replacements = {"replace_me": "replaced"}, - deps = [":produces_files"], + deps = [ + ":produces_files", + ":ts_library", + ], ) jasmine_node_test( diff --git a/internal/npm_package/test/data.json b/internal/npm_package/test/data.json new file mode 100644 index 0000000000..fe51488c70 --- /dev/null +++ b/internal/npm_package/test/data.json @@ -0,0 +1 @@ +[] diff --git a/internal/npm_package/test/foo.d.ts b/internal/npm_package/test/foo.d.ts new file mode 100644 index 0000000000..425b80f244 --- /dev/null +++ b/internal/npm_package/test/foo.d.ts @@ -0,0 +1 @@ +export const a: string; diff --git a/internal/npm_package/test/foo.js b/internal/npm_package/test/foo.js new file mode 100644 index 0000000000..ca6a755661 --- /dev/null +++ b/internal/npm_package/test/foo.js @@ -0,0 +1 @@ +export const a = ''; diff --git a/internal/npm_package/test/npm_package.spec.js b/internal/npm_package/test/npm_package.spec.js index cd8053eb0e..a2614fad3b 100644 --- a/internal/npm_package/test/npm_package.spec.js +++ b/internal/npm_package/test/npm_package.spec.js @@ -21,4 +21,10 @@ describe('npm_package srcs', () => { it('copies files from other packages', () => { expect(read('dependent_file')).toEqual('dependent_file content'); }); + it('copies declaration files from ts_library', () => { + expect(read('foo.d.ts')).toEqual('export const a: string;'); + }); + it('copies data dependencies', () => { + expect(read('data.json')).toEqual('[]'); + }); });