fix: CJS detection regression for packages with "export" in comments (#3793)

Closes #3794

Fixes a regression from #3767 where CJS npm packages fail with `exports
is not defined` in dev mode.

- The old heuristic (`code.includes("export ")`) was fooled by the word
"export" appearing in comments of CJS files — e.g. `@opentelemetry/api`
has `// TODO: Remove ProxyTracerProvider export in the next major
version.` which caused the CJS shim to be skipped entirely
- Replaces the naive string check with two-layer detection:
1. **Package.json `type` field** (Node.js semantics): `.cjs` → always
CJS, `.js` → CJS unless nearest `package.json` has `"type": "module"`.
Results cached per directory.
2. **ESM statement regex fallback**: for dual CJS/ESM packages that ship
ESM in `.js` without `"type": "module"` (e.g. `@opentelemetry/api`,
`preact-render-to-string`), detects actual ESM syntax including minified
forms like `import{` and `export*from`
- Adds a test importing from a real CJS npm package (`qs`) in
`node_modules` — the existing "CJS test" fixtures were local ESM files
that never exercised the shim path

Author: bartlomieju

packages/plugin-vite/demo/routes/tests/cjs_npm.tsx

@@ -0,0 +1,6 @@
+import qs from "qs";
+
+export default function Page() {
+  const parsed = qs.parse("a=1&b=2");
+  return <h1>{parsed.a === "1" ? "qs-ok" : "qs-fail"}</h1>;
+}

packages/plugin-vite/src/plugins/deno.ts

@@ -23,6 +23,37 @@ export function deno(): Plugin {
 
   let isDev = false;
 
+  // Cache for package.json "type" field lookups. Per Node.js semantics,
+  // .js files in node_modules are CJS unless the nearest package.json
+  // has "type": "module".
+  const pkgTypeCache = new Map<string, boolean>();
+  async function isEsmPackage(filePath: string): Promise<boolean> {
+    let dir = path.dirname(filePath);
+    while (true) {
+      const cached = pkgTypeCache.get(dir);
+      if (cached !== undefined) return cached;
+
+      try {
+        const text = await Deno.readTextFile(path.join(dir, "package.json"));
+        const isEsm = JSON.parse(text).type === "module";
+        pkgTypeCache.set(dir, isEsm);
+        return isEsm;
+      } catch {
+        const parent = path.dirname(dir);
+        if (parent === dir) break;
+        dir = parent;
+      }
+    }
+    return false;
+  }
+
+  // Detect actual ESM export/import statements at the statement level.
+  // More robust than code.includes("export ") which matches comments
+  // and strings (e.g. "// Remove export in next version" would trick it).
+  // Handles both formatted and minified ESM (e.g. "import{" with no space).
+  const ESM_STMT_RE =
+    /(?:^|[\n;])\s*(?:export\s*[{*]|export\s+(?:default|const|let|var|function|class|async)\b|import\s*[{*"']|import\s+[a-zA-Z_$])/;
+
   return {
     name: "deno",
     sharedDuringBuild: true,
@@ -186,27 +217,33 @@ export function deno(): Plugin {
       // - SSR: module runner evaluates as ESM, needs module/exports/require
       // - Client: browser evaluates as ESM, needs module/exports
       // In build mode, Rollup's @rollup/plugin-commonjs handles CJS.
+      //
+      // CJS detection uses Node.js semantics (package.json "type" field)
+      // instead of content heuristics, which can be fooled by comments
+      // or strings containing "export"/"import".
       if (
         isDev &&
         !id.startsWith("\0") &&
         id.includes("node_modules") &&
         /\.(c?js|cjs)$/.test(id)
       ) {
-        try {
-          const code = await Deno.readTextFile(id);
-          // Quick heuristic: if file has CJS patterns and no ESM
-          if (
-            !code.includes("export ") &&
-            !code.includes("import ") &&
-            (code.includes("module.exports") ||
-              code.includes("exports.") ||
-              code.includes("require("))
-          ) {
-            const isServer = this.environment.config.consumer === "server";
-
-            if (isServer) {
-              // SSR: use Node.js createRequire for full CJS compat
-              const wrapped = `
+        // .cjs is always CJS. For .js files, check the nearest
+        // package.json "type" field first (Node.js semantics), then
+        // fall back to content-based detection for dual CJS/ESM
+        // packages that ship ESM in .js without "type": "module".
+        if (id.endsWith(".cjs") || !(await isEsmPackage(id))) {
+          try {
+            const code = await Deno.readTextFile(id);
+
+            // Skip if the file contains actual ESM syntax. Some packages
+            // (e.g. @opentelemetry/api) ship both CJS and ESM as .js
+            // without "type": "module" in package.json.
+            if (!ESM_STMT_RE.test(code)) {
+              const isServer = this.environment.config.consumer === "server";
+
+              if (isServer) {
+                // SSR: use Node.js createRequire for full CJS compat
+                const wrapped = `
 import { createRequire as __cjs_createRequire } from "node:module";
 import { fileURLToPath as __cjs_fileURLToPath } from "node:url";
 import { dirname as __cjs_dirname } from "node:path";
@@ -220,26 +257,26 @@ ${code}
 
 export default module.exports;
 `;
-              return { code: wrapped };
-            }
-
-            // Client: convert require() calls to ESM imports so
-            // browsers can load them. Hoist static require() calls
-            // to import statements at the top.
-            const imports: string[] = [];
-            let idx = 0;
-            const transformed = code.replace(
-              /\brequire\(["']([^"']+)["']\)/g,
-              (_match: string, spec: string) => {
-                const varName = `__cjs_import_${idx++}`;
-                imports.push(
-                  `import ${varName} from ${JSON.stringify(spec)};`,
-                );
-                return `(${varName}.default ?? ${varName})`;
-              },
-            );
-
-            const wrapped = `${imports.join("\n")}
+                return { code: wrapped };
+              }
+
+              // Client: convert require() calls to ESM imports so
+              // browsers can load them. Hoist static require() calls
+              // to import statements at the top.
+              const imports: string[] = [];
+              let idx = 0;
+              const transformed = code.replace(
+                /\brequire\(["']([^"']+)["']\)/g,
+                (_match: string, spec: string) => {
+                  const varName = `__cjs_import_${idx++}`;
+                  imports.push(
+                    `import ${varName} from ${JSON.stringify(spec)};`,
+                  );
+                  return `(${varName}.default ?? ${varName})`;
+                },
+              );
+
+              const wrapped = `${imports.join("\n")}
 var module = { exports: {} };
 var exports = module.exports;
 var __filename = "";
@@ -249,10 +286,11 @@ ${transformed}
 
 export default module.exports;
 `;
-            return { code: wrapped };
+              return { code: wrapped };
+            }
+          } catch {
+            // Fall through to default loading
           }
-        } catch {
-          // Fall through to default loading
         }
       }
 

packages/plugin-vite/tests/dev_server_test.ts

@@ -452,6 +452,17 @@ Deno.test({
   sanitizeResources: false,
 });
 
+Deno.test({
+  name: "vite dev - CJS npm package (qs)",
+  fn: async () => {
+    const res = await fetch(`${demoServer.address()}/tests/cjs_npm`);
+    const text = await res.text();
+    expect(text).toContain("<h1>qs-ok</h1>");
+  },
+  sanitizeOps: false,
+  sanitizeResources: false,
+});
+
 // issue: https://github.com/denoland/fresh/issues/3666
 integrationTest(
   "vite dev - basePath does not intercept Vite URLs",