Skip to content
Open
Show file tree
Hide file tree
Changes from 11 commits
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
10 changes: 10 additions & 0 deletions src/bun.js/bindings/JSCommonJSModule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -812,6 +812,16 @@ class JSCommonJSModulePrototype final : public JSC::JSNonFinalObject {
clientData(vm)->builtinNames().requireNativeModulePrivateName(),
0,
jsFunctionRequireNativeModule, ImplementationVisibility::Public, NoIntrinsic, JSC::PropertyAttribute::ReadOnly | JSC::PropertyAttribute::DontDelete);

// `Module.prototype.load(filename)` — needed for packages like
// `requizzle` that construct `new Module(...)` directly and call
// `.load()` on it. Mirrors Node's cjs loader semantics.
this->putDirectBuiltinFunction(
vm,
globalObject,
JSC::Identifier::fromString(vm, "load"_s),
WebCore::commonJSModulePrototypeLoadCodeGenerator(vm),
0);
}
};

Expand Down
13 changes: 12 additions & 1 deletion src/bun.js/modules/NodeModuleModule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -766,7 +766,7 @@ JSC_DEFINE_CUSTOM_SETTER(setNodeModuleWrapper,
static JSValue getModulePrototypeObject(VM& vm, JSObject* moduleObject)
{
auto* globalObject = defaultGlobalObject(moduleObject->globalObject());
auto prototype = constructEmptyObject(globalObject, globalObject->objectPrototype(), 2);
auto prototype = constructEmptyObject(globalObject, globalObject->objectPrototype(), 3);

prototype->putDirectCustomAccessor(
vm, WebCore::clientData(vm)->builtinNames().requirePublicName(),
Expand All @@ -776,6 +776,17 @@ static JSValue getModulePrototypeObject(VM& vm, JSObject* moduleObject)

prototype->putDirect(vm, Identifier::fromString(vm, "_compile"_s), globalObject->modulePrototypeUnderscoreCompileFunction());

// Also expose `load` here so `require('module').prototype.load` is a
// function, matching Node (whose `Module.prototype` IS the instance
// prototype and thus exposes both). The instance prototype
// (`JSCommonJSModulePrototype`) has its own `load` binding; this one
// is only consulted by code that reads `Module.prototype.load` off
// the constructor directly.
prototype->putDirect(
vm, Identifier::fromString(vm, "load"_s),
JSC::JSFunction::create(vm, globalObject, WebCore::commonJSModulePrototypeLoadCodeGenerator(vm), globalObject),
0);

return prototype;
}

Expand Down
64 changes: 64 additions & 0 deletions src/js/builtins/CommonJS.ts
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,70 @@ export function createRequireCache() {
return proxy;
}

// `Module.prototype.load(filename)` — used by packages like `requizzle` that
// construct `new Module(...)` directly and expect Node's module-loader shape.
// Mirrors Node's lib/internal/modules/cjs/loader.js `Module.prototype.load`.
$visibility = "Private";
export function modulePrototypeLoad(this: JSCommonJSModule, filename: string) {
// Match Node's `assert(!this.loaded, 'Module already loaded')` so a
// caller that catches the error and checks `e.code === 'ERR_ASSERTION'`
// behaves the same way on both runtimes.
const assert = require("node:assert");
assert(!this.loaded, "Module already loaded");

const Module = require("node:module");
const path = require("node:path");

// Update `filename`, `path` (= `m_dirname`, drives `__dirname`), and
// `paths` before dispatching: the .js handler goes through the native
// evaluate() path, which reads `this.path` for the module's __dirname.
// Without this, `__dirname` would stay at whatever the constructor was
// given, not where the file actually lives.
const dirname = path.dirname(filename);
this.filename = filename;
this.path = dirname;
this.paths = Module._nodeModulePaths(dirname);

// Find the longest-matching registered extension, mirroring Node's
// `findLongestRegisteredExtension` in lib/internal/modules/cjs/loader.js.
// `path.extname` only returns the trailing suffix, so it would miss
// compound extensions like `.test.js` or `.esm.js`.
const basename = path.basename(filename);
const extensions = Module._extensions;
let handler: any;
let startDot = basename.indexOf(".");
while (startDot !== -1 && startDot !== basename.length - 1) {
// Skip a leading dot so dotfiles like `.gitignore` don't match a
// handler registered for the full filename. Node's
// findLongestRegisteredExtension and Bun's native Zig equivalent
// both do this.
if (startDot === 0) {
startDot = basename.indexOf(".", 1);
continue;
}
const suffix = basename.slice(startDot);
handler = extensions[suffix];
if (handler) break;
startDot = basename.indexOf(".", startDot + 1);
}
if (!handler) {
handler = extensions[".js"];
}

// Don't let a throw from the handler leave the module permanently
// marked "loaded" — otherwise a retry would hit the assert above.
// `module._compile` sets `hasEvaluated=true` before running user code,
// which is what `loaded` reflects, so we reset it on failure.
try {
handler.$call(extensions, this, filename);
} catch (e) {
this.loaded = false;
throw e;
}

this.loaded = true;
}

type WrapperMutate = (start: string, end: string) => void;
export function getWrapperArrayProxy(onMutate: WrapperMutate) {
const wrapper = ["(function(exports,require,module,__filename,__dirname){", "})"];
Expand Down
266 changes: 266 additions & 0 deletions test/regression/issue/29253.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,266 @@
// https://github.com/oven-sh/bun/issues/29253
//
// `new Module(id, parent)` produced an instance whose prototype
// did not expose `Module.prototype.load(filename)`, so packages
// that construct a module by hand and then call `.load()` on it
// (the same pattern Node's internal cjs loader uses) threw:
//
// TypeError: targetModule.load is not a function
//
// `requizzle` — a dependency of `jsdoc` — does exactly this
// inside its `exports.load` helper, so `bun run .../jsdoc.js`
// crashed before jsdoc got a chance to run.
//
// The fix adds `Module.prototype.load` as a real method on the
// prototype backing `new Module(...)` instances, and also puts it
// on `require("module").prototype` so Node-compat property lookups
// see a function in both places. (The two objects are still
// distinct — full prototype unification is deferred because the
// existing `_compile` CustomAccessor depends on the instance cast.)
import { expect, test } from "bun:test";
import { bunEnv, bunExe, tempDir } from "harness";
import Module from "node:module";

test("new Module() instances inherit load() (#29253)", () => {
// The ticket: `targetModule.load(targetModule.id)` on a freshly
// constructed Module was throwing "load is not a function" because
// the instance prototype had no `load` method.
const m = new Module("/tmp/does-not-matter-29253.js", null);
expect(typeof m.load).toBe("function");

// And it should be inherited from the prototype chain, not an own
// property on every instance (which would be wasteful).
expect(Object.prototype.hasOwnProperty.call(m, "load")).toBe(false);
expect(typeof Object.getPrototypeOf(m).load).toBe("function");

// `require("module").prototype` is a separate disposable object from
// the instance prototype (see the header comment). It also needs to
// expose `load` so code that does `typeof Module.prototype.load` or
// `Module.prototype.load = wrapper` sees a function. If the C++
// registration in getModulePrototypeObject were reverted, the other
// assertions above would still pass — so guard that code path too.
expect(typeof Module.prototype.load).toBe("function");
});

test.concurrent("new Module().load(filename) reads and evaluates the file (#29253)", async () => {
// Spawn a separate Bun so the test doesn't pollute its own
// require cache or Module.wrap state.
using dir = tempDir("issue-29253-load", {
"target.js": `
module.exports = { answer: 42, filename: __filename, dirname: __dirname };
`,
"driver.js": `
const Module = require("node:module");
const path = require("node:path");
const target = path.resolve(__dirname, "target.js");

const m = new Module(target, module);
m.load(target);

// After load(): the file has been read, wrapped, and
// executed. The module's exports must be the object the
// file assigned to module.exports, and the bookkeeping
// fields must be populated the way Node does.
console.log(JSON.stringify({
loaded: m.loaded,
filename: m.filename,
exports: m.exports,
}));
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "driver.js"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

const result = JSON.parse(stdout.trim());
expect(result.loaded).toBe(true);
expect(result.filename).toMatch(/target\.js$/);
expect(result.exports.answer).toBe(42);
expect(result.exports.filename).toBe(result.filename);
expect(exitCode).toBe(0);

Check warning on line 87 in test/regression/issue/29253.test.ts

View check run for this annotation

Claude / Claude Code Review

Missing __dirname assertion leaves this.path = dirname uncovered

The second subprocess test exports `dirname: __dirname` from target.js but the assertion block never checks `result.exports.dirname`, leaving `this.path = dirname` in `modulePrototypeLoad` completely uncovered. Because every test constructs `new Module(target, module)` with the same path passed to `m.load(target)`, the C++ constructor already initializes `this.path` correctly before `load()` runs, so removing `this.path = dirname` from `modulePrototypeLoad` would leave all 5 tests green while si
});

test.concurrent("Module.prototype.load honors an overridden Module.wrapper (#29253)", async () => {
// `load()` must compile the file through the CURRENT module
// wrapper (`Module.wrapper[0] + source + Module.wrapper[1]`)
// — not a hard-coded one. Mutating the wrapper array is how
// Bun exposes Node's wrapper-override hook.
using dir = tempDir("issue-29253-wrap", {
"target.js": `module.exports = { wrappedVar: typeof __swizzled };`,
"driver.js": `
const Module = require("node:module");
const path = require("node:path");
const originalWrapper0 = Module.wrapper[0];

// Inject a local 'const __swizzled = 1;' at the top of
// the module scope; if the wrapper is honored, the module
// sees typeof __swizzled === "number".
Module.wrapper[0] = originalWrapper0 + "const __swizzled = 1;\\n";

try {
const target = path.resolve(__dirname, "target.js");
const m = new Module(target, module);
m.load(target);
console.log(m.exports.wrappedVar);
} finally {
Module.wrapper[0] = originalWrapper0;
}
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "driver.js"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stdout.trim()).toBe("number");
expect(exitCode).toBe(0);
});

test.concurrent("new Module().load populates filename/paths/loaded (#29253)", async () => {
// Node's `Module.prototype.load` writes `filename`, `paths`,
// and `loaded` before returning. `requizzle` and any other
// package that reads those fields after `.load()` depends on
// this, even if it doesn't touch the wrapper.
using dir = tempDir("issue-29253-fields", {
"leaf.js": `module.exports = 'ok';`,
"driver.js": `
const Module = require("node:module");
const path = require("node:path");

const target = path.resolve(__dirname, "leaf.js");
const m = new Module(target, module);
// paths must come from Module._nodeModulePaths(dirname(filename)).
const expectedPaths = Module._nodeModulePaths(path.dirname(target));

// Pre-load state: loaded=false, no filename.
if (m.loaded !== false) throw new Error("pre-load 'loaded' should be false, got " + m.loaded);

m.load(target);

if (m.loaded !== true) throw new Error("post-load 'loaded' should be true");
if (m.filename !== target) throw new Error("filename mismatch: " + m.filename);
if (JSON.stringify(m.paths) !== JSON.stringify(expectedPaths)) {
throw new Error("paths mismatch: " + JSON.stringify(m.paths) + " vs " + JSON.stringify(expectedPaths));
}
if (m.exports !== 'ok') throw new Error("exports mismatch: " + m.exports);
console.log("ok");
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "driver.js"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stdout.trim()).toBe("ok");
expect(exitCode).toBe(0);
});

// Retry guard: a thrown extension handler must NOT leave the module
// permanently marked `loaded`, otherwise the next `.load(...)` call on
// the same instance would hit the "Module already loaded" assert and
// make failure recovery impossible.
test.concurrent("failed load() clears loaded so the instance can be retried (#29253)", async () => {
using dir = tempDir("issue-29253-retry", {
"broken.js": `throw new Error("boom");`,
"good.js": `module.exports = 'good-exports';`,
"driver.js": `
const Module = require("node:module");
const path = require("node:path");

const broken = path.resolve(__dirname, "broken.js");
const good = path.resolve(__dirname, "good.js");
const m = new Module(broken, module);

let threw = false;
try {
m.load(broken);
} catch (e) {
threw = true;
if (!String(e).includes("boom")) throw new Error("unexpected error: " + e);
}
if (!threw) throw new Error("expected load() to throw");
if (m.loaded) throw new Error("loaded should be false after a failed load()");

// Now reuse the instance with a good file — must not hit the
// "Module already loaded" guard.
m.load(good);
if (m.exports !== 'good-exports') throw new Error("retry exports mismatch: " + m.exports);
console.log("ok");
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "driver.js"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stdout.trim()).toBe("ok");
expect(exitCode).toBe(0);
});

// Compound-extension dispatch: `Module._extensions['.test.js']` must win
// over `Module._extensions['.js']` when `.load()` is called on a file
// ending in `.test.js`. `path.extname` alone would return `.js` and
// silently bypass the compound handler.
test.concurrent("load() picks the longest registered extension handler (#29253)", async () => {
using dir = tempDir("issue-29253-ext", {
"foo.test.js": `module.exports = 'raw-source-never-loaded';`,
"driver.js": `
const Module = require("node:module");
const path = require("node:path");

const target = path.resolve(__dirname, "foo.test.js");
Module._extensions['.test.js'] = function (module, filename) {
module.exports = { hookedBy: '.test.js', filename };
};

try {
const m = new Module(target, module);
m.load(target);
if (m.exports.hookedBy !== '.test.js') {
throw new Error("handler not used; exports=" + JSON.stringify(m.exports));
}
} finally {
delete Module._extensions['.test.js'];
}
console.log("ok");
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "driver.js"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stdout.trim()).toBe("ok");
expect(exitCode).toBe(0);
});
Loading