Repository navigation
fix(ext/node): module hook fixes for ESM nextLoad, createRequire URL, and builtin redirects - #34219
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.
lunadogbot
left a comment
lunadogbot
left a comment
There was a problem hiding this comment.
LGTM — three surgical fixes, each backed by an upstream test getting un-skipped. Left a few non-blocking nits inline.
One thought beyond the inline notes: kSourceURL is only set on the top-level Module from createRequireFromPath, so transitively-require()d children won't carry the original URL forward. That matches Node's intent (the URL identifies the parent of the require), but worth confirming test-module-hooks-create-require-with-url.mjs doesn't probe a deeper hop.
| const source = op_require_read_file(url.fileURLToPath(loadUrl)); | ||
| return { | ||
| source, | ||
| format: currentContext?.format ?? undefined, |
There was a problem hiding this comment.
Nit: ?? undefined is redundant here — currentContext?.format already evaluates to undefined when currentContext is null/undefined or .format is missing. Could just be format: currentContext?.format,.
| } catch { | ||
| // File isn't available synchronously (e.g. embedded in a compiled | ||
| // binary's eszip); fall through to Rust default loading. | ||
| return { source: null, shortCircuit: true }; |
There was a problem hiding this comment.
The bare catch swallows any failure, not just the eszip case the comment describes — e.g. EACCES, transient IO errors, or the file being deleted between resolve and load would all silently defer to Rust loading instead of surfacing at the hook. Probably fine in practice (Rust will produce a clearer error anyway), but consider widening the comment so a future reader doesn't assume eszip is the only intended path.
| return mod.exports; | ||
| // A resolve hook that redirected `require('zlib')` to a file path must | ||
| // not silently fall back to the native module via the original request. | ||
| if (!SetPrototypeHas(cjsHookResolvedFilenames, filename)) { |
There was a problem hiding this comment.
Two small things here:
-
maybeEmitNativeModuleDeprecation(filename)on line 1484 still fires before the gate. It's a no-op whenfilenameis a hook-resolved path (the lookup returnsundefined), so harmless — but semantically it might read more clearly inside the same!SetPrototypeHas(...)branch. -
The cleanup of
cjsHookResolvedFilenameshappens inModule.prototype._compile(line 2034). Ifmodule.load(filename)throws before reaching_compile, the set entry leaks and any future load of the same filename via a non-hook path would also skip native fallback. The existingthrewcleanup block (around line 1513) already deletesModule._cache[filename]— consider addingSetPrototypeDelete(cjsHookResolvedFilenames, filename)there for symmetry. Edge case, won't block.
|
I'll address these in a follow up in #34223 |
… and builtin redirects (denoland#34219) A trio of fixes to `node:module` registerHooks() compat behavior, motivated by failing tests in the bundled `test-module-hooks` suite. In the ESM load hook chain, `nextLoad()` used to return `{ source: null }` when the chain fell through, breaking user hooks that wrap `nextLoad` to transform real source. The terminal step now reads the file from disk for `file://` URLs (and returns the builtin marker for `node:*`), matching the contract followed by the CJS path. `createRequire(url)` dropped a URL's query/hash because the resolve hook chain rebuilt `context.parentURL` from the Module's filename via `pathToFileURL`. The original URL is now stashed on the Module under a private symbol and preferred when populating `parentURL`. When a resolve hook redirected `require('zlib')` to a file path, `Module._load` would still hit `loadNativeModule(filename, request)` and return the native builtin via the original request name. The fallback is now skipped when the filename came from a hook (tracked in `cjsHookResolvedFilenames`). Six previously-failing module-hooks compat tests are enabled in `tests/node_compat/config.jsonc` as a result.
2 participants
A trio of fixes to
node:moduleregisterHooks() compat behavior, motivatedby failing tests in the bundled
test-module-hookssuite.In the ESM load hook chain,
nextLoad()used to return{ source: null }when the chain fell through, breaking user hooks that wrap
nextLoadtotransform real source. The terminal step now reads the file from disk for
file://URLs (and returns the builtin marker fornode:*), matching thecontract followed by the CJS path.
createRequire(url)dropped a URL's query/hash because the resolve hookchain rebuilt
context.parentURLfrom the Module's filename viapathToFileURL. The original URL is now stashed on the Module under aprivate symbol and preferred when populating
parentURL.When a resolve hook redirected
require('zlib')to a file path,Module._loadwould still hitloadNativeModule(filename, request)andreturn the native builtin via the original request name. The fallback is
now skipped when the filename came from a hook (tracked in
cjsHookResolvedFilenames).Six previously-failing module-hooks compat tests are enabled in
tests/node_compat/config.jsoncas a result.