Skip to content

node:module: hand a file that bun cannot parse to a replaced module._compile - #42370

Open
robobun wants to merge 1 commit into
mainfrom
robobun/6ab4519a/compile-hook-unparseable
Open

robobun wants to merge 1 commit into
mainfrom
robobun/6ab4519a/compile-hook-unparseable

Conversation

@robobun

@robobun robobun commented Sep 11, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • With sucrase/register or esbuild-register installed, require("./comp.tsx") of a file that Bun loads fine with no hook throws AggregateError: 2 errors building "/…/comp.tsx". A Flow-typed .js file fails the same way. Node loads both.
  • These packages use the pirates shape: they wrap Module._extensions[ext], replace module._compile with their transform, and call the previous loader. For .tsx and .jsx they wrap the .js loader (Module._extensions[ext] || originalJSLoader), which forces Bun's JavaScript loader on a TSX file.
  • The built-in loader (fetchCommonJSModuleNonBuiltin<true>, src/jsc/bindings/ModuleLoader.cpp) throws the build error at if (!res->success). It only consults the replaced _compile after a successful transpile.

Fix

  • In that failure branch, when module._compile was replaced, the loader reads the file from disk and calls the replacement with the text (new builtin compileFromHijackedExtension in CommonJS.ts). Node's loader does the same: it reads the file and calls module._compile(source, filename) without parsing first.
  • If the file cannot be read, the first error is thrown unchanged. The JSON loader is excluded, because it does not use _compile in Node either.
  • Only a path that throws today changes. Files that Bun loads as ES modules still skip a replaced _compile, by design (require-extensions.test.ts, "secretly sync esm"). docs/runtime/nodejs-compat.mdx now says so.
  • Verified: test/js/node/module/require-extensions.test.ts (new test fails on 1.4.3). Also ran all of test/js/node/module/.

Background

  • Module._extensions['.js'] is the function Node calls to load a .js file. Bun's built-in entries are native and run only when user code calls them.
  • m_overriddenCompile holds the value user code assigned to module._compile on one module object.
Notes

Installed-package repro (bun add sucrase esbuild-register esbuild):

// comp.tsx
type Props = { name: string };
const greet = <T extends Props>(p: T): string => "hello " + p.name;
export const kind = "tsx";
export const out = greet({ name: "bun" });
// flow.js
// @flow
const x: number = 1;
module.exports = { kind: "flow", x };
// main.cjs
const caught = fn => { try { return fn(); } catch (e) { return "THROW " + e.name + ": " + String(e.message).split("\n")[0].slice(0, 60); } };
console.log("no hook      tsx ->", JSON.stringify(caught(() => require("./comp.tsx"))));
delete require.cache[require.resolve("./comp.tsx")];
require(process.argv[2]);
console.log("with hook    tsx ->", JSON.stringify(caught(() => require("./comp.tsx"))));
console.log("with hook   flow ->", JSON.stringify(caught(() => require("./flow.js"))));

bun main.cjs sucrase/register on 1.4.3 (same with esbuild-register):

no hook      tsx -> {"kind":"tsx","out":"hello bun"}
with hook    tsx -> "THROW AggregateError: 2 errors building \"/tmp/t19/comp.tsx\""
with hook   flow -> "THROW AggregateError: 2 errors building \"/tmp/t19/flow.js\""

This branch, and Node v26.3.0 with the hook:

with hook    tsx -> {"kind":"tsx","out":"hello bun"}
with hook   flow -> {"kind":"flow","x":1}

