Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 52 additions & 1 deletion src/jsc/RuntimeTranspilerStore.rs
Original file line number Diff line number Diff line change
Expand Up @@ -892,6 +892,17 @@
}
}

// Node compile cache: record the failed module so exit-time
// persist logs the "was not initialized" skip (Node parity).
Comment thread
robobun marked this conversation as resolved.
Outdated
if crate::node_compile_cache::is_enabled()
&& loader.is_java_script_like()
&& path.is_file()
{
crate::node_compile_cache::note_parse_failure(
path.text,
!matches!(module_type, ModuleType::Esm),
);
}

Check warning on line 905 in src/jsc/RuntimeTranspilerStore.rs

View check run for this annotation

Claude / Claude Code Review

note_parse_failure on concurrent path uses package.json module_type instead of extension

The `is_cjs` argument to `note_parse_failure` here is derived from `module_type`, which on the concurrent path comes solely from the enclosing package.json's `"type"` field (via `this_tag`), whereas the sync path's `note_compile_cache_parse_failure` (jsc_hooks.rs:2613/2669) receives an extension-sniffed `module_type` (jsc_hooks.rs:4347-4362: `.cjs`/`.cts`→Cjs, `.mjs`/`.mts`→Esm regardless of package.json). So a syntactically-invalid `foo.mjs` inside a `"type":"commonjs"` package records `is_cjs=
Comment thread
robobun marked this conversation as resolved.
Outdated
self.parse_error = Some(crate::CrateError::ParseError);
return;
};
Expand Down Expand Up @@ -961,6 +972,25 @@
ptr::null_mut()
};

let is_commonjs_module = entry.metadata.module_type == CacheModuleType::Cjs;
// Node compile cache hook (transpiler-cache-hit path). UTF-16
// output would hash differently than the print path, so skip it.
Comment thread
robobun marked this conversation as resolved.
Outdated
let node_compile_cache_blob = if crate::node_compile_cache::is_enabled()
&& path.is_file()
&& loader.is_java_script_like()
&& !matches!(&entry.output_code, OutputCode::String(s) if s.is_utf16())
{
crate::node_compile_cache::fetch(
path.text,
is_commonjs_module,
entry.output_code.byte_slice(),
)
} else {
None
};
let (bytecode_cache, bytecode_cache_size) =
node_compile_cache_blob.unwrap_or((ptr::null_mut(), 0));

self.resolved_source = OwnedResolvedSource::from(ResolvedSource {
source_code: match &mut entry.output_code {
OutputCode::String(s) => *s,
Expand All @@ -970,9 +1000,11 @@
result
}
},
is_commonjs_module: entry.metadata.module_type == CacheModuleType::Cjs,
is_commonjs_module,
module_info,
tag: this_tag,
bytecode_cache,
bytecode_cache_size,
..Default::default()
});

Expand Down Expand Up @@ -1132,6 +1164,23 @@
dump_source(vm, specifier, source_code_printer);
}

// Node compile cache hook (concurrent transpile path). `fetch` copies
// the printed bytes, so replacing the print buffer below is safe.
Comment thread
robobun marked this conversation as resolved.
Outdated
let node_compile_cache_blob = if crate::node_compile_cache::is_enabled()
&& path.is_file()
&& loader.is_java_script_like()
{
crate::node_compile_cache::fetch(
path.text,
is_commonjs_module,
source_code_printer.ctx.get_written(),
)
} else {
None
};
let (bytecode_cache, bytecode_cache_size) =
node_compile_cache_blob.unwrap_or((ptr::null_mut(), 0));

let source_code = 'brk: {
let written = source_code_printer.ctx.get_written();

Expand Down Expand Up @@ -1172,6 +1221,8 @@
})
.unwrap_or(ptr::null_mut()),
tag: this_tag,
bytecode_cache,
bytecode_cache_size,
..Default::default()
});

Expand Down
3 changes: 0 additions & 3 deletions src/runtime/jsc_hooks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4401,9 +4401,6 @@ unsafe fn transpile_file(
// TODO: allow running concurrently when no onLoad handlers match a plugin.
&& plugin_runner_is_none
&& store_enabled
// With the Node compile cache enabled, transpile on-thread so the
// fetch hook sees every module.
&& !bun_jsc::node_compile_cache::is_enabled()
{
// Disgusting workaround: polyfills like
// `reflect-metadata` are CJS-with-side-effects that other ESM
Expand Down
33 changes: 33 additions & 0 deletions test/js/node/module/node-module-module.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,39 @@ describe.concurrent("node-module-module", () => {
expect(exitCode).toBe(0);
});

test("compile cache does not change ESM evaluation order", async () => {
// A `.cjs` sibling with no CommonJS features is loaded as ESM and must
// evaluate after the `.mjs` module imported before it, with or without
// the compile cache (cold and warm), and via the API enable path.
using dir = tempDir("compile-cache-eval-order", {
"main.mjs": `import "./e.mjs";\nimport "./c.cjs";\nconsole.log(globalThis.o.join(","));`,
"e.mjs": `(globalThis.o ??= []).push("esm");\nexport {};`,
"c.cjs": `(globalThis.o ??= []).push("cjs");`,
"preload.cjs": `require("node:module").enableCompileCache(__dirname + "/cc-api");`,
});
const cacheDir = path.join(String(dir), "cc");
const run = async (args, env) => {
await using proc = Bun.spawn({
cmd: [bunExe(), ...args],
env: { ...bunEnv, ...env },
cwd: String(dir),
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toBe("");
expect(exitCode).toBe(0);
return stdout.trim();
};
Comment thread
coderabbitai[bot] marked this conversation as resolved.
expect(await run(["main.mjs"], {})).toBe("esm,cjs");
expect(await run(["main.mjs"], { NODE_COMPILE_CACHE: cacheDir })).toBe("esm,cjs"); // cold
expect(await run(["main.mjs"], { NODE_COMPILE_CACHE: cacheDir })).toBe("esm,cjs"); // warm
expect(await run(["--preload", "./preload.cjs", "main.mjs"], {})).toBe("esm,cjs");
// The cached runs only prove anything if the cache was actually active.
const tagged = fs.readdirSync(cacheDir);
expect(tagged).toHaveLength(1);
expect(fs.readdirSync(path.join(cacheDir, tagged[0])).length).toBeGreaterThanOrEqual(3);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
});

test.skipIf(process.platform === "win32")(
"compile cache persists modules loaded after a non-fatal self-kill",
async () => {
Expand Down