From 3945066324304b8e2f0c4c50143aa01f6c955d12 Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Wed, 2 Dec 2020 11:34:45 -0800 Subject: [PATCH] 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 https://github.com/bazelbuild/rules_nodejs/pull/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"] Fixes #2125 --- e2e/nodejs_image/BUILD.bazel | 2 -- examples/nestjs/src/BUILD.bazel | 1 - internal/linker/test/local/BUILD.bazel | 1 - internal/linker/test/workspace_link/BUILD.bazel | 1 - internal/node/launcher.sh | 6 ++---- internal/node/test/BUILD.bazel | 9 --------- internal/npm_install/generate_build_file.ts | 9 +++------ internal/npm_install/index.js | 7 +++---- .../golden/@gregmagolan/test-a/bin/BUILD.bazel.golden | 1 - .../test/golden/@gregmagolan/test-a/index.bzl.golden | 4 ++-- .../test/golden/jasmine/bin/BUILD.bazel.golden | 1 - .../npm_install/test/golden/jasmine/index.bzl.golden | 4 ++-- internal/pkg_npm/test/directory/BUILD.bazel | 1 - internal/pkg_npm/test/linking/BUILD.bazel | 1 - internal/providers/node_runtime_deps_info.bzl | 5 ----- packages/cypress/internal/template.cypress_web_test.bzl | 2 -- packages/rollup/install.md | 1 - packages/typescript/internal/ts_project.bzl | 1 - packages/typescript/test/some_module/BUILD.bazel | 3 +-- packages/typescript/test/ts_project/a/tsconfig.json | 4 ++-- 20 files changed, 15 insertions(+), 49 deletions(-) diff --git a/e2e/nodejs_image/BUILD.bazel b/e2e/nodejs_image/BUILD.bazel index c2f9c6b35e..88ce171dde 100644 --- 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 index 28e27272a8..34027a69ad 100644 --- 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/linker/test/local/BUILD.bazel b/internal/linker/test/local/BUILD.bazel index 27712aacf6..041d7b4194 100644 --- 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 index 3dd30cd4e5..560829fd3c 100644 --- 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 index 10efb6c4e6..49756bfbb4 100644 --- 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 index 6c1d0398a3..8db589e346 100644 --- a/internal/node/test/BUILD.bazel +++ b/internal/node/test/BUILD.bazel @@ -331,14 +331,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 +483,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/generate_build_file.ts b/internal/npm_install/generate_build_file.ts index e81991f07a..c9684aea46 100644 --- 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 index c0a025b923..4479119b28 100644 --- 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/golden/@gregmagolan/test-a/bin/BUILD.bazel.golden b/internal/npm_install/test/golden/@gregmagolan/test-a/bin/BUILD.bazel.golden index 92d22a1dbf..b78ca2cd65 100644 --- 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 index bd6420ee64..a386d4ac6b 100644 --- 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 index b1b65eb5de..9748049b8a 100644 --- 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 index 95c02cce05..ec701d22ec 100644 --- 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/test/directory/BUILD.bazel b/internal/pkg_npm/test/directory/BUILD.bazel index 25501a5e65..7e3daf4d29 100644 --- 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 index f4a5673248..858ae60920 100644 --- 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/providers/node_runtime_deps_info.bzl b/internal/providers/node_runtime_deps_info.bzl index e08fef2bb8..bf22fe7a08 100644 --- 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 index 77a371ab92..6a895493ec 100644 --- 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 index 0278e6a556..8fa7b404c1 100644 --- 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=", ], ) diff --git a/packages/typescript/internal/ts_project.bzl b/packages/typescript/internal/ts_project.bzl index 7a094a5f66..9a61d1a77e 100644 --- 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 index a474e43bcf..3cb7b28c2d 100644 --- 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 index 3e6e16dcfc..2feec65a00 100644 --- 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", } -} \ No newline at end of file +}