Skip to content

Commit

Permalink
refactor: make dependencies on --bazel_patch_module_resolver explicit
Browse files Browse the repository at this point in the history
We are about to flip the default for this flag in #2125 so we need to explicitly opt-in in places where we need the behavior.
Ideally most of these should be fixed, this would be necessary if we ever want to deprecate the module resolver patches
  • Loading branch information
Alex Eagle authored and gregmagolan committed Dec 11, 2020
1 parent 65f432b commit 0203c06
Show file tree
Hide file tree
Showing 19 changed files with 69 additions and 3 deletions.
2 changes: 2 additions & 0 deletions e2e/node_loader_preserve_symlinks/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,6 @@ nodejs_test(
],
entry_point = ":node_loader_test.spec.js",
node_modules = "@npm//:node_modules",
# legacy "node_modules" attribute means linker didn't see deps
templated_args = ["--bazel_patch_module_resolver"],
)
4 changes: 4 additions & 0 deletions e2e/packages/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ nodejs_test(
node_modules = "@e2e_packages_npm_install_duplicate_for_determinism_testing//:node_modules",
# TODO(gregmagolan): fix this test on windows
tags = ["fix-windows"],
# TODO: use runfiles
templated_args = ["--bazel_patch_module_resolver"],
)

nodejs_test(
Expand All @@ -52,4 +54,6 @@ nodejs_test(
node_modules = "@e2e_packages_yarn_install_duplicate_for_determinism_testing//:node_modules",
# TODO(gregmagolan): fix this test on windows
tags = ["fix-windows"],
# TODO: use runfiles
templated_args = ["--bazel_patch_module_resolver"],
)
4 changes: 4 additions & 0 deletions e2e/typescript/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ jasmine_node_test(
# Verify that worker_protocol.proto can be referenced as a target in the generated npm bazel workspace
"@npm//@bazel/typescript/third_party/github.com/bazelbuild/bazel/src/main/protobuf:worker_protocol.proto",
],
templated_args = [
# ts_library produces named AMD output with repo name in the require statement
"--bazel_patch_module_resolver",
],
deps = [
":test_lib",
],
Expand Down
2 changes: 2 additions & 0 deletions examples/angular/tools/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,7 @@ nodejs_binary(
"@npm//@bazel/typescript",
],
entry_point = "@npm//:node_modules/@bazel/typescript/internal/tsc_wrapped/tsc_wrapped.js",
# TODO: turn on --worker_sandboxing and remove this flag to see failure to load the plugin
templated_args = ["--bazel_patch_module_resolver"],
visibility = ["//:__subpackages__"],
)
6 changes: 5 additions & 1 deletion examples/kotlin/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -74,5 +74,9 @@ jasmine_node_test(
":bundle",
"@npm//domino",
],
templated_args = ["--node_options=--experimental-modules"],
templated_args = [
# TODO: don't rely on patching require()
"--bazel_patch_module_resolver",
"--node_options=--experimental-modules",
],
)
2 changes: 2 additions & 0 deletions examples/user_managed_deps/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ jasmine_node_test(
srcs = glob(["*.spec.js"]),
node_modules = "//:node_modules",
tags = ["no-local-jasmine-deps"],
# user-managed deps don't expose the provider the linker needs to make require work
templated_args = ["--bazel_patch_module_resolver"],
deps = [
":decrement",
":program",
Expand Down
9 changes: 8 additions & 1 deletion internal/node/test/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,13 @@ nodejs_binary(
)

# You can have a nodejs_binary with a node_modules attribute
# and no fine grained deps
# and no fine grained deps, but it requires patching the module resolver
nodejs_binary(
name = "has_deps_legacy",
data = ["has-deps.js"],
entry_point = ":has-deps.js",
node_modules = "@fine_grained_deps_yarn//:node_modules",
templated_args = ["--bazel_patch_module_resolver"],
)

# You can have a nodejs_binary with no node_modules attribute
Expand Down Expand Up @@ -101,6 +102,8 @@ nodejs_test(
"@npm//node_resolve_nested_main",
],
entry_point = ":module_resolution.spec.js",
# this is a test for the patched module resolver
templated_args = ["--bazel_patch_module_resolver"],
)

nodejs_test(
Expand Down Expand Up @@ -218,6 +221,8 @@ nodejs_test(
"@npm//node_resolve_nested_main",
],
entry_point = ":module_resolution_built.spec.js",
# TODO: passes locally without this flag but fails on CircleCI
templated_args = ["--bazel_patch_module_resolver"],
)

nodejs_test(
Expand All @@ -227,6 +232,8 @@ nodejs_test(
":genfile-runfile",
],
entry_point = ":data_resolution_built.spec.js",
# TODO: fails on Windows without this flag
templated_args = ["--bazel_patch_module_resolver"],
)

npm_package_bin(
Expand Down
2 changes: 2 additions & 0 deletions internal/node/test/binary_as_data/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,6 @@ nodejs_test(
name = "main_bin_data_test",
data = [":main_bin"],
entry_point = "test.js",
# TODO: fails on Windows without this flag
templated_args = ["--bazel_patch_module_resolver"],
)
2 changes: 2 additions & 0 deletions internal/npm_install/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -56,5 +56,7 @@ nodejs_binary(
"//third_party/npm/node_modules/named-amd",
],
entry_point = ":browserify-wrapped.js",
# TODO: figure out why browserify isn't resolved properly
templated_args = ["--bazel_patch_module_resolver"],
visibility = ["//visibility:public"],
)
7 changes: 7 additions & 0 deletions internal/npm_install/test/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ jasmine_node_test(
"@fine_grained_goldens//:golden_files",
"@npm//unidiff",
],
# Depends on having the .js file in source tree but resolve relative paths
# to .js files in the output tree
templated_args = ["--bazel_patch_module_resolver"],
)

