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

refactor: make dependencies on --bazel_patch_module_resolver explicit #2344

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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()
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's actual protractor and something we can't fix?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

pretty sure it's our wrapper code. I figured our protractor package isn't that popular so i didn't spend much time here digging in to figure out why it's not working, but i think this is the main one to come back to and try to fix by 3.0

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