fix: remove module cache in sandbox plugin

[Strum355]

This module cache came up as problematic with the presence of multiple versions of a package in a given dependency closure. In our case, we have minipass 4.2.8 and 7.0.4 (required by glob 9.3.2 and path-scurry 1.10.1 respectively) in a given closure. These are strictly not compatible with each other, so resolving minipass from glob should yield 4.2.8 and from path-scurry should yield 7.0.4. The module cache however results in everything getting the same version, as we are keying only by name sans version.

Log output I added to illustrate the point:

DEBUG NOAH BEGIN: 
        IMPORTER /home/noah/.cache/bazel/_bazel_noah/8fd1d20666a46767e7f29541678514a0/sandbox/linux-sandbox/10/execroot/__main__/bazel-out/k8-fastbuild/bin/node_modules/.aspect_rules_js/[email protected]/node_modules/glob/dist/mjs/walker.js
DEBUG NOAH RETURN RESOLVE: 
        IMPORTER /home/noah/.cache/bazel/_bazel_noah/8fd1d20666a46767e7f29541678514a0/sandbox/linux-sandbox/10/execroot/__main__/bazel-out/k8-fastbuild/bin/node_modules/.aspect_rules_js/[email protected]/node_modules/glob/dist/mjs/walker.js
        RESOLVED /home/noah/.cache/bazel/_bazel_noah/8fd1d20666a46767e7f29541678514a0/sandbox/linux-sandbox/10/execroot/__main__/bazel-out/k8-fastbuild/bin/node_modules/.aspect_rules_js/[email protected]/node_modules/minipass/index.mjs
DEBUG NOAH BEGIN: 
        IMPORTER /home/noah/.cache/bazel/_bazel_noah/8fd1d20666a46767e7f29541678514a0/sandbox/linux-sandbox/10/execroot/__main__/bazel-out/k8-fastbuild/bin/node_modules/.aspect_rules_js/[email protected]/node_modules/path-scurry/dist/mjs/index.js
DEBUG NOAH RETURN WOULD-BE CACHE: 
        IMPORTER /home/noah/.cache/bazel/_bazel_noah/8fd1d20666a46767e7f29541678514a0/sandbox/linux-sandbox/10/execroot/__main__/bazel-out/k8-fastbuild/bin/node_modules/.aspect_rules_js/[email protected]/node_modules/path-scurry/dist/mjs/index.js
        RESOLVED /home/noah/.cache/bazel/_bazel_noah/8fd1d20666a46767e7f29541678514a0/sandbox/linux-sandbox/10/execroot/__main__/bazel-out/k8-fastbuild/bin/node_modules/.aspect_rules_js/[email protected]/node_modules/minipass/index.mjs
DEBUG NOAH RETURN RESOLVE: 
        IMPORTER /home/noah/.cache/bazel/_bazel_noah/8fd1d20666a46767e7f29541678514a0/sandbox/linux-sandbox/10/execroot/__main__/bazel-out/k8-fastbuild/bin/node_modules/.aspect_rules_js/[email protected]/node_modules/path-scurry/dist/mjs/index.js
        RESOLVED /home/noah/.cache/bazel/_bazel_noah/8fd1d20666a46767e7f29541678514a0/sandbox/linux-sandbox/10/execroot/__main__/bazel-out/k8-fastbuild/bin/node_modules/.aspect_rules_js/[email protected]/node_modules/minipass/dist/esm/index.js

from the following diff to 0.19.0:

diff --git a/esbuild/private/plugins/bazel-sandbox.js b/esbuild/private/plugins/bazel-sandbox.js
index 84f0921..32ccaa7 100644
--- a/esbuild/private/plugins/bazel-sandbox.js
+++ b/esbuild/private/plugins/bazel-sandbox.js
@@ -28,15 +28,27 @@ function bazelSandboxPlugin() {
           }
           otherOptions.pluginData.executedSandboxPlugin = true
 
+          if (importPath === 'minipass') {
+            console.error(`DEBUG NOAH BEGIN: \n\tIMPORTER ${otherOptions.importer}`)
+          }
+
           // Prevent us from loading different forms of a module (CJS vs ESM).
           if (pkgImport.test(importPath)) {
-            if (!moduleCache.has(importPath)) {
+            const isNotCached = !moduleCache.has(importPath)
+            if (isNotCached) {
               moduleCache.set(
                 importPath,
                 resolveInExecroot(build, importPath, otherOptions)
               )
             }
-            return await moduleCache.get(importPath)
+            const res = await moduleCache.get(importPath)
+            if (importPath === 'minipass' && !isNotCached) {
+              console.error(`DEBUG NOAH RETURN WOULD-BE CACHE: \n\tIMPORTER ${otherOptions.importer}\n\tRESOLVED ${res.path}`)
+            }
+            // only for log output clarity
+            if (isNotCached) {
+              return res
+            }
           }
           return await resolveInExecroot(build, importPath, otherOptions)
         }
