Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add e2e tests/examples of transitive npm deps for ts_library - nodejs_binary interop #612

Closed
wants to merge 24 commits into from
Closed
Show file tree
Hide file tree
Changes from 21 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
47 changes: 47 additions & 0 deletions e2e/typescript_3.1/BUILD.bazel
Expand Up @@ -12,12 +12,25 @@
# See the License for the specific language governing permissions and
# limitations under the License.

load("@build_bazel_rules_nodejs//:defs.bzl", "nodejs_binary", "rollup_bundle")
load("@npm_bazel_jasmine//:index.bzl", "jasmine_node_test")
load("@npm_bazel_typescript//:index.bzl", "ts_library")

ts_library(
name = "lib",
srcs = ["lib.ts"],
deps = [
"@npm//@types/node",
"@npm//date-fns",
],
)

ts_library(
name = "main",
srcs = ["main.ts"],
deps = [
":lib",
],
)

ts_library(
Expand All @@ -42,3 +55,37 @@ jasmine_node_test(
":test_lib",
],
)

nodejs_binary(
name = "main_js",
data = [
":main",
],
entry_point = "e2e_typescript_3_1/main.js",
)

nodejs_binary(
name = "main_js_sm",
data = [
":main",
"@npm//source-map-support",
],
entry_point = "e2e_typescript_3_1/main.js",
)

rollup_bundle(
name = "bundle",
entry_point = "main",
deps = [
":main",
],
)

rollup_bundle(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should change the test script in package.json to bazel build ... && bazel test ... so that these targets are built

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am pretty sure bazel test ... implicitly also builds all targets (unless --build_tests_only) is also specified. Let me verify this though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep just verified. I had a failing rollup_bundle and bazel test ... failed on building it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhhh. Good to know. Thanks for verifying.

name = "bundle_sm",
entry_point = "main",
deps = [
":main",
"@npm//source-map-support",
],
)
5 changes: 5 additions & 0 deletions e2e/typescript_3.1/lib.ts
@@ -0,0 +1,5 @@
import {format} from 'date-fns'

export function sayDate() {
return 'hello ' + format(new Date(2014, 1, 11), 'MM/dd/YYYY');
}
2 changes: 1 addition & 1 deletion e2e/typescript_3.1/main.spec.ts
Expand Up @@ -4,7 +4,7 @@ import * as main from './main';

