js_run_devserver symlinks scoped node_modules to bazel-out instead of…

… runfiles (unscoped node_modules are already linked to bazel-out) (#1210)

* js_run_devserver symlinks scoped node_modules to bazel-out instead of runfiles

* Add test which confirms the single node_modules tree

* Little cleanup

* Simplify test script, exit test even if server still has connections

* Clarify comment

* Move tests to Jasmine, add explicit symlink tests

* Simplify tests to just the symlink tests

* Add js_run_devserver_internal to keep original js_run_devserver API the same

* Dedicate test target specifically to the node_modules symlinks

* Identifier name fix

* Second identifier name fix in test

* Fix docs

* Fix macro for docs update

* Update npm/private/test/repositories_checked.bzl - fixes test

* Buildifier fix for js_run_devserver_test.bzl

* Use 'tool' instead of 'command' for js_run_devserver_test

* Workaround for "js_run_devserver_internal() got multiple values for parameter 'rule_to_execute'" error

* Slight comment tweak

* Remove unnecessary js_run_devserver_internal macro

* Turns out still need to scope stardoc with the symbol name to avoid printing docs for the lib

---------

Co-authored-by: Greg Magolan <greg@aspect.dev>

.aspect/rules/external_repository_action_cache/npm_translate_lock_LTE4Nzc1MDcwNjU=

@@ -2,10 +2,10 @@
 # Input hashes for repository rule npm_translate_lock(name = "npm", pnpm_lock = "//:pnpm-lock.yaml").
 # This file should be checked into version control along with the pnpm-lock.yaml file.
 .npmrc=-2065072158
-pnpm-lock.yaml=754060271
+pnpm-lock.yaml=1169067734
 examples/npm_deps/patches/meaning-of-life@1.0.0-pnpm.patch=-442666336
 package.json=965846805
-pnpm-workspace.yaml=1536402859
+pnpm-workspace.yaml=116986059
 examples/js_binary/package.json=-41174383
 examples/macro/package.json=857146175
 examples/npm_deps/package.json=283109008
@@ -16,6 +16,7 @@ examples/webpack_cli/package.json=1911342006
 js/private/coverage/bundle/package.json=-1543718929
 js/private/image/package.json=-1260474848
 js/private/test/image/package.json=1295393035
+js/private/test/js_run_devserver/package.json=-260856079
 js/private/worker/src/package.json=1608383745
 npm/private/test/package.json=1424344949
 npm/private/test/vendored/lodash-4.17.21.tgz=-1206623349

.bazelignore

@@ -9,6 +9,7 @@ examples/webpack_cli/node_modules/
 js/private/coverage/bundle/node_modules
 js/private/image/node_modules
 js/private/test/image/node_modules
+js/private/test/js_run_devserver/node_modules
 js/private/worker/src/node_modules
 node_modules/
 npm/private/test/node_modules/

WORKSPACE

@@ -89,6 +89,7 @@ npm_translate_lock(
         "//js/private/coverage/bundle:package.json",
         "//js/private/image:package.json",
         "//js/private/test/image:package.json",
+        "//js/private/test/js_run_devserver:package.json",
         "//js/private/worker/src:package.json",
         "//npm/private/test:package.json",
         "//npm/private/test:vendored/lodash-4.17.21.tgz",

docs/BUILD.bazel

@@ -18,6 +18,7 @@ stardoc_with_diff_test(
 stardoc_with_diff_test(
     name = "js_run_devserver",
     bzl_library_target = "//js/private:js_run_devserver",
+    symbol_names = ["js_run_devserver"],
 )
 
 stardoc_with_diff_test(

js/private/js_run_devserver.bzl

@@ -95,10 +95,16 @@ def _impl(ctx):
         ),
     ]
 
-_js_run_devserver = rule(
+js_run_devserver_lib = struct(
     attrs = _attrs,
     implementation = _impl,
     toolchains = js_binary_lib.toolchains,
+)
+
+_js_run_devserver = rule(
+    attrs = js_run_devserver_lib.attrs,
+    implementation = js_run_devserver_lib.implementation,
+    toolchains = js_run_devserver_lib.toolchains,
     executable = True,
 )
 
@@ -237,7 +243,10 @@ def js_run_devserver(
     if kwargs.get("entry_point", None):
         fail("`entry_point` is set implicitly by `js_run_devserver` and cannot be overridden.")
 
-    _js_run_devserver(
+    # Allow the js_run_devserver rule to execute to be overridden for tests
+    rule_to_execute = kwargs.pop("rule_to_execute", _js_run_devserver)
+
+    rule_to_execute(
         name = name,
         enable_runfiles = select({
             "@aspect_rules_js//js:enable_runfiles": True,

js/private/js_run_devserver.mjs

@@ -33,6 +33,27 @@ function mkdirpSync(p) {
     mkdirs.add(p)
 }
 
+// Determines if a file path refers to a node module.
+//
+// Examples:
+//     isNodeModulePath('/private/var/.../node_modules/@babel/core')  // true
+//     isNodeModulePath('/private/var/.../node_modules/lodash')       // true
+//     isNodeModulePath('/private/var/.../some-file.js')              // false
+function isNodeModulePath(srcPath) {
+    const parentDir = path.dirname(srcPath);
+    const parentDirName = path.basename(parentDir);
+
+    if (parentDirName === 'node_modules') {
+        // unscoped module like 'lodash'
+        return true;
+    } else if (parentDirName.startsWith('@')) {
+        // scoped module like '@babel/core'
+        const parentParentDir = path.dirname(parentDir);
+        return path.basename(parentParentDir) === 'node_modules';
+    }
+    return false;
+}
+
 // Recursively copies a file, symlink or directory to a destination. If the file has been previously
 // synced it is only re-copied if the file's last modified time has changed since the last time that
 // file was copied. Symlinks are not copied but instead a symlink is created under the destination
@@ -52,7 +73,7 @@ async function syncRecursive(src, dst, writePerm) {
             if (process.env.JS_BINARY__LOG_DEBUG) {
                 console.error(`Syncing symlink ${srcWorkspacePath}`)
             }
-            if (path.basename(path.dirname(src)) == 'node_modules') {
+            if (isNodeModulePath(src)) {
                 // Special case for node_modules symlinks where we should _not_ symlink to the runfiles but rather
                 // the bin copy of the symlink to avoid finding npm packages in multiple node_modules trees
                 const maybeBinSrc = path.join(

js/private/test/js_run_devserver/BUILD

@@ -0,0 +1,29 @@
+load("@npm//:defs.bzl", "npm_link_all_packages")
+load("@npm//js/private/test/js_run_devserver:jasmine/package_json.bzl", jasmine_bin = "bin")
+load(":js_run_devserver_test.bzl", "js_run_devserver_test")
+
+npm_link_all_packages(name = "node_modules")
+
+# Checks node_modules symlinks that they refer to exec root (instead of runfiles)
+# in order to make sure bundlers see exactly one node_modules tree. Checks:
+#     - https://github.com/aspect-build/rules_js/pull/1043
+#     - https://github.com/aspect-build/rules_js/issues/1204
+js_run_devserver_test(
+    name = "node_modules_symlink_to_execroot_test",
+    args = ["node_modules_symlink_to_execroot.test.mjs"],
+    chdir = "js/private/test/js_run_devserver",
+    data = [
+        "node_modules_symlink_to_execroot.test.mjs",
+
+        # Some packages to link back to the exec root for the tests for
+        # https://github.com/aspect-build/rules_js/issues/1204 (and need jasmine
+        # anyway for the runner)
+        ":node_modules/@types/node",
+        ":node_modules/jasmine",
+    ],
+    tool = ":jasmine",
+)
+
+jasmine_bin.jasmine_binary(
+    name = "jasmine",
+)

js/private/test/js_run_devserver/js_run_devserver_test.bzl

@@ -0,0 +1,41 @@
+"Implementation of the js_run_devserver_test rule (the 'test' version of js_run_devserver)"
+
+load("//js/private:js_run_devserver.bzl", "js_run_devserver", "js_run_devserver_lib")
+
+# 'test' version of the _js_run_devserver` rule
+_js_run_devserver_test = rule(
+    attrs = js_run_devserver_lib.attrs,
+    implementation = js_run_devserver_lib.implementation,
+    toolchains = js_run_devserver_lib.toolchains,
+    test = True,
+)
+
+def js_run_devserver_test(
+        name,
+        tags = [],
+        **kwargs):
+    """
+    'Test' version of the `js_run_devserver` macro.
+
+    Provides the test rule to the macro, along with the 'no-sandbox' tag in
+    order to properly simulate the js_run_devserver environment of 'bazel run'.
+
+    Args:
+        name: A unique name for this target.
+
+        tags: Additional Bazel tags to supply to the rule.
+
+        **kwargs: All other args for `js_run_devserver`.
+
+            See https://docs.aspect.build/rules/aspect_rules_js/docs/js_run_devserver
+    """
+    js_run_devserver(
+        name,
+        # Override the rule to execute
+        rule_to_execute = _js_run_devserver_test,
+        # 'no-sandbox' needed to simulate 'bazel run' command - normally tests
+        # are sandboxed, but sandboxing doesn't exhibit the issue in
+        # https://github.com/aspect-build/rules_js/issues/1204
+        tags = tags + ["no-sandbox"],
+        **kwargs
+    )

js/private/test/js_run_devserver/node_modules_symlink_to_execroot.test.mjs

@@ -0,0 +1,30 @@
+import fs from 'fs'
+
+describe('node_modules symlinks >', () => {
+    const execRootDir = process.env.JS_BINARY__EXECROOT
+    const runfilesDir = process.env.JS_BINARY__RUNFILES
+
+    beforeAll(() => {
+        // first ensure that the environment variables are strings with some value
+        if (typeof execRootDir !== 'string' || execRootDir.length === 0) {
+            throw new Error(`process.env.JS_BINARY__EXECROOT is empty - can't run tests which rely on it`)
+        }
+        if (typeof runfilesDir !== 'string' || runfilesDir.length === 0) {
+            throw new Error(`process.env.JS_BINARY__RUNFILES is empty - can't run tests which rely on it`)
+        }
+    })
+
+    it(`symlinks non-scoped node_modules (e.g. 'jasmine') to the exec root instead of runfiles to ensure bundlers see a single node_modules tree (https://github.com/aspect-build/rules_js/pull/1043)`, () => {
+        const jasmineSymlink = fs.readlinkSync('./node_modules/jasmine')
+
+        expect(jasmineSymlink).toContain(execRootDir)
+        expect(jasmineSymlink).not.toContain(runfilesDir)
+    })
+
+    it(`symlinks scoped node_modules (e.g. '@types/node') to the exec root instead of runfiles to ensure bundlers see a single node_modules tree (https://github.com/aspect-build/rules_js/issues/1204)`, () => {
+        const typesNodeSymlink = fs.readlinkSync('./node_modules/@types/node')
+
+        expect(typesNodeSymlink).toContain(execRootDir)
+        expect(typesNodeSymlink).not.toContain(runfilesDir)
+    })
+})

js/private/test/js_run_devserver/package.json

@@ -0,0 +1,9 @@
+{
+    "name": "js_run_devserver_test",
+    "version": "0.0.0",
+    "private": true,
+    "dependencies": {
+        "@types/node": "16.18.11",
+        "jasmine": "5.1.0"
+    }
+}