fix: include transitive source in js_run_devserver sandbox (#605)

e2e/js_run_devserver/multirun_test.sh

@@ -93,7 +93,7 @@ if ! curl http://localhost:8081/index.html --fail 2>/dev/null | grep "A second l
 fi
 
 echo "<div>A new file</div>" > src/new.html
-_sedi 's#"other.html",#"other.html", "new.html",#' src/BUILD.bazel
+_sedi 's#"other.html"#"other.html", "new.html"#' src/BUILD.bazel
 
 echo "Waiting 10 seconds for ibazel rebuild after change to src/BUILD.bazel..."
 sleep 10

e2e/js_run_devserver/serve_test.sh

@@ -63,7 +63,7 @@ if ! curl http://localhost:8080/index.html --fail 2>/dev/null | grep "A second l
 fi
 
 echo "<div>A new file</div>" > src/new.html
-_sedi 's#"other.html",#"other.html", "new.html",#' src/BUILD.bazel
+_sedi 's#"other.html"#"other.html", "new.html"#' src/BUILD.bazel
 
 echo "Waiting 10 seconds for ibazel rebuild after change to src/BUILD.bazel..."
 sleep 10

e2e/js_run_devserver/src/BUILD.bazel

@@ -7,15 +7,25 @@ http_server_bin.http_server_binary(
     name = "http_server",
 )
 
+# A trivial graph of sources to include in js_run_devserver data to test
+# that transitive sources are include
+js_library(
+    name = "transitive_web_files",
+    srcs = ["other.html"],
+)
+
+js_library(
+    name = "web_files",
+    srcs = ["index.html"],
+    deps = [":transitive_web_files"],
+)
+
 # Simple js_run_devserver rule that syncs two html files and takes an http-server js_binary
 # as the tool
 js_run_devserver(
     name = "devserver",
     chdir = package_name(),
-    data = [
-        "index.html",
-        "other.html",
-    ],
+    data = [":web_files"],
     log_level = "debug",
     tool = ":http_server",
 )
@@ -47,6 +57,7 @@ js_run_devserver(
         ":http_root",
         "//:node_modules/http-server",
     ],
+    log_level = "debug",
 )
 
 # Use command to bake-in the arguments into the runnable binary so that the target
@@ -87,8 +98,8 @@ js_run_devserver(
     chdir = package_name(),
     command = "./simple.js",
     data = [
-        "index.html",
-        "other.html",
         ":simple",
+        ":web_files",
     ],
+    log_level = "debug",
 )

js/private/BUILD.bazel

@@ -127,7 +127,6 @@ bzl_library(
     deps = [
         ":js_binary",
         ":js_binary_helpers",
-        ":js_library_helpers",
         "@bazel_skylib//lib:dicts",
     ],
 )

js/private/js_run_devserver.bzl

@@ -2,7 +2,6 @@
 
 load(":js_binary.bzl", "js_binary_lib")
 load(":js_binary_helpers.bzl", _gather_files_from_js_providers = "gather_files_from_js_providers")
-load(":js_library_helpers.bzl", _gather_npm_linked_packages = "gather_npm_linked_packages")
 load("@bazel_skylib//lib:dicts.bzl", "dicts")
 
 _DOC = """Runs a devserver via binary target or command.
@@ -69,11 +68,14 @@ The devserver specified by either `tool` or `command` is run in a custom sandbox
 compatible with devserver watch modes in Node.js tools such as Webpack and Next.js.
 
 The custom sandbox is populated with the default outputs of all targets in `data`
-as well as transitive npm links.
+as well as transitive sources & npm links.
 
-rules_js npm package link targets such as `//:node_modules/next` are handled efficiently.
-Since these targets are symlinks in the output tree, they are recreated as symlinks
-in the custom sandbox and do not incur a fully copy of the underlying npm packages.
+An an optimization, virtual store files are explicitly excluded from the sandbox since the npm
+links will point to the virtual store in the execroot and Node.js will follow those links as it
+does within the execroot. As a result, rules_js npm package link targets such as
+`//:node_modules/next` are handled efficiently. Since these targets are symlinks in the output
+tree, they are recreated as symlinks in the custom sandbox and do not incur a fully copy of the
+underlying npm packages.
 
 Supports running with [ibazel](https://github.com/bazelbuild/bazel-watcher).
 Only `data` files that change on incremental builds are synchronized when running with ibazel.
@@ -116,34 +118,33 @@ def _impl(ctx):
     if ctx.attr.tool and ctx.attr.command:
         fail("Only one of tool or command may be specified")
 
+    transitive_runfiles = [_gather_files_from_js_providers(
+        targets = ctx.attr.data,
+        include_transitive_sources = ctx.attr.include_transitive_sources,
+        include_declarations = ctx.attr.include_declarations,
+        include_npm_linked_packages = ctx.attr.include_npm_linked_packages,
+    )]
+
     # The .to_list() calls here are intentional and cannot be avoided; they should be small sets of
     # files as they only include direct npm links (node_modules/foo) and the virtual store tree
     # artifacts those symlinks point to (node_modules/.aspect_rules_js/[email protected]/node_modules/foo)
-    transitive_npm_links_files = []
-    transitive_npm_links = depset(transitive = [p.files for p in _gather_npm_linked_packages(srcs = [], deps = ctx.attr.data).transitive.to_list()])
-    for f in transitive_npm_links.to_list():
+    data_files = []
+    for f in depset(transitive = transitive_runfiles + [dep.files for dep in ctx.attr.data]).to_list():
         # don't include the virtual store tree artifact; only the node_module link is needed
         if not "/.aspect_rules_js/" in f.path:
-            transitive_npm_links_files.append(f)
+            data_files.append(f)
 
     config = {
-        "data_files": [f.short_path for f in ctx.files.data + transitive_npm_links_files],
+        "data_files": [f.short_path for f in data_files],
     }
     if ctx.attr.tool:
         config["tool"] = ctx.executable.tool.short_path
     if ctx.attr.command:
         config["command"] = ctx.attr.command
     ctx.actions.write(config_file, json.encode(config))
 
-    transitive_runfiles = [_gather_files_from_js_providers(
-        targets = ctx.attr.data,
-        include_transitive_sources = ctx.attr.include_transitive_sources,
-        include_declarations = ctx.attr.include_declarations,
-        include_npm_linked_packages = ctx.attr.include_npm_linked_packages,
-    )]
     runfiles_merge_targets = ctx.attr.data[:]
     if ctx.attr.tool:
-        transitive_runfiles.append(ctx.attr.tool.files)
         runfiles_merge_targets.append(ctx.attr.tool)
 
     runfiles = ctx.runfiles(