describe('main', () => {
it('should compile and run with @bazel/typescript npm package', () => {
expect(main.test()).toEqual('test');
expect(main.test()).toEqual('test hello 02/Tu/2014');
});

it('should successfully require @bazel/typescript', () => {
Expand Down
6 changes: 5 additions & 1 deletion e2e/typescript_3.1/main.ts
@@ -1,3 +1,7 @@
import {sayDate} from './lib';

console.log(sayDate());

export function test() {
return 'test';
return `test ${sayDate()}`;
}
2 changes: 2 additions & 0 deletions e2e/typescript_3.1/package.json
Expand Up @@ -4,6 +4,8 @@
"@bazel/typescript": "bazel://@npm_bazel_typescript//:npm_package",
"@types/jasmine": "2.8.2",
"@types/node": "7.0.18",
"date-fns": "^1.30.1",
"source-map-support": "^0.5.10",
"typescript": "3.1.x"
},
"scripts": {
Expand Down
27 changes: 17 additions & 10 deletions e2e/typescript_3.1/yarn.lock
Expand Up @@ -2,15 +2,14 @@
# yarn lockfile v1


"@bazel/jasmine@file:../../packages/jasmine/bazel-bin/npm_package":
version "0.0.0"
"@bazel/jasmine@file:../../dist/npm_bazel_jasmine":
version "0.15.3-226-g0e656e0"
dependencies:
jasmine "~3.3.1"

"@bazel/typescript@file:../../packages/typescript/bazel-bin/npm_package":
version "0.0.0"
"@bazel/typescript@file:../../dist/npm_bazel_typescript":
version "0.15.3-238-g52c91ea"
dependencies:
jasmine-core "2.8.0"
protobufjs "5.0.3"
semver "5.6.0"
source-map-support "0.5.9"
Expand Down Expand Up @@ -93,6 +92,11 @@ concat-map@0.0.1:
resolved "https://registry.yarnpkg.com/concat-map/-/concat-map-0.0.1.tgz#d8a96bd77fd68df7793a73036a3ba0d5405d477b"
integrity sha1-2Klr13/Wjfd5OnMDajug1UBdR3s=

date-fns@^1.30.1:
version "1.30.1"
resolved "https://registry.yarnpkg.com/date-fns/-/date-fns-1.30.1.tgz#2e71bf0b119153dbb4cc4e88d9ea5acfb50dc05c"
integrity sha512-hBSVCvSmWC+QypYObzwGOd9wqdDpOt+0wl0KbU+R+uuZBS1jN8VsD1ss3irQDknRj5NvxiTF6oj/nDRnN/UQNw==

decamelize@^1.1.1:
version "1.2.0"
resolved "https://registry.yarnpkg.com/decamelize/-/decamelize-1.2.0.tgz#f6534d15148269b20352e7bee26f501f9a191290"
Expand Down Expand Up @@ -140,11 +144,6 @@ is-fullwidth-code-point@^1.0.0:
dependencies:
number-is-nan "^1.0.0"

jasmine-core@2.8.0:
version "2.8.0"
resolved "https://registry.yarnpkg.com/jasmine-core/-/jasmine-core-2.8.0.tgz#bcc979ae1f9fd05701e45e52e65d3a5d63f1a24e"
integrity sha1-vMl5rh+f0FcB5F5S5l06XWPxok4=

jasmine-core@~3.3.0:
version "3.3.0"
resolved "https://registry.yarnpkg.com/jasmine-core/-/jasmine-core-3.3.0.tgz#dea1cdc634bc93c7e0d4ad27185df30fa971b10e"
Expand Down Expand Up @@ -229,6 +228,14 @@ source-map-support@0.5.9:
buffer-from "^1.0.0"
source-map "^0.6.0"

source-map-support@^0.5.10:
version "0.5.10"
resolved "https://registry.yarnpkg.com/source-map-support/-/source-map-support-0.5.10.tgz#2214080bc9d51832511ee2bab96e3c2f9353120c"
integrity sha512-YfQ3tQFTK/yzlGJuX8pTwa4tifQj4QS2Mj7UegOu8jAz59MqIiMGPXxQhVQiIMNzayuUSF/jEuVnfFF5JqybmQ==
dependencies:
buffer-from "^1.0.0"
source-map "^0.6.0"

source-map@^0.6.0:
version "0.6.1"
resolved "https://registry.yarnpkg.com/source-map/-/source-map-0.6.1.tgz#74722af32e9614e9c287a8d0bbde48b5e2f1a263"
Expand Down
23 changes: 23 additions & 0 deletions internal/common/node_module_info.bzl
Expand Up @@ -22,13 +22,36 @@ NodeModuleInfo = provider(
},
)

NodeModuleSources = provider(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this is over complicating things here with two different node_modules providers and a NODE_MODULE_MARKER tag. This may be a good opportunity to remove the tag and create a single rule that is used instead of filegroup in generate_build_file.js so we can avoid the marker and have a single provider.

@kyliau recently added the ng_apf_library() rule which is used instead of filegroup when an angular package format is detected. We consolidate this work into a single rule and solve the transitive deps problem as well.

doc = "This provider contains all the transtive npm dependency sources of a non-npm dependency.",
fields = {
"srcs": "List of source files that are npm dependencies",
},
)

def _collect_node_modules_aspect_impl(target, ctx):
nm_wksp = None

if hasattr(ctx.rule.attr, "tags") and "NODE_MODULE_MARKER" in ctx.rule.attr.tags:
nm_wksp = target.label.workspace_root.split("/")[1] if target.label.workspace_root else ctx.workspace_name
return [NodeModuleInfo(workspace = nm_wksp)]

info = []
srcs = depset()
if hasattr(ctx.rule.attr, "deps"):
for dep in ctx.rule.attr.deps:
if NodeModuleInfo in dep and NodeModuleSources in dep:
fail("Dependency %s has both NodeModuleInfo and NodeModuleSources provider. It can only have one or the other." % dep)
if NodeModuleInfo in dep:
if nm_wksp and dep[NodeModuleInfo].workspace != nm_wksp:
fail("All npm dependencies need to come from a single workspace. Found '%s' and '%s'." % (nm_wksp, dep[NodeModuleInfo].workspace))
srcs = depset(transitive = [dep.files, srcs])
if NodeModuleSources in dep:
srcs = depset(transitive = [dep[NodeModuleSources].srcs, srcs])

if srcs:
return [NodeModuleSources(srcs = srcs)]

return []

collect_node_modules_aspect = aspect(
Expand Down
1 change: 1 addition & 0 deletions internal/rollup/BUILD.bazel
Expand Up @@ -41,6 +41,7 @@ nodejs_binary(
entry_point = "build_bazel_rules_nodejs_rollup_deps/node_modules/rollup/bin/rollup",
install_source_map_support = False,
node_modules = "@build_bazel_rules_nodejs_rollup_deps//:node_modules",
templated_args = ["--node_options=--preserve-symlinks-main"],
visibility = ["//visibility:public"],
)

Expand Down
2 changes: 2 additions & 0 deletions internal/rollup/rollup.config.js
Expand Up @@ -177,6 +177,7 @@ const config = {
module: true,
jsnext: true,
main: true,
jail: process.cwd(),
customResolveOptions: {moduleDirectory: nodeModulesRoot}
}),
amd({
Expand All @@ -193,6 +194,7 @@ const config = {
banner,
format: 'TMPL_output_format',
},
preserveSymlinks: true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Do these preserve symlinks & jail changes work as intended to keep the resolve within the cwd()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, in this particular case it was really the jail attribute that brought the non-hermetic build to fail.

}

if (enableCodeSplitting) {
Expand Down
5 changes: 4 additions & 1 deletion internal/rollup/rollup_bundle.bzl
Expand Up @@ -20,7 +20,7 @@ You do not need to install them into your project.

load("//internal/common:collect_es6_sources.bzl", _collect_es2015_sources = "collect_es6_sources")
load("//internal/common:module_mappings.bzl", "get_module_mappings")
load("//internal/common:node_module_info.bzl", "NodeModuleInfo", "collect_node_modules_aspect")
load("//internal/common:node_module_info.bzl", "NodeModuleInfo", "NodeModuleSources", "collect_node_modules_aspect")

_ROLLUP_MODULE_MAPPINGS_ATTR = "rollup_module_mappings"

Expand Down Expand Up @@ -190,6 +190,9 @@ def _run_rollup(ctx, sources, config, output, map_output = None):
# Note: we can't avoid calling .to_list() on files
direct_inputs += _filter_js_inputs(d.files.to_list())

if NodeModuleSources in d:
direct_inputs += _filter_js_inputs(d.files.to_list())

if ctx.file.license_banner:
direct_inputs += [ctx.file.license_banner]
if ctx.version_file:
Expand Down
2 changes: 2 additions & 0 deletions packages/typescript/internal/build_defs.bzl
Expand Up @@ -250,6 +250,8 @@ def _ts_library_impl(ctx):
# since they don't have the required providers.
# They were added to the action inputs for tsc_wrapped already.
# strict_deps checking currently skips node_modules.
# Make sure we are including dependencies with transitive node module
# dependencies.
# TODO(alexeagle): turn on strict deps checking when we have a real
# provider for JS/DTS inputs to ts_library.
deps = [d for d in ctx.attr.deps if not NodeModuleInfo in d],
Expand Down