Skip to content

node:module: select the Module._extensions handler of a require() with one rule, and pass it the file name - #47527

Open
robobun wants to merge 4 commits into
robobun/073c904f/js-handler-loader-by-namefrom
robobun/68977c8c/require-extensions-query
Open

node:module: select the Module._extensions handler of a require() with one rule, and pass it the file name#47527
robobun wants to merge 4 commits into
robobun/073c904f/js-handler-loader-by-namefrom
robobun/68977c8c/require-extensions-query

Conversation

@robobun

@robobun robobun commented Oct 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Stacked on #47216.

Problem

  • In a compiled executable and in bun repl, a Module._extensions handler on a made-up extension (.hbs) does not run. A handler on a built-in extension makes it run.
  • With a ?query it is the opposite. When it runs, fs.readFileSync(filename) throws ENOENT.
  • transpile_file (src/runtime/jsc_hooks.rs) reads the handler table in two places, chosen by the counter has_mutated_built_in_extensions.

Fix

  • registered_extension_for_require (src/jsc/NodeModuleModule.rs) is the one selection, from the path of the key. A loader from the importer (?raw, type) or from the configuration wins.
  • The handler gets the file name, and module.filename keeps the query. The built-in handler loads the key when a wrapper hands the name back.
  • Verified: test/js/node/module/require-extensions.test.ts (25 of 54 fail on the base), test/aot/strict-flag-refuses-as-without-aot.test.ts.
  • Self-reviewed: 37 concerns raised, 33 addressed. Notes name the rest.

Background

  • Module._extensions maps an extension to a handler (module, filename). Bun keeps the stored functions in a table.
  • A ?query is part of a module key. process.dlopen already gets the path.
  • Considered: hand the handler the key. Then handlebars throws ENOENT, and tsx.require() takes 1,793 ms, not 32 ms.

