refactor: default to nobazel_patch_module_resolver

This turns off our monkey-patches for require() for nodejs_binary, nodejs_test, and macros like jasmine_node_test. Note it was previously disabled for npm_package_bin and generated index.bzl binaries in bazel-contrib#1440 BREAKING CHANGE: we no longer patch the require() function, instead you should rely on the linker to make node modules resolvable at the standard location if this breaks you, try opting-in to the require patches with templated_args = ["--bazel_patch_module_resolver"]
alexeagle · Dec 2, 2020 · 17faada · 17faada
1 parent b9dc2c1
commit 17faada
Show file tree

Hide file tree

Showing 26 changed files with 27 additions and 50 deletions.
diff --git a/e2e/nodejs_image/BUILD.bazel b/e2e/nodejs_image/BUILD.bazel
@@ -9,8 +9,6 @@ nodejs_binary(
         "@npm//date-fns",
     ],
     entry_point = "main.js",
-    # Turn off require() monkey patches so linker is required to run inside docker container
-    templated_args = ["--nobazel_patch_module_resolver"],
 )
 
 # bazel run --platforms=@build_bazel_rules_nodejs//toolchains/node:linux_amd64 //:nodejs_image

diff --git a/examples/nestjs/src/BUILD.bazel b/examples/nestjs/src/BUILD.bazel
@@ -61,7 +61,6 @@ nodejs_binary(
         "@npm//minimist",
     ],
     entry_point = ":main.ts",
-    templated_args = ["--nobazel_patch_module_resolver"],
 )
 
 jasmine_node_test(

diff --git a/internal/BUILD.bazel b/internal/BUILD.bazel
@@ -32,7 +32,10 @@ nodejs_test(
         "//:.bazelversion",
     ],
     entry_point = ":check_bazel_version.js",
-    templated_args = [BAZEL_VERSION],
+    templated_args = [
+        "--bazel_patch_module_resolver",
+        BAZEL_VERSION,
+    ],
 )
 
 # Detect if the build is running under --stamp

diff --git a/internal/linker/test/local/BUILD.bazel b/internal/linker/test/local/BUILD.bazel
@@ -4,6 +4,5 @@ jasmine_node_test(
     name = "test",
     srcs = ["test.js"],
     tags = ["local"],
-    templated_args = ["--nobazel_patch_module_resolver"],
     deps = ["//internal/linker/test/local/fit"],
 )
diff --git a/internal/linker/test/workspace_link/BUILD.bazel b/internal/linker/test/workspace_link/BUILD.bazel
@@ -4,7 +4,6 @@ jasmine_node_test(
     name = "test",
     srcs = ["test.js"],
     link_workspace_root = True,
-    templated_args = ["--nobazel_patch_module_resolver"],
     deps = [
         "//internal/linker/test/workspace_link/bar",
         "//internal/linker/test/workspace_link/foo",

diff --git a/internal/node/launcher.sh b/internal/node/launcher.sh
@@ -179,8 +179,7 @@ EXIT_CODE_CAPTURE=""
 
 RUN_LINKER=true
 NODE_PATCHES=true
-# TODO(alex): change the default to false
-PATCH_REQUIRE=true
+PATCH_REQUIRE=false
 for ARG in ${ALL_ARGS[@]+"${ALL_ARGS[@]}"}; do
   case "$ARG" in
     # Supply custom linker arguments for first-party dependencies
@@ -193,10 +192,9 @@ for ARG in ${ALL_ARGS[@]+"${ALL_ARGS[@]}"}; do
     --bazel_capture_exit_code=*) EXIT_CODE_CAPTURE="${ARG#--bazel_capture_exit_code=}" ;;
     # Disable the node_loader.js monkey patches for require()
     # Note that this means you need an explicit runfiles helper library
+    # This flag is now a no-op since the default is also false
     --nobazel_patch_module_resolver) PATCH_REQUIRE=false ;;
     # Enable the node_loader.js monkey patches for require()
-    # Currently a no-op, but specifying this makes the behavior unchanged when we update
-    # the default for PATCH_REQUIRE above
     --bazel_patch_module_resolver) PATCH_REQUIRE=true ;;
     # Disable the --require node-patches (undocumented and unused; only here as an escape value)
     --nobazel_node_patches) NODE_PATCHES=false ;;

