diff --git a/backend/tests/bun_jobs.rs b/backend/tests/bun_jobs.rs index 8facba4a6e..e2fa85d246 100644 --- a/backend/tests/bun_jobs.rs +++ b/backend/tests/bun_jobs.rs @@ -1284,8 +1284,10 @@ fn test_generate_bun_bundle_propagates_exit_status() { /// see the named exports of a CommonJS package that builds `module.exports` /// dynamically (lodash & co.), so a named import fails to instantiate and a /// namespace import yields nothing but `default`. The bundle must import such a -/// package as a namespace and read the names off `default` instead — while -/// leaving an ESM package's named imports as the live bindings node gives it. +/// package as a namespace and read the names off `default` instead, while +/// leaving alone anything node loads as ESM — including a package whose +/// conditional exports hand bun a different file than they hand node — so its +/// named imports stay the live bindings node gives them. #[test] fn test_node_loader_cjs_named_export_interop() { use std::process::Command; @@ -1294,39 +1296,63 @@ fn test_node_loader_cjs_named_export_interop() { let temp_dir = tempfile::tempdir().unwrap(); let dir = temp_dir.path(); let dir_str = dir.to_str().unwrap(); + let write_pkg = |name: &str, files: &[(&str, &str)]| { + let pkg_dir = dir.join("node_modules").join(name); + std::fs::create_dir_all(&pkg_dir).unwrap(); + for (file, content) in files { + std::fs::write(pkg_dir.join(file), content).unwrap(); + } + }; // A CommonJS package whose exports only exist once it has run, which is // what defeats the lexer. - let pkg_dir = dir.join("node_modules").join("dyn-cjs-pkg"); - std::fs::create_dir_all(&pkg_dir).unwrap(); - std::fs::write( - pkg_dir.join("package.json"), - r#"{ "name": "dyn-cjs-pkg", "version": "1.0.0", "main": "index.js" }"#, - ) - .unwrap(); - std::fs::write( - pkg_dir.join("index.js"), - r#" + write_pkg( + "dyn-cjs-pkg", + &[ + ( + "package.json", + r#"{ "name": "dyn-cjs-pkg", "version": "1.0.0", "main": "index.js" }"#, + ), + ( + "index.js", + r#" const api = {}; ["greet"].forEach((k) => { api[k] = (s) => k + " " + s; }); module.exports = api; "#, - ) - .unwrap(); + ), + ], + ); // An ESM package whose exported binding changes after evaluation. - let esm_dir = dir.join("node_modules").join("live-esm-pkg"); - std::fs::create_dir_all(&esm_dir).unwrap(); - std::fs::write( - esm_dir.join("package.json"), - r#"{ "name": "live-esm-pkg", "version": "1.0.0", "type": "module", "main": "index.js" }"#, - ) - .unwrap(); - std::fs::write( - esm_dir.join("index.js"), - "export let count = 0;\nexport function bump() { count++; }\n", - ) - .unwrap(); + write_pkg( + "live-esm-pkg", + &[ + ( + "package.json", + r#"{ "name": "live-esm-pkg", "version": "1.0.0", "type": "module", "main": "index.js" }"#, + ), + ("index.js", "export let count = 0;\nexport function bump() { count++; }\n"), + ], + ); + + // Conditional exports that give bun CommonJS and node ESM: only node's + // answer says whether the bundle may snapshot the bindings. + write_pkg( + "dual-cond-pkg", + &[ + ( + "package.json", + r#"{ "name": "dual-cond-pkg", "version": "1.0.0", + "exports": { ".": { "bun": "./bun-cjs.js", "node": "./node-esm.mjs", "default": "./node-esm.mjs" } } }"#, + ), + ("bun-cjs.js", "module.exports = { count: 0, bump() {} };\n"), + ( + "node-esm.mjs", + "export let count = 0;\nexport function bump() { count++; }\n", + ), + ], + ); std::fs::write( dir.join("main.ts"), @@ -1334,7 +1360,12 @@ module.exports = api; import { greet } from "dyn-cjs-pkg"; import * as pkg from "dyn-cjs-pkg"; import { count, bump } from "live-esm-pkg"; -export function main() { bump(); return [greet("a"), pkg.greet("b"), count]; } +import { count as dual, bump as bumpDual } from "dual-cond-pkg"; +export function main() { + bump(); + bumpDual(); + return [greet("a"), pkg.greet("b"), { ...pkg }.greet("c"), count, dual]; +} "#, ) .unwrap(); @@ -1386,7 +1417,7 @@ console.log(JSON.stringify(Main.main())); "node rejected the bundle:\nstdout:\n{stdout}\nstderr:\n{}", String::from_utf8_lossy(&run.stderr) ); - assert_eq!(stdout.trim(), r#"["greet a","greet b",1]"#); + assert_eq!(stdout.trim(), r#"["greet a","greet b","greet c",1,1]"#); } /// Regression test for the install_bun_lockfile no-DB path: same code shape as diff --git a/backend/windmill-worker/node_cjs_interop.js b/backend/windmill-worker/node_cjs_interop.js index dc944a6ddb..18e67b58f5 100644 --- a/backend/windmill-worker/node_cjs_interop.js +++ b/backend/windmill-worker/node_cjs_interop.js @@ -12,6 +12,7 @@ import { readFileSync } from "node:fs"; import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; const WM_IDENT = "[A-Za-z_$][A-Za-z0-9_$]*"; const WM_NS_CLAUSE = `\\*\\s*as\\s+${WM_IDENT}`; @@ -20,21 +21,12 @@ const WM_CLAUSE = `(?:${WM_NS_CLAUSE}|${WM_NAMED_CLAUSE}|${WM_IDENT}(?:\\s*,\\s* const WM_ATTRS = "\\s*(?:with|assert)\\s*\\{[^{}]*\\}"; const WM_IMPORT = `(^|[;}\\n])import\\s*(?:(${WM_CLAUSE})\\s*from\\s*)?(?:"([^"\\n]*)"|'([^'\\n]*)')(${WM_ATTRS})?`; -function wmRewriteExternalImports(code, externals, jobDir) { +function wmRewriteExternalImports(code, externals, jobDir, nodePath) { if (!externals || externals.length === 0) { return code; } - const eligible = new Map(); - const needsInterop = (spec) => { - let ok = eligible.get(spec); - if (ok === undefined) { - ok = - externals.some((name) => spec === name || spec.startsWith(name + "/")) && - wmIsCommonJs(spec, jobDir); - eligible.set(spec, ok); - } - return ok; - }; + const isExternal = (spec) => + externals.some((name) => spec === name || spec.startsWith(name + "/")); // Import statements are found on a copy whose literals are blanked out, so // that generated code holding an import statement in a string is not touched. @@ -43,15 +35,15 @@ function wmRewriteExternalImports(code, externals, jobDir) { // makes the rewrite safe — it is load-bearing, not a belt-and-braces extra. const masked = wmMaskLiterals(code); const anchored = new RegExp(WM_IMPORT); - const found = []; + const matches = []; for (const hit of masked.matchAll(new RegExp(WM_IMPORT, "g"))) { const m = code.slice(hit.index, hit.index + hit[0].length).match(anchored); if (m === null || m.index !== 0) { continue; } const spec = m[3] !== undefined ? m[3] : m[4]; - if (needsInterop(spec)) { - found.push({ + if (isExternal(spec)) { + matches.push({ at: hit.index, len: hit[0].length, lead: m[1], @@ -61,6 +53,16 @@ function wmRewriteExternalImports(code, externals, jobDir) { }); } } + if (matches.length === 0) { + return code; + } + + const commonjs = wmCommonJsSpecs( + [...new Set(matches.map((m) => m.spec))], + jobDir, + nodePath + ); + const found = matches.filter((m) => commonjs.has(m.spec)); if (found.length === 0) { return code; } @@ -125,22 +127,55 @@ function wmRewriteExternalImports(code, externals, jobDir) { return ( `var ${getHelper}=(n,k,s)=>{if(k in n)return n[k];let d=Object(n.default);if(k in d)return d[k];` + "throw new SyntaxError(`The requested module '${s}' does not provide an export named '${k}'`)};" + - `var ${nsHelper}=(n)=>{let d=n.default;return d!=null&&(typeof d==="object"||typeof d==="function")` + - `?new Proxy(n,{get:(t,k,r)=>k in t?Reflect.get(t,k,r):d[k]}):n};` + + `var ${nsHelper}=(n)=>{let d=n.default;if(d==null||typeof d!=="object"&&typeof d!=="function")return n;` + + `let t={},a=(o,k)=>Object.defineProperty(t,k,{get:()=>o[k],enumerable:!0,configurable:!0});` + + `for(let k of Object.keys(d))a(d,k);for(let k of Object.keys(n))a(n,k);return t};` + out + code.slice(cursor) ); } -// Node's own rule for the format of the file a specifier resolves to: the -// extension decides, and `.js` follows the `type` of the closest package.json. -function wmIsCommonJs(spec, jobDir) { - let file; +// Node runs the bundle, and conditional exports can hand it a different file +// than they hand bun, so ask node itself where each specifier resolves. Bun's +// resolver is only the fallback for a node too old for `import.meta.resolve`. +function wmCommonJsSpecs(specs, jobDir, nodePath) { + const commonjs = new Set(); + let resolved = null; + const probe = + "const out={};" + + "for(const s of JSON.parse(process.argv[1])){try{out[s]=import.meta.resolve(s)}catch(e){out[s]=null}}" + + "console.log(JSON.stringify(out))"; try { - file = Bun.resolveSync(spec, jobDir); - } catch (err) { - return true; + const run = Bun.spawnSync([nodePath, "--input-type=module", "-e", probe, JSON.stringify(specs)], { + cwd: jobDir, + }); + if (run.success) { + resolved = JSON.parse(run.stdout.toString()); + } + } catch (err) {} + for (const spec of specs) { + let file; + if (resolved === null) { + try { + file = Bun.resolveSync(spec, jobDir); + } catch (err) { + continue; + } + } else if (resolved[spec] != null) { + file = fileURLToPath(resolved[spec]); + } else { + continue; + } + if (wmIsCommonJsFile(file)) { + commonjs.add(spec); + } } + return commonjs; +} + +// Node's own rule for a file's format: the extension decides, and `.js` follows +// the `type` of the closest package.json. +function wmIsCommonJsFile(file) { if (file.endsWith(".mjs")) { return false; } diff --git a/backend/windmill-worker/src/bun_executor.rs b/backend/windmill-worker/src/bun_executor.rs index da694f37be..e155ae1c19 100644 --- a/backend/windmill-worker/src/bun_executor.rs +++ b/backend/windmill-worker/src/bun_executor.rs @@ -912,7 +912,7 @@ if (!result?.success || !(result.outputs?.length > 0)) {{ try {{ const bundlePath = "{job_dir_js}/wrapper.js"; const bundle = await Bun.file(bundlePath).text(); - const interoped = wmRewriteExternalImports(bundle, fileNames, "{job_dir_js}"); + const interoped = wmRewriteExternalImports(bundle, fileNames, "{job_dir_js}", {node_bin}); if (interoped !== bundle) {{ await Bun.write(bundlePath, interoped); }} @@ -920,7 +920,9 @@ try {{ console.log("Failed to apply CommonJS interop to the node bundle: " + err); }} "#, - interop = NODE_CJS_INTEROP + interop = NODE_CJS_INTEROP, + node_bin = serde_json::to_string(&*NODE_BIN_PATH) + .unwrap_or_else(|_| "\"node\"".to_string()) ), )?; } else if mode == LoaderMode::Bun {