Downsides

  • filename is not module.filename for a key with a query, and .js hooks now run for it. tsx with tsx/cjs and a scope: 104 ms, not 32 ms.
  • A file under a directory what? no longer reaches a .js read hook. Main and Node load it.
  • With only made-up extensions registered, each other require() pays +324 of 67,384 instructions (implement require.extensions #18478's fast path).
Notes

Decisions for a maintainer. No maintainer has ruled on these rows. Each is pinned by a test, so a different ruling changes one expectation.

Row main Node v26.3.0 this PR
.foo handler, require("./x.foo?v=1"): the filename argument x.foo?v=1, equal to module.filename, while no built-in extension has a handler. No call otherwise no such key. Equal to module.filename on both of its routes x.foo. module.filename and the require.cache key are x.foo?v=1
__filename in a CommonJS module with a query that a hook compiles with module._compile(code, filename) the key (the hook is skipped) the argument of _compile the file name. delete require.cache[__filename] does not remove the module
.js hook that reads the file, path <dir>/what?/x.js loads loads ENOENT reading "<dir>/what". #41929 pins the load
.foo handler and --loader .foo:text or bunfig [loader] text while no built-in extension has a handler, the handler otherwise no equivalent text in every state
require("./p.js", { type: "js" }) under a .js hook the hook runs the hook runs (Node has no such option) the hook is skipped
closed bun build --compile --aot executable, .foo handler, file on disk Cannot load, or the handler once a built-in extension has one no equivalent the handler runs. module._compile in it is code from a string

The stack. #47216 makes the built-in .js handler pick the loader from the file name. Without it this change turns a load into the error of #47175 for a .js handler that is assigned or passed through under a TypeScript key with two dots, and for a ?query on a TypeScript or JSX file under a .js hook. On main those requires never reach the handler. This PR deletes the counter bookkeeping that #47216 extends (shadows_default_loader), and makes NodeModuleModule__builtinHandlerLoader read the extension from the path of the key.

The report. Two files in one directory, plain bun run.

// x.foo
module.exports = "plain x.foo";
// main.cjs
const Module = require("node:module");
const original = Module._extensions[".js"];
Module._extensions[".js"] = (m, f) => original(m, f);
Module._extensions[".foo"] = (m, f) => { m.exports = "the .foo handler got " + require("node:path").basename(f); };
console.log("with query:", require("./x.foo?query"));
console.log("without query:", require("./x.foo"));

Main prints with query: plain x.foo. Without the two .js lines it prints with query: the .foo handler got x.foo?query. This branch prints with query: the .foo handler got x.foo in both cases.

What the two reads did. The counter came with #18478 (relanded as #18686) as a cost tier: "mutating a built in extension will fire this slow path in more situations". The first read ran when a key of the default loader table (.js, .css, .json, 23 keys) had a handler. It ran before the loader was known, so a match also won over ?raw and a type attribute. The second read ran only when the first did not, the path had no loader, and has_loaded was set. has_loaded is set when the source of the entry point is produced (#33371). A compiled executable and the REPL do not set it. The counter stayed up when the original handler was assigned back. Nobody chose the operand of each read, their order against ?raw and type, or the has_loaded gate.

The first read took the extension from the text of the query. On main, with a .js handler, require("./p?x.js") (a file p) and require("./y.bar?v=1.js") call the .js handler, and require("./p.js?q") does not. The payload of a data: URL selected a handler in both reads.

Which line is the fix. The selection in transpile_file hands the module key to registered_extension_for_require, which cuts the query (path_of_module_key), where main handed it to the matcher as it was. The gate reads the table length, not the counter, and LoaderResult::importer_chose_loader, which get_loader_and_virtual_source sets where it applies ?raw and type. The rest is the deletion of the second read and of the counter.

The file name that a handler gets. registered_extension_for_require returns the file name with the entry: the key cut by the one Rust rule. transpile_file hands it to C++ in source_url when it is not the key. C++ has no cut of its own. Node calls a handler with module.filename, a path (loader.js#L1596). internalRequire (src/js/builtins/CommonJS.ts) already hands process.dlopen the path of a .node id (#33148).

A wrapper hands that file name back to the built-in handler. While require() calls a handler, evaluateCommonJSCustomExtension records the key and the file name of the call (currentExtensionHandlerCall, per thread, on the stack). builtinLoader loads the key when it is given that file name. It does not read module.filename, which script can assign. Without this step, three require() calls of one ES module (?v=1, ?v=2, no query) under a pass-through .js wrapper return one instance, and import.meta.url loses the query.

Behaviour changes. The tests in describe("the handler that a require() selects") pin each of them, except where a line says otherwise.

  • A handler on a made-up extension runs for a file on disk in a bun build --compile executable and in bun repl. The path is one that the build does not resolve. A literal require("./t.hbs") is answered at build time, on main and here. With --disallow-code-generation-from-strings=strict the handler runs too, and module._compile of text from the disk is still refused. docs/runtime/code-generation-from-strings.mdx gets that row.
  • The same in a closed --aot executable, where main refuses the load with Cannot load ... it isn't in this executable. A handler that calls module._compile gets EvalError unless the build saw an evaluator. The AOT rule holds: the route "own extension handler compiles" of code-the-compiler-never-saw-cases.ts passes before and after. strict-flag-refuses-as-without-aot.test.ts pins the two roads.
  • A handler runs for a key with a ?query, also on a built-in key. It gets the file name. Such a key gets what the key without the query gets: a .js hook that compiles the text throws SyntaxError for an ES module or a TypeScript file (node:module: make module._compile() accept ES module text and a hashbang line #47519 changes that), a tsconfig.json with a comment throws under a .json wrapper, and module.loaded stays false when a handler answers by itself.
  • A handler that hands the built-in handler another file's name gets that file's one ES module for every query of its key. On main a hook on a made-up key that carried the query into the other name got one module for each query.
  • require(id, { type }) gets the attribute's loader also while a built-in key has a handler. A type that names no loader is not a choice.
  • A loader that --loader or bunfig [loader] names for an extension with no default loader wins over a handler in every state. With bunfig ".hbs" = "text" and handlebars loaded, require("./t.hbs") stays the text. For an extension with a default loader the handler wins, as on main: --loader .js:jsx does not switch off a .js hook.
  • A handler on .sh, .base64, .file or a --loader extension runs in every state. So does a handler under a key with two dots (.lit.md, the shape of coffeescript's .coffee.md), which node:module: let the .js extension handler pick the loader from the file name #47216 reaches through the counter.
  • The query text and the payload of a data: URL never select a handler. The query starts at the first ? of the key.
  • A file that a plain --compile executable holds is not handed to a handler, with a query or without one. That is main's result for both forms.
  • Under bun test --isolate a handler that an earlier test file left is still in the table. It now also answers keys with a query. A file that awaits an async export of ./a.js?v=2 after another file left a .js wrapper times out here and passes on main. The same file without the query times out on main too. bun test --isolate: reset Module._extensions per file and ask it before the source cache answers a require() #47477 resets the table for each file. Not covered by a test here.
  • A file that a handler reads by itself is not watched by --watch or --hot, on main and here. Bun watches a file when its own transpiler opens it. That now holds for the keys that newly select a handler. node --watch restarts for such a file. Watch the file that a runtime plugin's onLoad serves, before the callback runs #47566 adds a watch by path for the file of a runtime plugin's onLoad. The same call before the handler runs, with the file name that this PR hands over, closes it for handlers. That change needs both PRs, so it is not in this one. Not covered by a test.

Node. Node selects in one place, findLongestRegisteredExtension in Module.prototype.load, from the file name. For every input that Node defines (16 rows in 4 states of the other handlers, one fresh process each), the cells equal to Node v26.3.0 are 56 of 64 on main, 61 on the base and 64 here. require-extensions.test.ts runs 7 processes of such requests under node too, where node is installed.

A name with a ? reaches Node's selection in two ways. When a Module._resolveFilename override returns x.foo?ns=1, Node walks the whole name, finds no key and calls the .js handler with x.foo?ns=1. When a registerHooks resolve hook returns a URL with a query, Node drops the query (loader.js#L1171): the .foo handler gets x.foo, and module.filename and the cache key have no query. Bun selects from the path and hands the handler the path, as the second way does. module.filename keeps the key, which is Bun's. No Node route has both.

Node hands a file with no registered extension to its .js handler. Bun does not, as on main: under a wrapped .js, Node calls the wrapper for ./p, ./y.bar and ./x.cjs, and Bun calls it for none of them. ts-node and tsx use that step on Node. #46828 adds Module.prototype.load with Node's rule.

Real hooks and a ?query. Release builds of main, the base, the first form of this PR (the handler gets the key) and this PR. The hook on a built-in extension now also runs for a key with a query. That is the cause of every row where this PR differs from main.

require() under main base handler gets the key this PR Node v26.3.0
pirates hook on .js, ./p.js?v=2 hook skipped hook skipped hook declines hook transforms the file no such key
pirates hook on .js, a module that does delete require.cache[__filename], required twice as ./self.js?v=1 runs twice runs twice runs twice runs once no such key
handlebars, ./t.hbs?v=2 ENOENT ENOENT ENOENT template no such key
handlebars and bunfig ".hbs" = "text", ./t.hbs text text template function text no equivalent
coffeescript, ./plain.coffee?v=2 ENOENT evaluated as JavaScript ENOENT compiled no such key
ignore-styles, ./logo.svg?url 2 errors building 2 errors building {} {} no such key
.js hook that reads the file and calls module._compile, ./q.js?v=2 (CommonJS) hook skipped, loads hook skipped, loads ENOENT hook loads the file no such key
the same hook, ./e.js?v=1 (an ES module) or ./t.ts?v=1 hook skipped, loads hook skipped, loads ENOENT SyntaxError, as without the query on every build without the query: loads the ES module, SyntaxError for TypeScript
tsx.require(), 300 modules, median of 5 31 ms 34 ms 1,793 ms 32 ms 1,660 ms
the same with require("tsx/cjs") also registered, warm tsx cache 32 ms 32 ms 1,486 ms 104 ms 1,698 ms
tsx, three register({ namespace }) scopes, one entry [1,2,3] [1,2,3] [1,1,1] [1,2,3] [1,2,3]
the same with require("tsx/cjs") also registered [1,2,3] [1,2,3] [1,1,1] [1,1,1] [1,2,3]

tsx appends ?namespace= in a Module._resolveFilename override. Its handler reads the namespace from filename and hands every other file to the handler that it replaced. Here the handler of a scope finds no namespace and declines. The handler that it replaced is Bun's when only scopes are registered. It is tsx's global handler when tsx/cjs is registered too, and that handler transforms the file. With an empty tsx cache the first run of the 104 ms row takes 1 to 2 s. The test "a key that Module._resolveFilename gives a query" has both states.

Probes. One fresh process for each cell, release builds of the base and of this branch.

  • 18 rows in 5 states of the other handlers (none, an unrelated .css handler, a .js wrapper, a .js wrapper that was restored, .js assigned to itself). Rows whose result differs between "none" and "an unrelated .css handler": 6 of 18 on main, 5 of 18 on the base, 0 of 18 here. 31 of 90 cells change against the base: x.foo?query and ./x?q (10), x.foo with type: "text" (4), p.js with type: "text" (3), p.js?q under the wrapper (1), three query texts that end in .js (3), a data: URL (5), a .sh handler alone (1), a .foo handler with --loader .foo:text while a built-in key has a handler (4).
  • 27 cells of entries that hand the file to the built-in .js handler: 9 keys (.spec.js, .min.js, .es.jsx, .spec.ts, .d.ts, .spec.tsx, .sh, .base64, .file) times 3 shapes (assigned, passed through, pirates). 9 cells change, the three loader names: main and the base give the path of the file, this branch and Node v26.3.0 evaluate it. Cells equal to Node go from 13 to 22. No cell leaves Node's result.

Numbers. Release builds (lto, baseline) of the base dbff1d5 and of this branch, same build directory. Instruction counts are gdb single-step counts of one call with the JIT off. A row for one function counts the instructions of that function alone. Whole-call counts include the dynamic linker, whose share moves with the layout of the binary. valgrind, perf, strace and bloaty are not in the build container.

base this PR
gate with no handler, per require() that loads a file 5 instructions the same 5, the operand is the table length
gate per import 2 2
transpile_file and get_loader_and_virtual_source alone, no handler: import / require() 425 / 492 427 / 492
Bun__transpileFile, no handler: require() of a .js file / of a .bar file 67,292 / 71,952 67,295 / 71,946
Bun__transpileFile, no handler: import 14,360 14,449 (+2 in Bun, +87 in the dynamic linker)
Bun__transpileFile, require() of a .js file, only .hbs and .handlebars registered 67,384 67,708 (+324, one hash probe)
Bun__transpileFile, require() of a .json file under a pass-through .js wrapper 16,714 16,762
Bun__transpileFile, the call that returns the handler, .js wrapper / .hbs handler 1,700 / 2,087 1,956 / 1,874
whole require() that a handler on a made-up key answers: no query / with a query 23,628 / 23,698 23,431 / 23,677, and one allocation for the file name with a query
builtinLoader and NodeModuleModule__builtinHandlerLoader alone, called by a .js wrapper 166 and 41 184 and 66
mimalloc calls and syscalls in Bun__transpileFile, 6 fixtures 310/5, 311/5, 298/5, 201/5, 0/0, 0/0 the same
.text 108,647,689 108,648,713 (+1,024 bytes)
size_of::<VirtualMachine>() 32,256 32,256

An earlier form placed the selection before get_loader_and_virtual_source, so that a handler hit skips it. It added 13 instructions to every import. A check of the file name against the keys before the hash walk saved about 150 instructions per miss with two keys. Its cost grows with the number of keys, and it skipped a handler for a key with a trailing separator. Neither is in the PR.

Tests. describe("the handler that a require() selects") in require-extensions.test.ts:

  • 28 requests in one fixture, run as a fresh process in 8 states: .foo alone, a wrapped .js, a restored .js, .css, handlers on 5 more keys, and three --loader states. The requests have keys with two ?, a separator in the query, and names that are not ASCII.
  • 7 processes of requests that mean the same to Node.js, compared with node where it is installed: a handler alone on .sh, .base64 and .file, the .js handler assigned and called under one of them, keys with two dots, names that begin with a dot.
  • A handler that reads the file itself, on .foo alone and on .foo and .js, with an ES module under the second.
  • An ES module and a CommonJS module under four keys each, a module that removes itself from the cache, and a TypeScript stack frame, with no wrapper, a wrapper, and a wrapper that replaces module._compile.
  • A handler that names another file, a key that Module._resolveFilename gives a query (the shape of tsx, two states), bun repl, and a bun build --compile executable with a file on disk and an embedded file, run plain and with =strict.

Removing one clause at a time fails tests: the data: clause (4), the query cut (21), the importer gate (7), "any type is a choice" (7), "a type that names no loader hides ?raw" (6), the arm for a built-in handler under another key (3), the query cut in the built-in handler (4), the file name for the handler (16), the configured loader (2), "only an extension with no default loader" (35), the embedded file (1), the record in the built-in handler (2), the name test of the record (1).

Suites run on the debug (ASAN) build of this branch: require-extensions.test.ts (54 pass), strict-flag-refuses-as-without-aot.test.ts (15 pass), require-extensions-override.test.ts, 22929-module-extensions-asi.test.ts, node-module-module.test.js, module-resolve-filename-paths.test.js, module-sourcemap.test.js, import-query.test.ts, import-meta.test.js, plugins.test.ts, run-cjs.test.ts, transpiler-cache.test.ts, module-graph.test.ts, test/cli/test/isolation.test.ts, isolation-one-module-fetched-twice.test.ts, test/js/bun/repl/repl.test.ts, and the 48 test-require-* and test-module-* files of test/js/node/test/parallel. On the release builds of the base and of this branch: module-graph-isolation.test.ts (413 pass on both), disallow-code-generation-from-strings.test.ts (34 pass on both) and the 5 routes about extensions of test/aot/parts.test.ts (pass on both). cargo clippy passes for bun_jsc and bun_runtime. cargo check passes for x86_64-pc-windows-msvc and aarch64-apple-darwin.

Other open PRs.

Not changed.

Self-review. The second review ran on the second commit. It kept the selection and asked for: one rule for the path of a key (done: Rust hands the file name to C++), a key for the built-in handler that script cannot write (done: the record of the call), the configured loader (done), an embedded file (done), rows that run under Node, rows for two ?, a separator, names that are not ASCII, a hook that names another file, __filename, the shape of tsx, the closed --aot executable, and a body that states each row above. Not done: a ruling by a maintainer on the table at the top, a tracking issue for the rows that are not changed, the --isolate case (#47477 has the reset), and cases in the three-ways harness of test/aot (main replaced that harness in #47129 after this branch forked).

no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/module/require-extensions.test.ts, test/aot/strict-flag-refuses-as-without-aot.test.ts

@robobun
node:module: select the Module._extensions handler of a require() wit…
0424ed5 
…h one rule

transpile_file read the table of Module._extensions handlers in two places.
A counter of built-in extensions that have a handler chose one. The first
read matched the module key with its ?query. The second read ran only after
the entry point was loaded, so a compiled executable and the REPL never
reached it.

registered_extension_for_require is now the one selection. It matches the
path of the key and reads no flag. A ?raw query or a type attribute that
names a loader wins over a handler. The second read and the counter are
gone. The built-in handlers read the extension from the path of the key.
@robobun

robobun commented Oct 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author
Updated 4:18 PM PT - Oct 31st, 2026

❌ @robobun, your commit 0435891 has 61 failures in Build #136201 (All Failures):

🧪   To try this PR locally:

bunx bun-pr 47527

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

bun-47527 --bun

@robobun

robobun commented Oct 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Status of #47527: open, stacked on #47216. It needs a decision by a maintainer on the six rows at the top of the notes in the PR body.

Reproduction on main (f7607a3), plain bun run, two files in one directory:

// x.foo
module.exports = "plain x.foo";
// main.cjs
const Module = require("node:module");
const original = Module._extensions[".js"];
Module._extensions[".js"] = (m, f) => original(m, f);
Module._extensions[".foo"] = (m, f) => { m.exports = "the .foo handler got " + require("node:path").basename(f); };
console.log("with query:", require("./x.foo?query"));

Main prints with query: plain x.foo. This branch prints with query: the .foo handler got x.foo.

Without the ?query and without the .js lines, main skips the same .foo handler for a file on disk in bun repl -e and in a bun build --compile executable. test/js/node/module/require-extensions.test.ts covers both.

CI of 0435891 (build 136201, finished):

  • test-bun passes on Debian, Ubuntu and Alpine (x64 and aarch64), Debian ASAN, Windows (x64 and aarch64) and macOS aarch64. test-aot and test-hardened pass on every lane.
  • One macOS x64 test-bun shard fails on test/cli/check/check.test.ts: in "finishes within 10 s on conditions nested deeper than the stack allows" the bun check child reached its 10 s limit on each of 4 attempts. This diff does not touch bun check, and the test passed on that lane for the first commit (build 136007).
  • binary-size fails for the Android and FreeBSD targets. The canary of main is 9 MB smaller there than the base of this stack, which is behind main. This diff adds 1,024 bytes of .text on linux-x64.
  • The test-aot-drawn lanes are red. They are a sample that holds up nothing (soft_fail in .buildkite/ci.ts), and no test of this PR fails in them. The same lanes are red on node:module: let the .js extension handler pick the loader from the file name #47216 (build 134640).
  • cargo clippy, Format, Lint JavaScript, Source lints and comment-cop pass.

claude[bot]
claude Bot reviewed Oct 31, 2026
View reviewed changes

@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.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Comment thread src/runtime/jsc_hooks.rs Resolved
Comment thread src/runtime/jsc_hooks.rs Resolved
Comment thread src/jsc/VirtualMachine.rs Resolved
Comment thread src/jsc/NodeModuleModule.rs Resolved
@robobun
node:module: hand a Module._extensions handler the file name of a key…
a18c042 
… with a query

A handler that a require() selects for a key with a ?query got the key as
its filename, and a hook that reads the file threw ENOENT. The selection
now returns the file name with the entry, and the handler gets that name.
module.filename and the require.cache key keep the query. __filename of a
module that a handler compiles is the name that the handler gives to
module._compile.

A wrapper hands that file name back to the built-in handler. While
require() calls a handler, builtinLoader loads the key of that call when
it is given the file name of the call, so each query stays its own module.

A loader that --loader or bunfig [loader] names for an extension with no
default loader wins over a handler. A file that a standalone executable
holds is not handed to a handler, with a query or without one.

Fix clippy::or_fun_call in NodeModuleModule__builtinHandlerLoader.
@robobun robobun changed the title node:module: select the Module._extensions handler of a require() with one rule node:module: select the Module._extensions handler of a require() with one rule, and pass it the file name Oct 31, 2026
github-actions[bot]
github-actions Bot reviewed Oct 31, 2026
View reviewed changes
Comment thread src/jsc/NodeModuleModule.rs Resolved
github-actions[bot]
github-actions Bot reviewed Oct 31, 2026
View reviewed changes
Comment thread src/jsc/VirtualMachine.rs Resolved
github-actions[bot]
github-actions Bot reviewed Oct 31, 2026
View reviewed changes
Comment thread src/jsc/bindings/JSCommonJSExtensions.cpp Resolved
github-actions[bot]
github-actions Bot reviewed Oct 31, 2026
View reviewed changes
Comment thread src/jsc/bindings/JSCommonJSExtensions.h Resolved
github-actions[bot]
github-actions Bot reviewed Oct 31, 2026
View reviewed changes
Comment thread src/jsc/bindings/ModuleLoader.cpp Resolved
github-actions[bot]
github-actions Bot reviewed Oct 31, 2026
View reviewed changes
Comment thread src/runtime/jsc_hooks.rs Resolved
@robobun

robobun commented Oct 31, 2026

Copy link
Copy Markdown
Collaborator Author

a18c042 and 21562f4 answer the reviews of 0424ed5 and a18c042.

  • A handler now gets the file name without the ?query, and the built-in handler loads the key when a wrapper hands that name back. A hook that reads the file no longer throws ENOENT.
  • A loader that the configuration names for an extension with no default loader now wins over a handler.
  • cargo clippy failed on or_fun_call at src/jsc/NodeModuleModule.rs:282. That line is fixed.
  • The six comments of more than one line in src/ are one line each now.
  • The findings about Node's .js step and about the Windows \\?\ prefix have a reply in their threads. They need no code change in this PR.

The notes of the PR body start with a table of six rows that need a decision by a maintainer.

claude[bot]
claude Bot reviewed Oct 31, 2026
View reviewed changes

@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.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline nits, I also checked the new currentExtensionHandlerCall thread-local in src/jsc/bindings/ModuleLoader.cpp — its key/filename references point at locals of fetchCommonJSModuleNonBuiltin that outlive the handler call, and SetForScope nests correctly under a re-entrant require() from inside a handler — and the repurposed ResolvedSource.source_url on the CommonJSCustomExtension tag, whose only consumer is the new read at src/jsc/bindings/ModuleLoader.cpp:930. The second push (handler now receives the query-less file name; a configured non-default loader now wins over a handler) covers the two actionable points from my earlier review.

Extended reasoning...

Reworks how a CommonJS require() selects a Module._extensions handler across Rust (jsc_hooks.rs, NodeModuleModule.rs, VirtualMachine.rs) and the C++ module loader, with a new thread-local handler-call record and a repurposed source_url field; no security-sensitive surface. The hunt was cut off at its finding cap and two nits are posted inline, so this is a ruled-out note rather than an approval.

Comment thread test/js/node/module/require-extensions.test.ts Resolved
Comment thread test/js/node/module/require-extensions.test.ts Resolved
@robobun

robobun commented Oct 31, 2026

Copy link
Copy Markdown
Collaborator Author

0435891 answers the two optional findings of the last review.

  • The compile test now prints whether the embedded file was there, so a missing asset fails the test.
  • The per-test timeout of the compile test stays. The reply in its thread has the timings on the debug build.

claude[bot]
claude Bot reviewed Oct 31, 2026
View reviewed changes

@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 completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

This was referenced Nov 1, 2026
Open
Open
Open

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

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments
@claude claude[bot] claude[bot] left review comments

Assignees

No one assigned

Labels

claude

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@robobun