nodejs_binary(
Expand Down Expand Up @@ -103,6 +106,8 @@ sh_test(
],
node_modules = "@fine_grained_deps_%s//:node_modules" % pkgmgr,
tags = ["no-local-jasmine-deps"],
# TODO: get this test running with just linker: failing under --config=no-runfiles
templated_args = ["--bazel_patch_module_resolver"],
deps = [
"@fine_grained_deps_%s//jasmine" % pkgmgr,
"@fine_grained_deps_%s//jasmine-core" % pkgmgr,
Expand All @@ -122,6 +127,8 @@ sh_test(
"fine.spec.js",
],
tags = ["no-local-jasmine-deps"],
# TODO: get this test running with just linker: failing under --config=no-runfiles
templated_args = ["--bazel_patch_module_resolver"],
deps = [
"@fine_grained_deps_%s//jasmine" % pkgmgr,
"@fine_grained_deps_%s//jasmine-core" % pkgmgr,
Expand Down
2 changes: 2 additions & 0 deletions internal/pkg_npm/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ nodejs_binary(
name = "packager",
data = ["//third_party/github.com/gjtorikian/isBinaryFile"],
entry_point = ":packager.js",
# TODO: figure out why isbinaryfile is not linked in a way this can resolve
templated_args = ["--bazel_patch_module_resolver"],
)

nodejs_binary(
Expand Down
4 changes: 4 additions & 0 deletions internal/pkg_web/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,16 @@ nodejs_binary(
],
entry_point = ":assembler.js",
node_modules = ":node_modules_none",
# TODO: figure out why isbinaryfile isn't resolved properly
templated_args = ["--bazel_patch_module_resolver"],
)

# BEGIN-INTERNAL
jasmine_node_test(
name = "assembler_test",
srcs = ["assembler_spec.js"],
# TODO: figure out why isbinaryfile isn't resolved properly
templated_args = ["--bazel_patch_module_resolver"],
deps = [
"assembler.js",
"//third_party/github.com/gjtorikian/isBinaryFile",
Expand Down
8 changes: 8 additions & 0 deletions packages/jasmine/test/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ jasmine_node_test(
# Use the generated_require.spec.js from the output tree
srcs = [":generated_require_spec"],
data = ["test.json"],
# TODO: fails under --config=no-runfiles without this flag
templated_args = ["--bazel_patch_module_resolver"],
)

copy_to_bin(
Expand Down Expand Up @@ -118,6 +120,8 @@ jasmine_node_test(
"coverage.spec.js",
":coverage_test_srcs",
],
# TODO: fails under --config=no-runfiles without this flag
templated_args = ["--bazel_patch_module_resolver"],
)

jasmine_node_test(
Expand All @@ -127,6 +131,8 @@ jasmine_node_test(
"dynamic_import.js",
],
args = [
# TODO: investigate why this fails without the patched require() function
"--bazel_patch_module_resolver",
# the --node_options arg will be consumed by the node launcher
"--node_options=--experimental-modules",
# the remaining args should be passed to the spec
Expand All @@ -147,6 +153,8 @@ jasmine_node_test(
"arg3",
],
templated_args = [
# TODO: investigate why this fails without the patched require() function
"--bazel_patch_module_resolver",
# the --node_options templated arg will be consumed by the node launcher
"--node_options=--experimental-modules",
# the remaining args should be passed to the spec
Expand Down
2 changes: 2 additions & 0 deletions packages/labs/grpc_web/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ nodejs_binary(
name = "change_import_style",
entry_point = ":change_import_style.js",
node_modules = "@build_bazel_rules_typescript_grpc_web_compiletime_deps//:node_modules",
# TODO: figure out why this doesn't resolve minimist
templated_args = ["--bazel_patch_module_resolver"],
visibility = ["//visibility:public"],
)

Expand Down
2 changes: 2 additions & 0 deletions packages/labs/test/grpc_web/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ jasmine_node_test(
"@npm//google-protobuf",
"@npm//grpc-web",
],
# TODO: make the test work with generated protobuf code that isn't runfiles-aware
templated_args = ["--bazel_patch_module_resolver"],
deps = [
":commonjs_test_lib",
],
Expand Down
6 changes: 5 additions & 1 deletion packages/node-patches/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,11 @@ nodejs_test(
],
entry_point = "@npm_node_patches//:node_modules/mocha/bin/mocha",
tags = ["fix-windows"],
templated_args = ["$$(rlocation $(rootpath %s))" % s for s in test_js] + ["--nobazel_node_patches"],
templated_args = ["$$(rlocation $(rootpath %s))" % s for s in test_js] + [
# TODO: passes locally on mac without this flag but fails on CircleCI
"--bazel_patch_module_resolver",
"--nobazel_node_patches",
],
)

rollup_bundle(
Expand Down
2 changes: 2 additions & 0 deletions packages/protractor/protractor_web_test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,8 @@ def protractor_web_test(
entry_point = Label(protractor_entry_point),
data = srcs + deps + data + [Label(d) for d in peer_deps],
testonly = 1,
# TODO: make protractor binary not depend on monkey-patched require()
templated_args = ["--bazel_patch_module_resolver"],
visibility = ["//visibility:private"],
)

Expand Down
3 changes: 3 additions & 0 deletions packages/protractor/test/protractor-utils/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -28,5 +28,8 @@ jasmine_node_test(
srcs = [":protractor_utils_tests_lib"],
data = [
":fake-devserver",
"//packages/protractor",
],
# TODO: fails under --config=no-runfiles without this flag
templated_args = ["--bazel_patch_module_resolver"],
)
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,9 @@ nodejs_binary(
],
entry_point = ":tsc_wrapped/tsc_wrapped.js",
visibility = ["//visibility:public"],
# With RBE or --worker_sandboxing you'll see that when supports_workers=True it doesn't run_node
# so it doesn't have the linker. Still needs our custom patches on require()
templated_args = ["--bazel_patch_module_resolver"],
)

ts_library(
Expand Down

0 comments on commit 0203c06

Please sign in to comment.