Repository navigation
fix(ext/node): run load hook chain on every require() of a builtin - #34223
Conversation
…ough
In `executeEsmLoadHookChain`, calling `nextLoad()` from a user hook used
to return `{ source: null }` once the chain reached its end. Node's
contract is that `nextLoad` returns the actual default-loaded source so
a hook can transform it. Now read the file from disk for `file://` URLs
(and return the builtin marker for `node:*`), matching the CJS path.
Fixes module-hooks compat tests `test-module-hooks-load-esm` and
`test-module-hooks-load-esm-mock`.
…oid native-module fallback for hook-redirected builtins
Two module hook fixes:
1. createRequire(url) used to drop a URL's query/hash because the
resolve hook chain rebuilt `context.parentURL` from the Module's
filename via `pathToFileURL`. Preserve the original URL on the
Module under a private symbol and prefer it when populating
`parentURL`.
2. When a resolve hook redirects `require('zlib')` to a file path,
`Module._load` would still fall back to `loadNativeModule(filename,
request)` and return the native builtin via the original request
name. Skip that fallback when the filename came from a hook
(tracked in `cjsHookResolvedFilenames`).
Enables module-hooks compat tests for create-require-with-url,
resolve-builtin-on-disk-require, resolve-load-builtin-override-both,
load-esm, load-esm-mock, and load-url-change-import.
In compile mode (and other cases where the file isn't present on the
real filesystem) op_require_read_file throws "path not found". Catch
that and return { source: null } so the Rust default loader (which can
read from the embedded eszip) handles it.
Module._load checked the prefixed builtin cache before invoking the load
hook chain, so a registerHooks() load hook only fired on the first
require() of a builtin (e.g. require('zlib')). A subsequent
require('node:zlib') would return the cached module without running the
hook. Node calls the load hook on every require() of a builtin.
Move the load hook chain so it runs before the cache lookup, then fall
through to the existing cache hit / native module path. Enables the
test-module-hooks-load-builtin-override-{commonjs,json,module} compat
tests.
lunadogbot
left a comment
lunadogbot
left a comment
There was a problem hiding this comment.
LGTM. The refactor is straightforward: hook chain now runs before the node:-prefixed cache check, so a registerHooks() load hook fires on every require() of a builtin (matching Node). The override / cache-hit / native-module branches below are functionally unchanged — just reordered.
Three builtin-override tests are un-skipped, which is good coverage for the format-override path. (Reviewed only the new content vs. #34219, which I already approved.)
One minor thought left inline about an implicit behavioral consequence worth flagging in the comment.
| // Run load hooks for builtins if registered. Node invokes the load hook | ||
| // chain on every require() of a builtin, so do this before the cache | ||
| // lookup; otherwise the second require would short-circuit and the hook | ||
| // wouldn't fire. |
There was a problem hiding this comment.
Worth expanding this comment with one more sentence on the implication: on a cache hit (line 1425 below), the hook still runs but its return value is fully discarded — only side effects (logging, counters, throws) survive. A user hook that returns a different source/format on the second call will see that return value silently ignored, since the cached module wins. That matches Node's "first invocation wins for caching" behavior, but it's not obvious from the current comment, which reads as if the hook still drives the load each time.
Related: a hook that throws on the Nth call will now propagate the error instead of being short-circuited by the cache (as it was previously). Also Node-aligned, but a real behavior change worth being explicit about.
…tin-override # Conflicts: # tests/node_compat/config.jsonc
…enoland#34223) `Module._load` was checking the prefixed builtin cache before invoking the load hook chain, so a `registerHooks()` load hook only fired on the first `require()` of a given builtin. A subsequent `require('node:zlib')` (after a prior `require('zlib')`) returned the cached module and never re-entered the hook, even though Node invokes the hook on every `require()` of a builtin. Moves the load hook chain so it runs before the cache lookup. The hook result is then handled by the existing override / cache-hit / native-module path, so cached modules are still reused and the override path is unchanged otherwise. Enables the previously-failing module-hooks compat tests `test-module-hooks-load-builtin-override-commonjs`, `-json`, and `-module` in `tests/node_compat/config.jsonc`.
Stacked on #34219.
Module._loadwas checking the prefixed builtin cache before invoking the load hook chain, so aregisterHooks()load hook only fired on the firstrequire()of a given builtin. A subsequentrequire('node:zlib')(after a priorrequire('zlib')) returned the cached module and never re-entered the hook, even though Node invokes the hook on everyrequire()of a builtin.Moves the load hook chain so it runs before the cache lookup. The hook result is then handled by the existing override / cache-hit / native-module path, so cached modules are still reused and the override path is unchanged otherwise.
Enables the previously-failing module-hooks compat tests
test-module-hooks-load-builtin-override-commonjs,-json, and-moduleintests/node_compat/config.jsonc.