From 2a4ba8f1be71cccf19381a2200b9a70748318cb7 Mon Sep 17 00:00:00 2001
From: Duarte Nunes <duarte@hey.com>
Date: Fri, 18 Dec 2020 11:35:02 -0300
Subject: [PATCH] feat: allow running NPM tools from execroot (#2297)

NPM tools are currently run out of external/{md}/node_modules. If the
tool loads user-provided files in the execroot, which is the common
case, then this can lead to bad interactions.

Two examples of bad interactions are:
  1. A tool that loads React and the user code too,
     hooks becoming unusable.
  2. A tool that calls into a bundler like Webpack,
     potentially bundling duplicate packages.

Signed-off-by: Duarte Nunes <duarte@hey.com>
---
 internal/node/launcher.sh | 59 ++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/internal/node/launcher.sh b/internal/node/launcher.sh
index 7fdec0d188..80579e1310 100644
--- a/internal/node/launcher.sh
+++ b/internal/node/launcher.sh
@@ -180,6 +180,7 @@ EXIT_CODE_CAPTURE=""
 RUN_LINKER=true
 NODE_PATCHES=true
 PATCH_REQUIRE=false
+FROM_EXECROOT=false
 for ARG in ${ALL_ARGS[@]+"${ALL_ARGS[@]}"}; do
   case "$ARG" in
     # Supply custom linker arguments for first-party dependencies
@@ -202,6 +203,8 @@ for ARG in ${ALL_ARGS[@]+"${ALL_ARGS[@]}"}; do
     # It also enables the --bazel_patch_module_resolver flag, as either the linker or require() patch
     # is needed for resolving third-party node modules.
     --nobazel_run_linker) RUN_LINKER=false PATCH_REQUIRE=true ;;
+    # If running an NPM package, run it from execroot instead of from external
+    --bazel_run_from_execroot) FROM_EXECROOT=true ;;
     # Let users pass through arguments to node itself
     --node_options=*) USER_NODE_OPTIONS+=( "${ARG#--node_options=}" ) ;;
     # Remaining argv is collected to pass to the program
@@ -235,29 +238,6 @@ if [ "$NODE_PATCHES" = true ]; then
   LAUNCHER_NODE_OPTIONS+=( "--require" "$node_patches_script" )
 fi
 
-if [ "$PATCH_REQUIRE" = true ]; then
-  require_patch_script=${BAZEL_NODE_PATCH_REQUIRE}
-  # See comment above
-  case "${require_patch_script}" in
-    # Absolute path on unix
-    /*          ) ;;
-    # Absolute path on Windows, e.g. C:/path/to/thing
-    [a-zA-Z]:/* ) ;;
-    # Otherwise it needs to be made relative
-    *           ) require_patch_script="./${require_patch_script}" ;;
-  esac
-  LAUNCHER_NODE_OPTIONS+=( "--require" "$require_patch_script" )
-  # Change the entry point to be the loader.js script so we run code before node
-  MAIN=$(rlocation "TEMPLATED_loader_script")
-else
-  # Entry point is the user-supplied script
-  MAIN=TEMPLATED_entry_point_execroot_path  
-  # TODO: after we link-all-bins we should not need this extra lookup
-  if [[ ! -f "$MAIN" ]]; then
-    MAIN=TEMPLATED_entry_point_manifest_path
-  fi
-fi
-
 # Tell the node_patches_script that programs should not escape the execroot
 # Bazel always sets the PWD to execroot/my_wksp so we go up one directory.
 export BAZEL_PATCH_ROOT=$(dirname $PWD)
@@ -278,14 +258,14 @@ if [[ "$PWD" == *"/bazel-out/"* ]]; then
     echo "No 'bazel-out' folder found in path '${PWD}'!"
     exit 1
   fi
-  readonly execroot=${PWD:0:${index}}
-  export BAZEL_PATCH_GUARDS="${execroot}/node_modules"
+  EXECROOT=${PWD:0:${index}}
 else
   # We are in execroot or in some other context all together such as a nodejs_image or a manually
   # run nodejs_binary. If this is execroot then linker node_modules is in the PWD. If this another
   # context then it is safe to assume the node_modules are there and guard that directory if it exists.
-  export BAZEL_PATCH_GUARDS="${PWD}/node_modules"
+  EXECROOT=${PWD}
 fi
+export BAZEL_PATCH_GUARDS="${EXECROOT}/node_modules"
 if [[ -n "${BAZEL_NODE_MODULES_ROOT:-}" ]]; then
   if [[ "${BAZEL_NODE_MODULES_ROOT}" != "${BAZEL_WORKSPACE}/node_modules" ]]; then
     # If BAZEL_NODE_MODULES_ROOT is set and it is not , add it to the list of bazel patch guards
@@ -295,6 +275,33 @@ if [[ -n "${BAZEL_NODE_MODULES_ROOT:-}" ]]; then
   fi
 fi
 
+if [ "$PATCH_REQUIRE" = true ]; then
+  require_patch_script=${BAZEL_NODE_PATCH_REQUIRE}
+  # See comment above
+  case "${require_patch_script}" in
+    # Absolute path on unix
+    /*          ) ;;
+    # Absolute path on Windows, e.g. C:/path/to/thing
+    [a-zA-Z]:/* ) ;;
+    # Otherwise it needs to be made relative
+    *           ) require_patch_script="./${require_patch_script}" ;;
+  esac
+  LAUNCHER_NODE_OPTIONS+=( "--require" "$require_patch_script" )
+  # Change the entry point to be the loader.js script so we run code before node
+  MAIN=$(rlocation "TEMPLATED_loader_script")
+else
+  # Entry point is the user-supplied script
+  MAIN=TEMPLATED_entry_point_execroot_path  
+  # TODO: after we link-all-bins we should not need this extra lookup
+  if [[ ! -f "$MAIN" ]]; then
+    if [ "$FROM_EXECROOT" = true ]; then
+      MAIN="$EXECROOT/$MAIN"
+    else
+      MAIN=TEMPLATED_entry_point_manifest_path
+    fi
+  fi
+fi
+
 # The EXPECTED_EXIT_CODE lets us write bazel tests which assert that
 # a binary fails to run. Otherwise any failure would make such a test
 # fail before we could assert that we expected that failure.