js_run_devserver symlinks scoped node_modules to bazel-out instead of runfiles (unscoped node_modules are already linked to bazel-out)

.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=-2045978816
 examples/npm_deps/patches/meaning-of-life@1.0.0-pnpm.patch=-442666336
 package.json=965846805
-pnpm-workspace.yaml=1536402859
+pnpm-workspace.yaml=793246398
 examples/js_binary/package.json=-41174383
 examples/macro/package.json=857146175
 examples/npm_deps/package.json=283109008
@@ -16,6 +16,8 @@ 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=1461460308
+js/private/test/js_run_devserver/@my-org/test-component/package.json=-913942525
 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,8 @@ 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/test/js_run_devserver/@my-org/test-component/node_modules
 js/private/worker/src/node_modules
 node_modules/
 npm/private/test/node_modules/

WORKSPACE

@@ -89,6 +89,8 @@ 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/test/js_run_devserver/@my-org/test-component:package.json",
         "//js/private/worker/src:package.json",
         "//npm/private/test:package.json",
         "//npm/private/test:vendored/lodash-4.17.21.tgz",

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,
 )
 
@@ -109,6 +115,7 @@ def js_run_devserver(
         grant_sandbox_write_permissions = False,
         use_execroot_entry_point = True,
         allow_execroot_entry_point_with_no_copy_data_to_bin = False,
+        js_run_devserver_rule = _js_run_devserver,
         **kwargs):
     """Runs a devserver via binary target or command.
 
@@ -237,7 +244,7 @@ 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(
+    js_run_devserver_rule(
         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/@my-org/test-component/BUILD

@@ -0,0 +1,26 @@
+load("@npm//:defs.bzl", "npm_link_all_packages")
+load("@aspect_rules_js//js:defs.bzl", "js_library")
+load("@aspect_rules_js//npm:defs.bzl", "npm_package")
+load("@bazel_skylib//rules:write_file.bzl", "write_file")
+
+npm_link_all_packages(name = "node_modules")
+
+npm_package(
+    name = "test-component",
+    visibility = ["//visibility:public"],
+    package = "@my-org/test-component",
+    srcs = [":library"],
+    out = "package",
+    include_runfiles = False,
+)
+
+js_library(
+    name = "library",
+    srcs = [
+        "package.json",
+        "index.jsx",
+    ],
+    deps = [
+        ":node_modules/react",
+    ]
+)

js/private/test/js_run_devserver/@my-org/test-component/index.jsx

@@ -0,0 +1,11 @@
+import React, { useState } from 'react';
+
+/**
+ * Clearly a very complex button component.
+ */
+export const TestComponent = props => {
+    // Use a hook to trigger the "multiple copies of react" issue
+    useState(42);
+
+    return React.createElement('button', null, props.children);
+};

js/private/test/js_run_devserver/@my-org/test-component/package.json

@@ -0,0 +1,11 @@
+
+{
+    "name": "@my-org/test-component",
+    "version": "0.0.0",
+    "main": "./index.jsx",
+    "module": "./index.jsx",
+    "sideEffects": false,
+    "dependencies": {
+        "react": "18.2.0"
+    }
+}

js/private/test/js_run_devserver/BUILD

@@ -0,0 +1,32 @@
+load("@npm//:defs.bzl", "npm_link_all_packages")
+load("//js:defs.bzl", "js_binary")
+load(":js_run_devserver_test.bzl", "js_run_devserver_test")
+
+npm_link_all_packages(name = "node_modules")
+
+# Test that bundlers see a single node_modules tree.
+# https://github.com/aspect-build/rules_js/issues/1204
+js_run_devserver_test(
+    name = "single_node_modules_tree_test",
+    tool = ":test_single_node_modules_tree_binary",
+    chdir = "js/private/test/js_run_devserver",
+    data = [
+        "app.jsx",
+
+        # 1st-party package
+        ":node_modules/@my-org/test-component",
+
+        ":node_modules/chalk",
+        ":node_modules/html-webpack-plugin",
+        ":node_modules/http-server",
+        ":node_modules/puppeteer",
+        ":node_modules/react",
+        ":node_modules/react-dom",
+        ":node_modules/webpack",
+    ],
+)
+
+js_binary(
+    name = "test_single_node_modules_tree_binary",
+    entry_point = "test_single_node_modules_tree.mjs",
+)

js/private/test/js_run_devserver/app.jsx

@@ -0,0 +1,15 @@
+import React from 'react';
+import { createRoot } from 'react-dom/client';
+import { TestComponent } from '@my-org/test-component'; // 1st party package
+
+const App = () => {
+    return React.createElement(TestComponent, null, 'If you see me, there is no double React issue!');
+}
+
+// -----------------
+
+const reactRootEl = document.createElement('div');
+document.body.appendChild(reactRootEl);
+
+const root = createRoot(reactRootEl);
+root.render(React.createElement(App, null, ''));

js/private/test/js_run_devserver/js_run_devserver_test.bzl

@@ -0,0 +1,24 @@
+load("//js/private:js_run_devserver.bzl", "js_run_devserver", "js_run_devserver_lib")
+
+_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,
+)
+
+# 'test' version of js_run_devserver
+def js_run_devserver_test(
+        name,
+        tags = [],
+        **kwargs):
+
+    js_run_devserver(
+        name,
+        js_run_devserver_rule = _js_run_devserver_test,
+        # 'no-sandbox' needed to simulate 'bazel run' command - normally tests 
+        # are sandboxed, and 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/package.json

@@ -0,0 +1,15 @@
+{
+    "name": "js_run_devserver_test",
+    "version": "0.0.0",
+    "private": true,
+    "dependencies": {
+        "@my-org/test-component": "workspace:*",
+        "chalk": "5.3.0",
+        "html-webpack-plugin": "5.5.3",
+        "http-server": "14.1.1",
+        "puppeteer": "21.0.3",
+        "react": "18.2.0",
+        "react-dom": "18.2.0",
+        "webpack": "5.88.2"
+    }
+}

js/private/test/js_run_devserver/test_single_node_modules_tree.mjs

@@ -0,0 +1,113 @@
+import { createServer } from 'http-server'
+import path from 'path'
+import puppeteer from 'puppeteer'
+import webpack from 'webpack'
+import HtmlWebpackPlugin from 'html-webpack-plugin'
+
+const port = 19000
+let server
+let browser
+
+try {
+    await main()
+} catch (error) {
+    console.error(error)
+    process.exit(1)
+} finally {
+    stopServer()
+    browser.close()
+}
+
+/**
+ * Performs a test to make sure only a single node_modules tree is made
+ * available in the js_run_devserver sandbox, as reported in
+ * https://github.com/aspect-build/rules_js/issues/1204
+ * 
+ * This test is done as the following:
+ * 
+ * 1. Uses Webpack to compile a very basic React app that simply displays a 
+ *    single <button> on the page. 
+ * 2. Uses http-server to serve the generated index.html file.
+ * 3. Uses puppeteer to launch Chrome and check for the <button> on the page.
+ * 
+ * If the button doesn't exist on the page, it's because multiple copies of 
+ * React have entered the bundle and are causing an error to be thrown.
+ */
+async function main() {
+    // 1. Compile app
+    await compileApp()
+
+    // 2. Start http server
+    server = await startServer()
+    process.on('SIGINT', stopServer)
+
+    // 3. Launch Chrome and look for the button
+    console.log('Starting test...')
+    browser = await puppeteer.launch({ headless: 'new' })
+    process.on('SIGINT', () => browser.close())
+    const page = await browser.newPage()
+    await page.goto(`http://localhost:${port}`)
+    try {
+        await page.waitForSelector('button', { timeout: 10000 })
+    } catch (error) {
+        console.error(
+            `Could not find the <button> element that would be displayed on the ` +
+            `page if there was no React error from multiple copies being present`
+        )
+        throw error;
+    }
+    console.log('Test complete - button component successfully found')
+}
+
+async function compileApp() {
+    return new Promise((resolve, reject) => {
+        const config = {
+            mode: 'development',
+            entry: './app',
+            output: {
+                path: path.join(process.cwd(), 'dist'),
+            },
+            resolve: {
+                extensions: ['.jsx', '.js'],
+                symlinks: true,
+            },
+            plugins: [new HtmlWebpackPlugin()],
+        }
+
+        webpack(config, (err, stats) => {
+            if (err) {
+                console.error(err.stack || err)
+                if (err.details) {
+                    console.error(err.details)
+                }
+                reject(err)
+            } else {
+                if (stats.hasErrors()) {
+                    console.error(stats.toJson().errors)
+                    reject('An error occurred during compilation - see above')
+                } else {
+                    resolve()
+                }
+            }
+        })
+    })
+}
+
+async function startServer() {
+    const server = createServer({
+        root: './dist',
+        cache: -1,
+    })
+
+    return new Promise((resolve) => {
+        server.listen(port, () => resolve(server))
+    })
+}
+
+async function stopServer() {
+    console.log(`Stopping server...`)
+    await new Promise((resolve) => {
+        server.close(resolve)
+    })
+    console.log(`Server stopped.`)
+}