From 0d83e892170b5cdc8f5dc09109730bc98a0b704e Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 9 Dec 2022 16:45:55 +0000 Subject: [PATCH] build: support enabling esm interop loader even outside of `defaults.bzl` (#48521) We introduced a loader that supports ESM with Bazel. This loader can only be enabled as part of our `nodejs_binary` defaults.bzl test. This works fine, but there might be other binaries/tests e.g. from `@angular/build-tooling` that should be able to use ESM & might need to for importing the `compiler-cli`. We move the logic for installing the patch into a `rules_nodejs` patch. There is an existing one that is just updated to "enable ESM support". PR Close #48521 --- WORKSPACE | 2 +- tools/defaults.bzl | 12 ++--- tools/esm-interop/esm-loaders.bzl | 26 ---------- tools/esm-interop/index.bzl | 2 - ....patch => nodejs_binary_esm_support.patch} | 47 +++++++++++++++++-- 5 files changed, 47 insertions(+), 42 deletions(-) delete mode 100644 tools/esm-interop/esm-loaders.bzl rename tools/esm-interop/patches/bazel/{nodejs_binary_mjs_execution.patch => nodejs_binary_esm_support.patch} (51%) diff --git a/WORKSPACE b/WORKSPACE index 1f2a995ae1d..f10eb6871cf 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -28,7 +28,7 @@ http_archive( patches = [ # TODO(devversion): remove when https://github.com/bazelbuild/rules_nodejs/pull/3605 is available. "//tools:bazel-repo-patches/rules_nodejs__#3605.patch", - "//tools/esm-interop:patches/nodejs_binary_mjs_execution.patch", + "//tools/esm-interop:patches/bazel/nodejs_binary_esm_support.patch", ], sha256 = "c29944ba9b0b430aadcaf3bf2570fece6fc5ebfb76df145c6cdad40d65c20811", urls = ["https://github.com/bazelbuild/rules_nodejs/releases/download/5.7.0/rules_nodejs-5.7.0.tar.gz"], diff --git a/tools/defaults.bzl b/tools/defaults.bzl index 1520aadd816..9637b22dbfe 100644 --- a/tools/defaults.bzl +++ b/tools/defaults.bzl @@ -17,7 +17,7 @@ load("@npm//@angular/build-tooling/bazel:extract_types.bzl", _extract_types = "e load("@npm//@angular/build-tooling/bazel/esbuild:index.bzl", _esbuild = "esbuild", _esbuild_config = "esbuild_config") load("@npm//tsec:index.bzl", _tsec_test = "tsec_test") load("//packages/bazel:index.bzl", _ng_module = "ng_module", _ng_package = "ng_package") -load("//tools/esm-interop:index.bzl", "enable_esm_node_module_loader", "extract_esm_outputs", "install_esm_loaders") +load("//tools/esm-interop:index.bzl", "enable_esm_node_module_loader", "extract_esm_outputs") _DEFAULT_TSCONFIG_TEST = "//packages:tsconfig-test" _INTERNAL_NG_MODULE_COMPILER = "//packages/bazel/src/ngc-wrapped" @@ -394,9 +394,6 @@ def nodejs_binary( enable_linker = False, **kwargs): npm_workspace = _node_modules_workspace_name() - rule_data = [] - - (templated_args, rule_data) = install_esm_loaders(templated_args, rule_data) if not enable_linker: templated_args = templated_args + [ @@ -415,7 +412,7 @@ def nodejs_binary( _nodejs_binary( name = name, - data = [":%s_esm_deps" % name] + rule_data + data_for_args, + data = [":%s_esm_deps" % name] + data_for_args, testonly = testonly, entry_point = entry_point.replace(".js", ".mjs"), env = env, @@ -425,9 +422,6 @@ def nodejs_binary( ) def nodejs_test(name, data = [], env = {}, data_for_args = [], templated_args = [], enable_linker = False, **kwargs): - rule_data = [] - (templated_args, rule_data) = install_esm_loaders(templated_args, rule_data) - if not enable_linker: templated_args = templated_args + [ # Disable the linker and rely on patched resolution which works better on Windows @@ -446,7 +440,7 @@ def nodejs_test(name, data = [], env = {}, data_for_args = [], templated_args = _nodejs_test( name = name, - data = [":%s_esm_deps" % name] + rule_data + data_for_args, + data = [":%s_esm_deps" % name] + data_for_args, env = env, templated_args = templated_args, use_esm = True, diff --git a/tools/esm-interop/esm-loaders.bzl b/tools/esm-interop/esm-loaders.bzl deleted file mode 100644 index 952b31e89b1..00000000000 --- a/tools/esm-interop/esm-loaders.bzl +++ /dev/null @@ -1,26 +0,0 @@ -"""ESM loader helpers.""" - -def install_esm_loaders( - templated_args, - data): - """Installs a NodeJS import loader for ESM support. Individual loades may \ - be controlled via environment variables. - - Args: - templated_args: Existing list of arguments passed to the binary/test. - data: Existing runtime dependencies for the binary/test. - - Returns: - A ruple with the updated `templated_args` and `data`. - """ - - templated_args = templated_args + [ - "--node_options=--experimental-loader=file:///$$(rlocation $(rootpath //tools/esm-interop:esm-main-loader.mjs))", - "--node_options=--no-warnings", # `--loader` is an experimental feature with warnings. - ] - data = data + [ - "//tools/esm-interop:esm-main-loader.mjs", - "//tools/esm-interop:loaders", - ] - - return (templated_args, data) diff --git a/tools/esm-interop/index.bzl b/tools/esm-interop/index.bzl index 9fd677385bd..26a54d1336d 100644 --- a/tools/esm-interop/index.bzl +++ b/tools/esm-interop/index.bzl @@ -2,9 +2,7 @@ load("@npm//@angular/build-tooling/bazel:extract_js_module_output.bzl", "extract_js_module_output") load("//tools/esm-interop:esm-node-module-loader.bzl", _enable_esm_node_module_loader = "enable_esm_node_module_loader") -load("//tools/esm-interop:esm-loaders.bzl", _install_esm_loaders = "install_esm_loaders") -install_esm_loaders = _install_esm_loaders enable_esm_node_module_loader = _enable_esm_node_module_loader def extract_esm_outputs(name, deps, testonly = False): diff --git a/tools/esm-interop/patches/bazel/nodejs_binary_mjs_execution.patch b/tools/esm-interop/patches/bazel/nodejs_binary_esm_support.patch similarity index 51% rename from tools/esm-interop/patches/bazel/nodejs_binary_mjs_execution.patch rename to tools/esm-interop/patches/bazel/nodejs_binary_esm_support.patch index 27ea32efccb..6d57e39b7b4 100644 --- a/tools/esm-interop/patches/bazel/nodejs_binary_mjs_execution.patch +++ b/tools/esm-interop/patches/bazel/nodejs_binary_esm_support.patch @@ -1,5 +1,22 @@ +diff --git internal/node/launcher.sh internal/node/launcher.sh +index 191622f..24b475e 100755 +--- internal/node/launcher.sh ++++ internal/node/launcher.sh +@@ -352,6 +352,12 @@ if [ "$PATCH_REQUIRE" = true ]; then + LAUNCHER_NODE_OPTIONS+=( "--require" "$require_patch_script" ) + fi + ++if [[ "TEMPLATED_use_esm" == "true" ]]; then ++ esmLoaderPath=$(rlocation "angular/tools/esm-interop/esm-main-loader.mjs") ++ LAUNCHER_NODE_OPTIONS+=( "--loader" "file://$esmLoaderPath" ) ++ LAUNCHER_NODE_OPTIONS+=( "--no-warnings" ) ++fi ++ + # Entry point is the user-supplied script + MAIN="${PWD}/"TEMPLATED_entry_point_execroot_path + # TODO: after we link-all-bins we should not need this extra lookup diff --git internal/node/node.bzl internal/node/node.bzl -index 3b8ce74..2e643cc 100755 +index 3b8ce74..85bc77d 100755 --- internal/node/node.bzl +++ internal/node/node.bzl @@ -93,7 +93,7 @@ def _write_require_patch_script(ctx, data, node_modules_root): @@ -25,7 +42,15 @@ index 3b8ce74..2e643cc 100755 elif entry_point_path.endswith(".tsx"): return entry_point_path[:-4] + ".js" return entry_point_path -@@ -333,8 +338,8 @@ if (process.cwd() !== __dirname) { +@@ -322,6 +327,7 @@ if (process.cwd() !== __dirname) { + "TEMPLATED_node_patches_script": _to_manifest_path(ctx, ctx.file._node_patches_script), + "TEMPLATED_require_patch_script": _to_manifest_path(ctx, ctx.outputs.require_patch_script), + "TEMPLATED_runfiles_helper_script": _to_manifest_path(ctx, ctx.file._runfile_helpers_main), ++ "TEMPLATED_use_esm": "true" if ctx.attr.use_esm else "false", + "TEMPLATED_node_tool_path": strip_external(node_toolchain.nodeinfo.target_tool_path), + "TEMPLATED_node_args": ctx.attr._node_args[UserBuildSettingInfo].value, + "TEMPLATED_chdir": chdir_script_runfiles_path if ctx.attr.chdir else "", +@@ -333,8 +339,8 @@ if (process.cwd() !== __dirname) { #else: # substitutions["TEMPLATED_script_path"] = "$(rlocation \"%s\")" % _to_manifest_path(ctx, ctx.file.entry_point) # For now we need to look in both places @@ -36,7 +61,7 @@ index 3b8ce74..2e643cc 100755 if DirectoryFilePathInfo in ctx.attr.entry_point: substitutions["TEMPLATED_entry_point_main"] = ctx.attr.entry_point[DirectoryFilePathInfo].path else: -@@ -355,7 +360,7 @@ if (process.cwd() !== __dirname) { +@@ -355,7 +361,7 @@ if (process.cwd() !== __dirname) { # Note: `to_list()` is expensive and should only be called once. sources_list = sources.to_list() @@ -45,10 +70,24 @@ index 3b8ce74..2e643cc 100755 entry_point_script = None for f in sources_list: -@@ -631,6 +636,7 @@ Predefined genrule variables are not supported in this context. +@@ -370,6 +376,9 @@ if (process.cwd() !== __dirname) { + # need to explicitly repeat the entry point in the `data` attribute. + runfiles.append(entry_point_script) + ++ if ctx.attr.use_esm: ++ node_tool_files.extend(ctx.files._esm_interop_runfiles) ++ + return [ + DefaultInfo( + executable = executable, +@@ -631,6 +640,11 @@ Predefined genrule variables are not supported in this context. ], allow_files = True, ), ++ "_esm_interop_runfiles": attr.label_list( ++ default = ["@angular//tools/esm-interop:loaders"], ++ allow_files = True, ++ ), + "use_esm": attr.bool(default = False), }