-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Add Module.prototype.load for new Module() instances #29256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
robobun
wants to merge
15
commits into
main
Choose a base branch
from
farm/fe239255/fix-module-prototype-load
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+377
−1
Open
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
896c40a
add Module.prototype.load for new Module() instances
robobun ff60c09
[autofix.ci] apply automated fixes
autofix-ci[bot] 1bf6b8b
scope load() fix, drop risky prototype unification
robobun d4d1c67
[autofix.ci] apply automated fixes
autofix-ci[bot] 217ed5d
test: assert stdout before exitCode for clearer diagnostics
robobun f5c713d
test: use test.concurrent + drop brittle stderr checks
robobun f344c1f
test: strengthen m.paths check against Module._nodeModulePaths
robobun a99e8eb
restore modulePrototypeLoad and load registration
robobun fc64d31
fix __dirname + expose load on Module.prototype
robobun 9d939d5
skip leading-dot in extension lookup + refresh stale test comment
robobun 90334a8
test: pin Module.prototype.load registration on the disposable prototype
robobun 9a99fd3
set Module.prototype.load.name to 'load'
robobun ee3ae67
test: exercise this.path reset with mismatched ctor id vs load path
robobun 4c089e6
test: guard the Module.prototype.load.name override
robobun 4d3379d
test: raise timeout to 30s on subprocess tests
robobun File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,263 @@ | ||
| // 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 shared by instances created via `new Module(...)` | ||
| // and unifies `require("module").prototype` with that same | ||
| // prototype, so patching one is reflected in the other (Node | ||
| // semantics). | ||
| 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"); | ||
claude[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }); | ||
robobun marked this conversation as resolved.
Show resolved
Hide resolved
claude[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| test("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); | ||
|
|
||
robobun marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // 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]); | ||
|
|
||
| expect(stderr).not.toContain("TypeError"); | ||
| expect(stderr).not.toContain("Error"); | ||
| expect(exitCode).toBe(0); | ||
|
|
||
| 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); | ||
| }); | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| test("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(stderr).not.toContain("TypeError"); | ||
| expect(stderr).not.toContain("ReferenceError"); | ||
| expect(exitCode).toBe(0); | ||
| expect(stdout.trim()).toBe("number"); | ||
| }); | ||
|
|
||
| test("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); | ||
|
|
||
| // 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 (!Array.isArray(m.paths)) throw new Error("paths should be an array"); | ||
| 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(stderr).not.toContain("TypeError"); | ||
| expect(stderr).not.toContain("Error:"); | ||
| expect(exitCode).toBe(0); | ||
| expect(stdout.trim()).toBe("ok"); | ||
| }); | ||
|
|
||
| // 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("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(stderr).not.toContain("Module already loaded"); | ||
| expect(exitCode).toBe(0); | ||
| expect(stdout.trim()).toBe("ok"); | ||
| }); | ||
|
|
||
| // 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("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(stderr).not.toContain("TypeError"); | ||
| expect(stderr).not.toContain("Error:"); | ||
| expect(exitCode).toBe(0); | ||
| expect(stdout.trim()).toBe("ok"); | ||
| }); | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.