From f041852fbbe023ac2f7bbe2eb4857e05bef0df27 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 21 Aug 2026 10:10:27 +0000 Subject: [PATCH] fix: do not shim npm packages whose bun and node entries can diverge Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01LwrJtHsao2FCEDH75EefzH --- backend/tests/bun_jobs.rs | 12 ++- backend/windmill-worker/src/bun_executor.rs | 102 ++++++++++++++------ 2 files changed, 80 insertions(+), 34 deletions(-) diff --git a/backend/tests/bun_jobs.rs b/backend/tests/bun_jobs.rs index e6afd54419..85e017a180 100644 --- a/backend/tests/bun_jobs.rs +++ b/backend/tests/bun_jobs.rs @@ -540,7 +540,9 @@ export function main() { } /// Node's ESM loader cannot see the named exports of a CommonJS package that cjs-module-lexer -/// fails to analyze, so `//nodejs` scripts must not leave such packages as plain externals. +/// fails to analyze, so `//nodejs` scripts must not leave such packages as plain externals. The +/// ESM-only import guards the other half: those must stay external, since `require`ing one throws +/// on the node versions without require(esm). #[sqlx::test(fixtures("base"))] async fn test_bun_nodejs_cjs_named_and_namespace_import(db: Pool) -> anyhow::Result<()> { initialize_tracing().await; @@ -550,9 +552,10 @@ async fn test_bun_nodejs_cjs_named_and_namespace_import(db: Pool) -> a let content = r#"//nodejs import { chunk } from "lodash"; import * as lodash from "lodash"; +import { nanoid } from "nanoid"; export function main() { - return [chunk([1, 2, 3, 4], 2), lodash.chunk([1, 2], 1)]; + return [chunk([1, 2, 3, 4], 2), lodash.chunk([1, 2], 1), typeof nanoid()]; } "# .to_owned(); @@ -578,7 +581,10 @@ export function main() { .json_result() .unwrap(); - assert_eq!(result, serde_json::json!([[[1, 2], [3, 4]], [[1], [2]]])); + assert_eq!( + result, + serde_json::json!([[[1, 2], [3, 4]], [[1], [2]], "string"]) + ); Ok(()) } diff --git a/backend/windmill-worker/src/bun_executor.rs b/backend/windmill-worker/src/bun_executor.rs index ac2aba7ce9..3a3ac9b90c 100644 --- a/backend/windmill-worker/src/bun_executor.rs +++ b/backend/windmill-worker/src/bun_executor.rs @@ -887,35 +887,81 @@ try {{ }} catch (e) {{ }} +const nodeModulesDir = "{job_dir_js}/node_modules"; const cjsShimDir = "{job_dir_js}/.wm_node_cjs"; +// Bun is the only one of the two that applies a "bun" export condition, so a manifest carrying one +// can point bun at a CommonJS entry where node loads ESM. Such a package cannot be classified from +// bun's resolution at all. +function usesBunCondition(exports) {{ + if (Array.isArray(exports)) {{ + return exports.some(usesBunCondition); + }} + if (exports && typeof exports === "object") {{ + return Object.keys(exports).some((key) => + key === "bun" || key === "bun-macro" || usesBunCondition(exports[key])); + }} + return false; +}} + // Node only sees the named exports of a CommonJS dependency that cjs-module-lexer finds // statically, which fails on packages such as lodash, so leaving those as plain externals breaks // `import {{ x }} from "pkg"` and `import * as pkg from "pkg"`. A generated CommonJS shim makes -// bun synthesize the interop while the package itself is still required at runtime. Packages node -// loads as ESM keep real named exports and must stay plain externals: requiring them would throw -// on the node versions without require(esm). -const isCjsCache = new Map(); -function isCjs(specifier) {{ - if (!isCjsCache.has(specifier)) {{ - let cjs = false; - try {{ - const file = Bun.resolveSync(specifier, "{job_dir_js}"); - if (file.endsWith(".cjs") || file.endsWith(".node")) {{ - cjs = true; - }} else if (file.endsWith(".js")) {{ - // Same nearest-package.json walk node does to decide how to load a bare .js - for (let dir = dirname(file); dir !== dirname(dir); dir = dirname(dir)) {{ - try {{ - cjs = JSON.parse(readFileSync(dir + "/package.json", "utf8")).type !== "module"; - break; - }} catch (e) {{}} - }} - }} - }} catch (e) {{}} - isCjsCache.set(specifier, cjs); +// bun synthesize the interop while the package itself is still required at runtime. Only packages +// proven CommonJS get one: requiring an ESM entry throws on the node versions without +// require(esm), so anything unproven keeps the plain external it had before. +const moduleKinds = new Map(); +function moduleKind(specifier) {{ + if (!moduleKinds.has(specifier)) {{ + moduleKinds.set(specifier, classifyModule(specifier)); + if (moduleKinds.get(specifier) === "unknown") {{ + console.log("could not tell whether '" + specifier + + "' is CommonJS or ESM, leaving it external: named imports from it may not resolve under node"); + }} }} - return isCjsCache.get(specifier); + return moduleKinds.get(specifier); +}} + +function classifyModule(specifier) {{ + const segments = specifier.split("/"); + const pkg = specifier.startsWith("@") ? segments.slice(0, 2).join("/") : segments[0]; + try {{ + const manifest = JSON.parse(readFileSync(nodeModulesDir + "/" + pkg + "/package.json", "utf8")); + if (manifest.bun !== undefined || usesBunCondition(manifest.exports)) {{ + return "unknown"; + }} + const file = Bun.resolveSync(specifier, "{job_dir_js}"); + if (file.endsWith(".mjs")) {{ + return "esm"; + }} + if (file.endsWith(".cjs") || file.endsWith(".node")) {{ + return "cjs"; + }} + if (file.endsWith(".js")) {{ + // Same nearest-package.json walk node does to decide how to load a bare .js + for (let dir = dirname(file); dir !== dirname(dir); dir = dirname(dir)) {{ + try {{ + return JSON.parse(readFileSync(dir + "/package.json", "utf8")).type === "module" + ? "esm" : "cjs"; + }} catch (e) {{}} + }} + }} + }} catch (e) {{}} + return "unknown"; +}} + +const cjsShims = new Map(); +function cjsShim(specifier) {{ + if (!cjsShims.has(specifier)) {{ + const shim = cjsShimDir + "/" + specifier.replace(/[^a-zA-Z0-9]/g, "_") + + "_" + Bun.hash(specifier).toString(36) + ".cjs"; + mkdirSync(cjsShimDir, {{ recursive: true }}); + // The local binding is load-bearing: bun collapses a bare `module.exports = require(x)` + // back into a passthrough external import, which is the shape that breaks node. + writeFileSync(shim, "const mod = require(" + JSON.stringify(specifier) + ");\nmodule.exports = mod;\n"); + cjsShims.set(specifier, shim); + }} + return cjsShims.get(specifier); }} const nodeExternals = {{ @@ -928,16 +974,10 @@ const nodeExternals = {{ if (!fileNames.includes(args.path.split("/")[0])) {{ return undefined; }} - if (!isCjs(args.path)) {{ + if (moduleKind(args.path) !== "cjs") {{ return {{ path: args.path, external: true }}; }} - const shim = cjsShimDir + "/" + args.path.replace(/[^a-zA-Z0-9]/g, "_") - + "_" + Bun.hash(args.path).toString(36) + ".cjs"; - mkdirSync(cjsShimDir, {{ recursive: true }}); - // The local binding is load-bearing: bun collapses a bare `module.exports = require(x)` - // back into a passthrough external import, which is the shape that breaks node. - writeFileSync(shim, "const mod = require(" + JSON.stringify(args.path) + ");\nmodule.exports = mod;\n"); - return {{ path: shim }}; + return {{ path: cjsShim(args.path) }}; }}); }}, }};