refactor: remove bootstrap attribute & fix $(location) expansions i…

…n nodejs_binary templated_args

BREAKING CHANGE:
bootstrap attribute in nodejs_binary, nodejs_test & jasmine_node_test removed

This can be replaced with the `--node_options=--require=$(location label)` argument such as,

```
nodejs_test(
name = "bootstrap_test",
templated_args = ["--node_options=--require=$(rlocation $(location :bootstrap.js))"],
entry_point = ":bootstrap.spec.js",
data = ["bootstrap.js"],
)
```
or
```
jasmine_node_test(
name = "bootstrap_test",
srcs = ["bootstrap.spec.js"],
templated_args = ["--node_options=--require=$(rlocation $(location :bootstrap.js))"],
data = ["bootstrap.js"],
)
```

`templated_args` `$(location)` and `$(locations)` are now correctly expanded when there is no space before ` $(location`
such as `templated_args = ["--node_options=--require=$(rlocation $(location :bootstrap.js))"]`.

Path is returned in runfiles manifest path format such as `repo/path/to/file`. This differs from how $(location)
and $(locations) expansion behaves in expansion the `args` attribute of a *_binary or *_test which returns
the runfiles short path of the format `./path/to/file` for user repo and `../external_repo/path/to/file` for external
repositories. We may change this behavior in the future with $(mlocation) and $(mlocations) used to expand
to the runfiles manifest path.
See https://docs.bazel.build/versions/master/be/common-definitions.html#common-attributes-binaries.

BUILD.bazel