diff --git a/internal/node/test/BUILD.bazel b/internal/node/test/BUILD.bazel
@@ -101,6 +101,7 @@ nodejs_test(
         "@npm//node_resolve_nested_main",
     ],
     entry_point = ":module_resolution.spec.js",
+    templated_args = ["--bazel_patch_module_resolver"],
 )
 
 nodejs_test(
@@ -218,6 +219,7 @@ nodejs_test(
         "@npm//node_resolve_nested_main",
     ],
     entry_point = ":module_resolution_built.spec.js",
+    templated_args = ["--bazel_patch_module_resolver"],
 )
 
 nodejs_test(
@@ -331,14 +333,6 @@ jasmine_node_test(
         "dir_output",
         "minified.js",
     ],
-    # Turn on the linker & turn off require patches so that the external workspace jasmine_node_test
-    # entry point npm/@bazel/jasmine/jasmine_runner.js's require('@bazel/jasmine') is exercised without
-    # require patches.
-    templated_args = select({
-        # TODO: fix this linker assertion on Windows
-        "@bazel_tools//src/conditions:host_windows": [],
-        "//conditions:default": ["--nobazel_patch_module_resolver"],
-    }),
 )
 
 [nodejs_toolchain_test(
@@ -491,5 +485,4 @@ nodejs_test(
     name = "main_test",
     data = [":main_lib"],
     entry_point = ":main.js",
-    templated_args = ["--nobazel_patch_module_resolver"],
 )
diff --git a/internal/npm_install/BUILD.bazel b/internal/npm_install/BUILD.bazel
@@ -56,5 +56,6 @@ nodejs_binary(
         "//third_party/npm/node_modules/named-amd",
     ],
     entry_point = ":browserify-wrapped.js",
+    templated_args = ["--bazel_patch_module_resolver"],
     visibility = ["//visibility:public"],
 )
diff --git a/internal/npm_install/generate_build_file.ts b/internal/npm_install/generate_build_file.ts
@@ -1108,8 +1108,7 @@ export function printPackageBin(pkg: Dep) {
 nodejs_binary(
     name = "${name}",
     entry_point = "//:node_modules/${pkg._dir}/${path}",
-    data = [${data.map(p => `"${p}"`).join(', ')}],
-    templated_args = ["--nobazel_patch_module_resolver"],${additionalAttributes(pkg, name)}
+    data = [${data.map(p => `"${p}"`).join(', ')}],${additionalAttributes(pkg, name)}
 )
 `;
     }
@@ -1144,8 +1143,7 @@ def ${name.replace(/-/g, '_')}(**kwargs):
         nodejs_binary(
             entry_point = "@${WORKSPACE}//:node_modules/${pkg._dir}/${path}",
             data = [${data.map(p => `"${p}"`).join(', ')}] + kwargs.pop("data", []),
-            templated_args = ["--nobazel_patch_module_resolver"] + kwargs.pop("templated_args", []),${
-          additionalAttributes(pkg, name)}
+            templated_args = kwargs.pop("templated_args", []),${additionalAttributes(pkg, name)}
             **kwargs
         )
 
@@ -1154,8 +1152,7 @@ def ${name.replace(/-/g, '_')}_test(**kwargs):
     nodejs_test(
       entry_point = "@${WORKSPACE}//:node_modules/${pkg._dir}/${path}",
       data = [${data.map(p => `"${p}"`).join(', ')}] + kwargs.pop("data", []),
-      templated_args = ["--nobazel_patch_module_resolver"] + kwargs.pop("templated_args", []),${
-          additionalAttributes(pkg, name)}
+      templated_args = kwargs.pop("templated_args", []),${additionalAttributes(pkg, name)}
       **kwargs
     )
 `;

diff --git a/internal/npm_install/index.js b/internal/npm_install/index.js
@@ -673,8 +673,7 @@ function printPackageBin(pkg) {
 nodejs_binary(
     name = "${name}",
     entry_point = "//:node_modules/${pkg._dir}/${path}",
-    data = [${data.map(p => `"${p}"`).join(', ')}],
-    templated_args = ["--nobazel_patch_module_resolver"],${additionalAttributes(pkg, name)}
+    data = [${data.map(p => `"${p}"`).join(', ')}],${additionalAttributes(pkg, name)}
 )
 `;
         }
@@ -706,7 +705,7 @@ def ${name.replace(/-/g, '_')}(**kwargs):
         nodejs_binary(
             entry_point = "@${WORKSPACE}//:node_modules/${pkg._dir}/${path}",
             data = [${data.map(p => `"${p}"`).join(', ')}] + kwargs.pop("data", []),
-            templated_args = ["--nobazel_patch_module_resolver"] + kwargs.pop("templated_args", []),${additionalAttributes(pkg, name)}
+            templated_args = kwargs.pop("templated_args", []),${additionalAttributes(pkg, name)}
             **kwargs
         )
 
@@ -715,7 +714,7 @@ def ${name.replace(/-/g, '_')}_test(**kwargs):
     nodejs_test(
       entry_point = "@${WORKSPACE}//:node_modules/${pkg._dir}/${path}",
       data = [${data.map(p => `"${p}"`).join(', ')}] + kwargs.pop("data", []),
-      templated_args = ["--nobazel_patch_module_resolver"] + kwargs.pop("templated_args", []),${additionalAttributes(pkg, name)}
+      templated_args = kwargs.pop("templated_args", []),${additionalAttributes(pkg, name)}
       **kwargs
     )
 `;

diff --git a/internal/npm_install/test/BUILD.bazel b/internal/npm_install/test/BUILD.bazel
@@ -18,6 +18,7 @@ jasmine_node_test(
         "@fine_grained_goldens//:golden_files",
         "@npm//unidiff",
     ],
+    templated_args = ["--bazel_patch_module_resolver"],
 )
 
 nodejs_binary(
@@ -81,6 +82,7 @@ jasmine_node_test(
         "//internal/npm_install:browserify-wrapped",
         "@npm//date-fns:date-fns.umd.js",
     ],
+    templated_args = ["--bazel_patch_module_resolver"],
 )
 
 sh_test(

diff --git a/internal/npm_install/test/golden/@gregmagolan/test-a/bin/BUILD.bazel.golden b/internal/npm_install/test/golden/@gregmagolan/test-a/bin/BUILD.bazel.golden
@@ -5,5 +5,4 @@ nodejs_binary(
     name = "test",
     entry_point = "//:node_modules/@gregmagolan/test-a/@bin/test.js",
     data = ["//@gregmagolan/test-a:test-a"],
-    templated_args = ["--nobazel_patch_module_resolver"],
 )
diff --git a/internal/npm_install/test/golden/@gregmagolan/test-a/index.bzl.golden b/internal/npm_install/test/golden/@gregmagolan/test-a/index.bzl.golden
@@ -7,13 +7,13 @@ def test(**kwargs):
         nodejs_binary(
             entry_point = "@fine_grained_goldens//:node_modules/@gregmagolan/test-a/@bin/test.js",
             data = ["@fine_grained_goldens//@gregmagolan/test-a:test-a"] + kwargs.pop("data", []),
-            templated_args = ["--nobazel_patch_module_resolver"] + kwargs.pop("templated_args", []),
+            templated_args = kwargs.pop("templated_args", []),
             **kwargs
         )
 def test_test(**kwargs):
     nodejs_test(
       entry_point = "@fine_grained_goldens//:node_modules/@gregmagolan/test-a/@bin/test.js",
       data = ["@fine_grained_goldens//@gregmagolan/test-a:test-a"] + kwargs.pop("data", []),
-      templated_args = ["--nobazel_patch_module_resolver"] + kwargs.pop("templated_args", []),
+      templated_args = kwargs.pop("templated_args", []),
       **kwargs
     )
diff --git a/internal/npm_install/test/golden/jasmine/bin/BUILD.bazel.golden b/internal/npm_install/test/golden/jasmine/bin/BUILD.bazel.golden
@@ -5,5 +5,4 @@ nodejs_binary(
     name = "jasmine",
     entry_point = "//:node_modules/jasmine/bin/jasmine.js",
     data = ["//jasmine:jasmine"],
-    templated_args = ["--nobazel_patch_module_resolver"],
 )
diff --git a/internal/npm_install/test/golden/jasmine/index.bzl.golden b/internal/npm_install/test/golden/jasmine/index.bzl.golden
@@ -7,13 +7,13 @@ def jasmine(**kwargs):
         nodejs_binary(
             entry_point = "@fine_grained_goldens//:node_modules/jasmine/bin/jasmine.js",
             data = ["@fine_grained_goldens//jasmine:jasmine"] + kwargs.pop("data", []),
-            templated_args = ["--nobazel_patch_module_resolver"] + kwargs.pop("templated_args", []),
+            templated_args = kwargs.pop("templated_args", []),
             **kwargs
         )
 def jasmine_test(**kwargs):
     nodejs_test(
       entry_point = "@fine_grained_goldens//:node_modules/jasmine/bin/jasmine.js",
       data = ["@fine_grained_goldens//jasmine:jasmine"] + kwargs.pop("data", []),
-      templated_args = ["--nobazel_patch_module_resolver"] + kwargs.pop("templated_args", []),
+      templated_args = kwargs.pop("templated_args", []),
       **kwargs
     )
diff --git a/internal/pkg_npm/BUILD.bazel b/internal/pkg_npm/BUILD.bazel
@@ -16,6 +16,7 @@ nodejs_binary(
     name = "packager",
     data = ["//third_party/github.com/gjtorikian/isBinaryFile"],
     entry_point = ":packager.js",
+    templated_args = ["--bazel_patch_module_resolver"],
 )
 
 nodejs_binary(

diff --git a/internal/pkg_npm/test/directory/BUILD.bazel b/internal/pkg_npm/test/directory/BUILD.bazel
@@ -22,5 +22,4 @@ nodejs_test(
         ":folder_pkg",
     ],
     entry_point = "main.js",
-    templated_args = ["--nobazel_patch_module_resolver"],
 )
diff --git a/internal/pkg_npm/test/linking/BUILD.bazel b/internal/pkg_npm/test/linking/BUILD.bazel
@@ -86,5 +86,4 @@ nodejs_test(
         "//internal/pkg_npm/test/linking/fuz:scoped_fuz",
     ],
     entry_point = ":main.js",
-    templated_args = ["--nobazel_patch_module_resolver"],
 )
diff --git a/internal/pkg_web/BUILD.bazel b/internal/pkg_web/BUILD.bazel
@@ -27,6 +27,7 @@ nodejs_binary(
     ],
     entry_point = ":assembler.js",
     node_modules = ":node_modules_none",
+    templated_args = ["--bazel_patch_module_resolver"],
 )
 
 # BEGIN-INTERNAL

diff --git a/internal/pkg_web/test2/BUILD.bazel b/internal/pkg_web/test2/BUILD.bazel
@@ -98,4 +98,5 @@ jasmine_node_test(
     tags = [
         "fix-windows",
     ],
+    templated_args = ["--bazel_patch_module_resolver"],
 )
diff --git a/internal/providers/node_runtime_deps_info.bzl b/internal/providers/node_runtime_deps_info.bzl
@@ -112,11 +112,6 @@ def run_node(ctx, inputs, arguments, executable, **kwargs):
         add_arg(arguments, "--bazel_capture_exit_code=%s" % exit_code_file.path)
         outputs = outputs + [exit_code_file]
 
-    # By using the run_node helper, you suggest that your program
-    # doesn't implicitly use runfiles to require() things
-    # To access runfiles, you must use a runfiles helper in the program instead
-    add_arg(arguments, "--nobazel_patch_module_resolver")
-
     env = kwargs.pop("env", {})
 
     # Always forward the COMPILATION_MODE to node process as an environment variable

diff --git a/packages/cypress/internal/template.cypress_web_test.bzl b/packages/cypress/internal/template.cypress_web_test.bzl
@@ -112,7 +112,6 @@ def cypress_web_test(
         ] + srcs,
         entry_point = "@build_bazel_rules_nodejs//packages/cypress:internal/run-cypress.js",
         templated_args = [
-            "--nobazel_patch_module_resolver",
             "$(rootpath {config_file})".format(config_file = config_file),
             "$(rootpath {cypress_plugin})".format(cypress_plugin = cypress_plugin),
             "$(rootpath {cypress_archive})".format(cypress_archive = cypress_archive),
@@ -154,7 +153,6 @@ def cypress_web_test_global_cache(
         ] + srcs,
         entry_point = "@build_bazel_rules_nodejs//packages/cypress:internal/run-cypress.js",
         templated_args = [
-            "--nobazel_patch_module_resolver",
             "$(rootpath {config_file})".format(config_file = config_file),
             "$(rootpath {cypress_plugin})".format(cypress_plugin = cypress_plugin),
         ] + templated_args,

diff --git a/packages/rollup/install.md b/packages/rollup/install.md
@@ -144,7 +144,6 @@ nodejs_binary(
     data = ["@npm//rollup:rollup"],
     entry_point = "@npm//:node_modules/rollup/dist/bin/rollup",
     templated_args = [
-        "--nobazel_patch_module_resolver",
         "--node_options=--max-old-space-size=<SOME_SIZE>",
     ],
 )

diff --git a/packages/typescript/internal/ts_project.bzl b/packages/typescript/internal/ts_project.bzl
@@ -644,7 +644,6 @@ def ts_project_macro(
             ],
             entry_point = Label("//packages/typescript/internal/worker:worker_adapter"),
             templated_args = [
-                "--nobazel_patch_module_resolver",
                 "$(execpath {})".format(Label(worker_tsc_bin)),
                 "--project",
                 "$(execpath {})".format(tsconfig),

diff --git a/packages/typescript/test/some_module/BUILD.bazel b/packages/typescript/test/some_module/BUILD.bazel
@@ -54,8 +54,7 @@ nodejs_binary(
 
 # custom rule using the executable above via run_node
 # invoked via bazel build //packages/typescript/test/some_module:test
-# as --nobazel_patch_module_resolver is added by default by run_node, the entry_point on the executable is resolved
-# to the ts file - writer.ts
+# the entry_point on the executable is resolved to the ts file - writer.ts
 # this works for bazel run //packages/typescript/test/some_module:writer_bin as this will patch require and resolve the
 # ts to js
 ts_write_file(

diff --git a/packages/typescript/test/ts_project/a/tsconfig.json b/packages/typescript/test/ts_project/a/tsconfig.json
@@ -12,9 +12,9 @@
         //  (05:54:49) ERROR: C:/b/bk-windows-b4qr/bazel/rules-nodejs-nodejs/packages/typescript/test/ts_project/b/BUILD.bazel:6:1: Compiling TypeScript project packages/typescript/test/ts_project/b/tsconfig.json failed (Exit 2): tsc.bat failed: error executing command
         //    cd C:/b/uuxnwop3/execroot/build_bazel_rules_nodejs
         //    SET COMPILATION_MODE=fastbuild
-        //    bazel-out/host/bin/external/npm/typescript/bin/tsc.bat -p packages/typescript/test/ts_project/b/tsconfig.json --outDir bazel-out/x64_windows-fastbuild/bin/packages/typescript/test/ts_project/b --declarationDir bazel-out/x64_windows-fastbuild/bin/packages/typescript/test/ts_project/b --bazel_node_modules_manifest=bazel-out/x64_windows-fastbuild/bin/packages/typescript/test/ts_project/b/_tsconfig.module_mappings.json --nobazel_patch_module_resolver
+        //    bazel-out/host/bin/external/npm/typescript/bin/tsc.bat -p packages/typescript/test/ts_project/b/tsconfig.json --outDir bazel-out/x64_windows-fastbuild/bin/packages/typescript/test/ts_project/b --declarationDir bazel-out/x64_windows-fastbuild/bin/packages/typescript/test/ts_project/b --bazel_node_modules_manifest=bazel-out/x64_windows-fastbuild/bin/packages/typescript/test/ts_project/b/_tsconfig.module_mappings.json
         //  Execution platform: @local_config_platform//:host
         //  packages/typescript/test/ts_project/b/b.ts(1,17): error TS6305: Output file 'C:/b/uuxnwop3/execroot/build_bazel_rules_nodejs/packages/typescript/test/ts_project/a/a.d.ts' has not been built from source file 'C:/b/uuxnwop3/execroot/build_bazel_rules_nodejs/packages/typescript/test/ts_project/a/a.ts'.
         "outDir": "../../../../../bazel-out/x64_windows-fastbuild/bin/packages/typescript/test/ts_project/a",
     }
-}
+}