@@ -62,6 +74,14 @@ async function resolveInExecroot(build, importPath, otherOptions) {
     return result
   }
 
+  const res = correctImportPath(result, otherOptions, false)
+  if (importPath ==='minipass') {
+    console.error(`DEBUG NOAH RETURN RESOLVE: \n\tIMPORTER ${otherOptions.importer}\n\tRESOLVED ${res.path}`)
+  }
+  return res
+}
+
+function correctImportPath(result, otherOptions, firstEntry) {
   // If esbuild attempts to leave the execroot, map the path back into the execroot.
   if (!result.path.startsWith(execroot)) {
     // If it tried to leave bazel-bin, error out completely.

The one thing Im unsure about is the comment about avoiding loading both ESM and CJS, Im not sure if theres an easy reproducer somewhere or a test case in this repo that covers it cc @vpanta

---

Test plan

• Manual testing; please provide instructions so we can reproduce:

This issue was being worked on as part of a series of issues that cropped up when upgrading to 0.19.0, including https://github.com/aspect-build/rules_esbuild/pull/201. As such, the manual testing I did was the same as mentioned over there, except for a different commit: github.com/sourcegraph/sourcegraph/commit/51503969643612665485aa8e6e150566b6863d54 with target //client/web/dev:esbuild-config-production_bundle

[jbedard]

Have you been able to reproduce this in a test at all? Or any understandable way in your repo that you can describe here?

[jbedard]

@vpanta can you take a look at this and maybe try it out? I believe you had a reason for this cache originally in https://github.com/aspect-build/rules_esbuild/pull/160

[vpanta]

So just for some context, this plugin was originally something I created internally for Fullstory and decided to upstream. The module cache was added because certain modules were getting dual-loaded by esbuild depending on their import-semantics: if it came from a require, it would load the CJS version of the module, and if it was from an import statement, it would load the ESM version of the module. If I remember correctly, this was at best causing bloat in our bundle but at worst causing full breakages (when something like react would double-load). Generally FWICT it was caused by certain modules having only one form and then, when transitively loading the module A via module B's lone CJS form, module A would be duplicated as CJS.

I don't think I have explicit tests for this case, as it was something inherent to our internal build.

Take all this with a grain of salt, YMMV. I'm not sure of the best way to approach this to be honest, because your case is perfectly valid.

[Strum355]

Thanks for the reply 🙏

Copying from my conversation with Jason:

on what I believe is the last error Im coming across, Im getting a quite confusing one:

 ✘ [ERROR] Could not read from file: /home/noah/.cache/bazel/_bazel_noah/8fd1d20666a46767e7f29541678514a0/sandbox/linux-sandbox/16/execroot/__main__/bazel-out/k8-fastbuild/bin/node_modules/.aspect_rules_js/[email protected]/node_modules/object-inspect/util.inspect
 
     node_modules/.aspect_rules_js/[email protected]/node_modules/object-inspect/index.js:68:26:
       68 │ var utilInspect = require('./util.inspect');
          ╵                           ~~~~~~~~~~~~~~~~

The weird part is that <snip>/node_modules/.aspect_rules_js/[email protected]/node_modules/object-inspect/util.inspect.js (with a .js suffix) does exist, and adding the following to the bazel-sandbox plugin makes the build succeed

           const res = await resolveInExecroot(build, importPath, otherOptions)
           if (importPath.includes('util.inspect')) {
             res.path = res.path + ".js"
           }

No idea how to explain this

Does this sound similar to the issue you came across that you solved with the module cache? @vpanta

[vpanta]

First off, sorry, I saw this question and completely forgot about it. But yeah, that actually looks like some kind of ESM/CJS incompatibility? Because ESM requires file extensions but CJS can appropriately assume them. I dunno why a require call would fail without a file extension :(

Like my only thought is esbuild is applying ESM rules to what should be a CJS package.

[jbedard]

I think I'd like to merge this then. @Strum355 did you have any thoughts on a test for this or is that not worth your time right now?