Add Module.prototype.load for new Module() instances #29256
+377
−1
Claude / Claude Code Review
completed
Apr 13, 2026 in 22m 9s
Code review found 1 important issue
Found 5 candidates, confirmed 4. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/js/builtins/CommonJS.ts:418-441 |
module.loaded stuck at true after a failed load(), blocking retry |
| 🟡 Nit | src/js/builtins/CommonJS.ts:420-422 |
modulePrototypeLoad throws plain Error instead of AssertionError for already-loaded module |
| 🟡 Nit | src/js/builtins/CommonJS.ts:435-438 |
modulePrototypeLoad uses path.extname instead of findLongestRegisteredExtension |
Annotations
Check failure on line 441 in src/js/builtins/CommonJS.ts
claude / Claude Code Review
module.loaded stuck at true after a failed load(), blocking retry
After a failed `new Module(id).load(id)` call, `module.loaded` returns `true` even though execution never completed, permanently blocking any retry with "Module already loaded". In Node.js, `this.loaded` is only set after the extension handler returns successfully, so the same module object can be retried on failure.
Check warning on line 422 in src/js/builtins/CommonJS.ts
claude / Claude Code Review
modulePrototypeLoad throws plain Error instead of AssertionError for already-loaded module
In modulePrototypeLoad, the already-loaded guard throws new Error('Module already loaded') instead of an AssertionError as Node.js does. Code checking instanceof AssertionError or e.code === 'ERR_ASSERTION' on this error will behave differently between Node.js and Bun. This is a narrow compatibility gap: calling .load() on an already-loaded module is itself a programming error and no real-world code inspects the error type here.
Check warning on line 438 in src/js/builtins/CommonJS.ts
claude / Claude Code Review
modulePrototypeLoad uses path.extname instead of findLongestRegisteredExtension
In `modulePrototypeLoad` (CommonJS.ts:436), the extension lookup uses `path.extname(filename)` which only returns the last extension component — `.js` for `foo.test.js` — so a user-registered `Module._extensions['.test.js']` handler is never consulted. Node.js and Bun's own native require path both use `findLongestRegisteredExtension` which tries progressively shorter suffixes (e.g. `.test.js` before `.js`); the new TypeScript implementation breaks that contract.
Loading