@@ -33,6 +33,7 @@ exports_files([
     "common.bazelrc",
     "tsconfig.json",
     "package.json",
+    "bootstrap.js",
 ])
 
 bzl_library(

bootstrap.js

@@ -0,0 +1,2 @@
+console.log('here')
+global.bootstrapped = true;

e2e/jasmine/BUILD.bazel

@@ -5,11 +5,19 @@ jasmine_node_test(
     srcs = ["test.spec.js"],
 )
 
+# This requires the linker as jasmine_shared_env_bootstrap.js requires @bazel/jasmine
+# which only works with the linker as the --require script doesn't get the require.resolve
+# patches.
 jasmine_node_test(
     name = "shared_env_test",
     srcs = ["jasmine_shared_env_test.spec.js"],
-    bootstrap = ["e2e_jasmine/jasmine_shared_env_bootstrap.js"],
     data = ["jasmine_shared_env_bootstrap.js"],
+    # TODO(gregmagolan): fix Windows error: Error: Cannot find module 'e2e_jasmine/shared_env_test_devmode_srcs.MF'
+    tags = ["fix-windows"],
+    templated_args = [
+        "--nobazel_patch_module_resolver",
+        "--node_options=--require=$(rlocation $(location :jasmine_shared_env_bootstrap.js))",
+    ],
     deps = [
         "@npm//jasmine",
         "@npm//jasmine-core",

internal/common/expand_into_runfiles.bzl

@@ -15,63 +15,82 @@
 """Helper functions to expand paths into runfiles
 """
 
-def expand_location_into_runfiles(ctx, path):
-    """Expand a path into runfiles if it contains a $(location).
-
-    If the path has a location expansion, expand it. Otherwise return as-is.
+# Expand $(location) and $(locations) to runfiles manifest path
+def _expand_mlocations(ctx, input, targets):
+    paths = ctx.expand_location(input, targets)
+    return " ".join([_short_path_to_runfiles_manifest_path(ctx, p, targets) for p in paths.split(" ")])
+
+# Convert a short_path in the execroot to the runfiles manifest path
+def _short_path_to_runfiles_manifest_path(ctx, path, targets):
+    if path.startswith("../"):
+        return path[len("../"):]
+    if path.startswith("./"):
+        path = path[len("./"):]
+    elif path.startswith(ctx.bin_dir.path):
+        path = path[len(ctx.bin_dir.path + "/"):]
+    elif path.startswith(ctx.genfiles_dir.path):
+        path = path[len(ctx.genfiles_dir.path + "/"):]
+    return ctx.workspace_name + "/" + path
+
+# Expand $(location) and $(locations) to runfiles short path
+def _expand_locations(ctx, input, targets):
+    paths = ctx.expand_location(input, targets)
+    return " ".join([_short_path_to_runfiles_short_path(ctx, p, targets) for p in paths.split(" ")])
+
+# Convert a short_path in the execroot to the runfiles short path
+def _short_path_to_runfiles_short_path(ctx, path, targets):
+    path = path.replace(ctx.bin_dir.path + "/external/", "../", 1)
+    path = path.replace(ctx.bin_dir.path + "/", "", 1)
+    path = path.replace(ctx.genfiles_dir.path + "/external/", "../", 1)
+    path = path.replace(ctx.genfiles_dir.path + "/", "", 1)
+    return path
+
+def expand_location_into_runfiles(ctx, input, targets = []):
+    """Expands all $(location ...) templates in the given string by replacing $(location //x) with the path
+    in runfiles of the output file of target //x. Expansion only works for labels that point to direct dependencies
+    of this rule or that are explicitly listed in the optional argument targets.
+
+    Path is returned in runfiles manifest path format such as `repo/path/to/file`. This differs from how $(location)
+    and $(locations) expansion behaves in expansion the `args` attribute of a *_binary or *_test which returns
+    the runfiles short path of the format `./path/to/file` for user repo and `../external_repo/path/to/file` for external
+    repositories. We may change this behavior in the future with $(mlocation) and $(mlocations) used to expand
+    to the runfiles manifest path.
+    See https://docs.bazel.build/versions/master/be/common-definitions.html#common-attributes-binaries.
 
     Args:
       ctx: context
-      path: the path to expand
+      input: String to be expanded
+      targets: List of targets for additional lookup information.
 
     Returns:
       The expanded path or the original path
     """
-    if path.find("$(location") < 0:
-        return path
-    return expand_path_into_runfiles(ctx, path)
-
-# TODO(gregmagolan): rename to _expand_path_into_runfiles after angular/angular protractor rule
-#                    is removed and no longer references this function
-def expand_path_into_runfiles(ctx, path):
-    """Expand paths into runfiles.
-
-    Given a file path that might contain a $(location) or $(locations) label expansion,
-    provide the paths to the file in runfiles.
-
-    See https://docs.bazel.build/versions/master/skylark/lib/ctx.html#expand_location
-
-    Args:
-      ctx: context
-      path: the paths to expand
-
-    Returns:
-      The expanded paths
-    """
-    targets = ctx.attr.data if hasattr(ctx.attr, "data") else []
-    expanded = ctx.expand_location(path, targets)
-
-    expansion = [_resolve_expanded_path(ctx, exp) for exp in expanded.strip().split(" ")]
-
-    return " ".join(expansion)
-
-def _resolve_expanded_path(ctx, expanded):
-    """Resolves an expanded path
-
-    Given a file path that has been expaned with $(location), resolve the path to include the workspace name,
-    handling when that path is within bin_dir or gen_fir
-
-    Args:
-      ctx: context
-      expanded: the expanded path to resolve
-
-    Returns:
-      The resolved path
-    """
-    if expanded.startswith("../"):
-        return expanded[len("../"):]
-    if expanded.startswith(ctx.bin_dir.path):
-        expanded = expanded[len(ctx.bin_dir.path + "/"):]
-    if expanded.startswith(ctx.genfiles_dir.path):
-        expanded = expanded[len(ctx.genfiles_dir.path + "/"):]
-    return ctx.workspace_name + "/" + expanded
+    target = "@%s//%s:%s" % (ctx.workspace_name, "/".join(ctx.build_file_path.split("/")[:-1]), ctx.attr.name)
+
+    # Loop through input an expand all $(location) and $(locations) using _expand_to_mlocation()
+    path = ""
+    length = len(input)
+    last = 0
+    for i in range(length):
+        if (input[i:i + 12] == "$(mlocation ") or (input[i:i + 13] == "$(mlocations "):
+            j = input.find(")", i) + 1
+            if (j == 0):
+                fail("invalid $(mlocation) expansion in string \"%s\" part of target %s" % (input, target))
+            path += input[last:i]
+            path += _expand_mlocations(ctx, "$(" + input[i + 3:j], targets)
+            last = j
+            i = j
+        if (input[i:i + 11] == "$(location ") or (input[i:i + 12] == "$(locations "):
+            j = input.find(")", i) + 1
+            if (j == 0):
+                fail("invalid $(location) expansion in string \"%s\" part of target %s" % (input, target))
+            path += input[last:i]
+
+            # TODO(gmagolan): flip to _expand_locations in the future so $(location) expands to runfiles short
+            # path which is more Bazel idiomatic and $(mlocation) can be used for runfiles manifest path
+            path += _expand_mlocations(ctx, input[i:j], targets)
+            last = j
+            i = j
+    path += input[last:]
+
+    return path

internal/node/node.bzl

@@ -109,9 +109,6 @@ def _write_loader_script(ctx):
         output = ctx.outputs.loader,
         substitutions = {
             "TEMPLATED_bin_dir": ctx.bin_dir.path,
-            "TEMPLATED_bootstrap": "\n  " + ",\n  ".join(
-                ["\"" + d + "\"" for d in ctx.attr.bootstrap],
-            ),
             "TEMPLATED_entry_point": entry_point_path,
             "TEMPLATED_gen_dir": ctx.genfiles_dir.path,
             "TEMPLATED_install_source_map_support": str(ctx.attr.install_source_map_support).lower(),
@@ -219,7 +216,7 @@ def _nodejs_binary_impl(ctx):
         # Put the params into the params file
         ctx.actions.write(
             output = ctx.outputs.templated_args_file,
-            content = "\n".join([expand_location_into_runfiles(ctx, p) for p in params]),
+            content = "\n".join([expand_location_into_runfiles(ctx, p, ctx.attr.data) for p in params]),
             is_executable = False,
         )
 
@@ -233,7 +230,7 @@ def _nodejs_binary_impl(ctx):
 
     substitutions = {
         "TEMPLATED_args": " ".join([
-            expand_location_into_runfiles(ctx, a)
+            expand_location_into_runfiles(ctx, a, ctx.attr.data)
             for a in templated_args
         ]),
         "TEMPLATED_bazel_require_script": _to_manifest_path(ctx, ctx.file._bazel_require_script),
@@ -295,13 +292,6 @@ def _nodejs_binary_impl(ctx):
     ]
 
 _NODEJS_EXECUTABLE_ATTRS = {
-    "bootstrap": attr.string_list(
-        doc = """JavaScript modules to be loaded before the entry point.
-        For example, Angular uses this to patch the Jasmine async primitives for
-        zone.js before the first `describe`.
-        """,
-        default = [],
-    ),
     "configuration_env_vars": attr.string_list(
         doc = """Pass these configuration environment variables to the resulting binary.
         Chooses a subset of the configuration environment variables (taken from `ctx.var`), which also

internal/node/node_loader.js

@@ -48,11 +48,6 @@ var MODULE_ROOTS = [
   TEMPLATED_module_roots
 ].sort((a, b) => b.module_name.toString().length - a.module_name.toString().length);
 
-/**
- * Array of bootstrap modules that need to be loaded before the entry point.
- */
-var BOOTSTRAP = [TEMPLATED_bootstrap];
-
 const USER_WORKSPACE_NAME = 'TEMPLATED_user_workspace_name';
 const NODE_MODULES_ROOT = 'TEMPLATED_node_modules_root';
 const BIN_DIR = 'TEMPLATED_bin_dir';
@@ -66,7 +61,6 @@ log_verbose(`running ${TARGET} with
   runfiles: ${process.env.RUNFILES}
 
   BIN_DIR: ${BIN_DIR}
-  BOOTSTRAP: ${JSON.stringify(BOOTSTRAP, undefined, 2)}
   ENTRY_POINT: ${ENTRY_POINT}
   GEN_DIR: ${GEN_DIR}
   INSTALL_SOURCE_MAP_SUPPORT: ${INSTALL_SOURCE_MAP_SUPPORT}
@@ -504,15 +498,6 @@ if (INSTALL_SOURCE_MAP_SUPPORT) {
       Set install_source_map_support = False in ${TARGET} to turn off this warning.`);
   }
 }
-// Load all bootstrap modules before loading the entrypoint.
-for (var i = 0; i < BOOTSTRAP.length; i++) {
-  try {
-    module.constructor._load(BOOTSTRAP[i], this);
-  } catch (e) {
-    console.error('bootstrap failure ' + e.stack || e);
-    process.exit(1);
-  }
-}
 
 if (require.main === module) {
   // Set the actual entry point in the arguments list.

internal/node/test/BUILD.bazel

@@ -99,6 +99,62 @@ nodejs_test(
     entry_point = ":data_resolution.spec.js",
 )
 
+nodejs_test(
+    name = "bootstrap_test",
+    data = ["bootstrap.js"],
+    entry_point = ":bootstrap.spec.js",
+    templated_args = ["--node_options=--require=$(rlocation $(location :bootstrap.js))"],
+)
+
+# Special case when $(location) is for a root file
+nodejs_test(
+    name = "bootstrap_root_test",
+    data = ["//:bootstrap.js"],
+    entry_point = ":bootstrap.spec.js",
+    templated_args = ["--node_options=--require=$(rlocation $(location //:bootstrap.js))"],
+)
+
+nodejs_test(
+    name = "bootstraps_test",
+    data = [
+        "bootstrap1.js",
+        "bootstrap2.js",
+    ],
+    entry_point = ":bootstraps.spec.js",
+    templated_args = [
+        "--node_options=--require=$(rlocation $(location :bootstrap1.js))",
+        "--node_options=--require=$(rlocation $(location :bootstrap2.js))",
+    ],
+)
+
+nodejs_test(
+    name = "bootstrap_mlocation_test",
+    data = ["bootstrap.js"],
+    entry_point = ":bootstrap.spec.js",
+    templated_args = ["--node_options=--require=$(rlocation $(mlocation :bootstrap.js))"],
+)
+
+# Special case when $(location) is for a root file
+nodejs_test(
+    name = "bootstrap_mlocation_root_test",
+    data = ["//:bootstrap.js"],
+    entry_point = ":bootstrap.spec.js",
+    templated_args = ["--node_options=--require=$(rlocation $(mlocation //:bootstrap.js))"],
+)
+
+nodejs_test(
+    name = "bootstraps_mlocation_test",
+    data = [
+        "bootstrap1.js",
+        "bootstrap2.js",
+    ],
+    entry_point = ":bootstraps.spec.js",
+    templated_args = [
+        "--node_options=--require=$(rlocation $(mlocation :bootstrap1.js))",
+        "--node_options=--require=$(rlocation $(mlocation :bootstrap2.js))",
+    ],
+)
+
 # Also test resolution from built files.
 copy_file(
     name = "module_resolution_lib",

internal/node/test/bootstrap.js

@@ -0,0 +1,2 @@
+console.log('here')
+global.bootstrapped = true;

internal/node/test/bootstrap.spec.js

@@ -0,0 +1,4 @@
+if (!global.bootstrapped) {
+  console.error('should run bootstrap script first');
+  process.exitCode = 1;
+}

internal/node/test/bootstrap1.js

@@ -0,0 +1,2 @@
+global.bootstrapped = global.bootstrapped ? global.bootstrapped + 1 : 1;
+global.last_bootstrap = 'bootstrap1';

internal/node/test/bootstrap2.js

@@ -0,0 +1,2 @@
+global.bootstrapped = global.bootstrapped ? global.bootstrapped + 1 : 1;
+global.last_bootstrap = 'bootstrap2';

internal/node/test/bootstraps.spec.js

@@ -0,0 +1,9 @@
+if (global.bootstrapped !== 2) {
+  console.error('should run 2 boostrap scripts');
+  process.exitCode = 1;
+}
+
+if (global.last_bootstrap !== 'bootstrap2') {
+  console.error('should run bootstrap scripts in order');
+  process.exitCode = 1;
+}