(esbuild-register still rejects flow.js on both runtimes. esbuild has no Flow support, and the error is esbuild's own.)

The test uses a hand-written 10 line copy of pirates.addHook(), because tests cannot install from npm. It checks that the hook receives the exact bytes on disk, that a .tsx file routed through the wrapped .js loader works, that broken JSON still throws JSON Parse error and never calls _compile, that a file that cannot be read keeps BuildMessage: ENOENT reading "missing.js", and that a hook that does not replace _compile still gets the build error. It runs in a subprocess because other tests in that file leave .js mapped to the TypeScript loader.

Scope decision: the other half of the original report (call the replaced _compile for files that Bun classifies as ES modules) is not in this PR and is not planned. #18686 chose the skip on purpose, a test pins it, and drizzle-kit <= 0.31.9 with esbuild-register works on Bun only because of it.

Self-review: checked every return null arm of Bun__transpileFile for an empty error value with no pending exception (none reachable on this path), that the new lazy function is GC-visited and initialised before builtinLoader is reachable, that fetchCommonJSModuleNonBuiltin<true> has one caller, and ran the scenario with BUN_JSC_validateExceptionChecks=1.

@robobun

robobun commented Sep 11, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: reproduced on 1.4.3 with sucrase/register and esbuild-register installed (script in the PR notes): a .tsx file that loads with no hook throws AggregateError: 2 errors building with the hook. USE_SYSTEM_BUN=1 bun test test/js/node/module/require-extensions.test.ts -t "replaced module._compile" fails, bun bd test on this branch passes, and both packages load the file on this branch as they do on Node v26.3.0.

CI (build 114417, 180 jobs passed): the new test passes on every lane. The one red job is windows 2019 x64, where test/cli/inspect/inspect.test.ts crashed in a background thread 114 ms into the run (stack in ntdll only). This diff only changes the failure branch of the built-in Module._extensions loaders, which that test does not reach.

@coderabbitai

coderabbitai Bot commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 9d034c97-28e0-434d-a23a-a777195cbef6

📥 Commits

Reviewing files that changed from the base of the PR and between 158ff6c and c44dc9d.

📒 Files selected for processing (7)
  • docs/runtime/nodejs-compat.mdx
  • src/js/builtins/CommonJS.ts
  • src/js/private.d.ts
  • src/jsc/bindings/ModuleLoader.cpp
  • src/jsc/bindings/ZigGlobalObject.h
  • src/jsc/modules/NodeModuleModule.cpp
  • test/js/node/module/require-extensions.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.


Walkthrough

The CommonJS extension loader now supports overridden module._compile hooks when transpilation fails. It reads source from disk, preserves loader-specific errors, wires the compile function through the global object, and adds integration coverage.

Changes

CommonJS extension compilation

Layer / File(s) Summary
Compile bridge and runtime wiring
src/js/private.d.ts, src/js/builtins/CommonJS.ts, src/jsc/bindings/ZigGlobalObject.h, src/jsc/modules/NodeModuleModule.cpp, docs/runtime/nodejs-compat.mdx
Declares _compile, adds the source-loading compile bridge, registers its lazy global function, and documents missing source-map support APIs.
Extension loader fallback
src/jsc/bindings/ModuleLoader.cpp
Invokes the hijacked compile function after non-JSON extension transpilation failures while preserving existing errors.
Integration validation
test/js/node/module/require-extensions.test.ts
Tests overridden compilation for JavaScript and TSX files, JSON and missing-file errors, source loading, hook counts, and unchanged compilation.

Suggested reviewers: jarred-sumner

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to c44dc

The CommonJS hook fallback preserves loader errors, excludes JSON, and is covered by integration tests for the changed loading paths.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly describes the main change: passing files that Bun cannot parse to a replaced module._compile handler.
Description check ✅ Passed The description explains the problem, fix, scope, verification steps, test coverage, and preserved behavior. It does not use the exact template headings, but it provides the required information in eq…

Comment @coderabbitai help to get the list of available commands.

…compile

When user code wraps Module._extensions['.js'] and replaces module._compile
(pirates, @babel/register, sucrase/register, esbuild-register, ts-node), the
built-in loader threw the build error before the replacement saw anything.
Those packages also wrap the '.js' loader for '.tsx' and '.jsx', which forces
the JavaScript loader, so a .tsx file that bun loads fine without the hook
failed with 'AggregateError: 2 errors building' once the hook was installed.

The built-in loaders now read the file from disk and call the replaced
module._compile with it, as Node does. If the file cannot be read, the first
error is thrown unchanged. The JSON loader is not affected.
@robobun
robobun force-pushed the robobun/6ab4519a/compile-hook-unparseable branch from c44dc9d to 5decc4f Compare September 11, 2026 19:53
@robobun

robobun commented Sep 11, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 1:47 PM PT - Sep 11th, 2026

❌ @robobun, your commit 5decc4f has 1 failures in Build #114417 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 42370

That installs a local version of the PR into your bun-42370 executable, so you can run:

bun-42370 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

@leo-cheron

Copy link
Copy Markdown

Another real-world case: Playwright Test (@playwright/test 1.63.0) hits the same failure under bun --bun, since its TS loader is pirates-shaped (playwright/lib/common/index.js, from src/transform/pirates.ts).

Any type annotation in a spec breaks it:

// e2e/d.spec.ts
import { test } from '@playwright/test'
test('probe', async () => { let r: number = 1 })
// pw.config.ts
export default { testDir: '.' }
$ bun --bun playwright test --list -c pw.config.ts
AggregateError: 2 errors building ".../d.spec.ts"
BuildMessage: Expected ";" but found ":"
Total: 0 tests in 0 files
  • Bun 1.4.2 (macOS arm64): fails as above
  • Bun 1.2.3: Total: 1 test in 1 file
  • Node 26.5: works
  • The same spec without the annotation lists fine on 1.4.2.

This blocks the "Playwright runs on Bun" path from the 1.4.0 release notes for any TypeScript suite.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants