Skip to content

module: implement synchronous module evaluate hooks #57139

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
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -2256,6 +2256,12 @@ The V8 platform used by this instance of Node.js does not support creating
Workers. This is caused by lack of embedder support for Workers. In particular,
this error will not occur with standard builds of Node.js.

<a id="ERR_MODULE_ALREADY_EVALUATED"></a>

### `ERR_MODULE_ALREADY_EVALUATED`

A module cannot be evaluated twice using the `evalaute` customization hook.

<a id="ERR_MODULE_NOT_FOUND"></a>

### `ERR_MODULE_NOT_FOUND`
Expand Down
55 changes: 55 additions & 0 deletions doc/api/module.md
Original file line number Diff line number Diff line change
Expand Up @@ -923,6 +923,61 @@ hook that returns without calling `next<hookName>()` _and_ without returning
prevent unintentional breaks in the chain. Return `shortCircuit: true` from a
hook to signal that the chain is intentionally ending at your hook.

#### `evaluate(context, nextEvaluate)`

<!-- YAML
added:
- REPLACEME
-->

> Stability: 1.1 - Active Development

* `context` {Object} An object that will be used along the evaluation of a module.
It contains the following immutable properties.
* `module` {Object} The `Module` instance of the module being evaluated.
* `format` {string} The format of the module, which may be one of the list of
acceptable values described in the [`load` hook][load hook].
Comment on lines +926 to +939
Copy link
Member

@GeoffreyBooth GeoffreyBooth Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like a signature that would be more consistent with the other hooks would be evaluate(module, context, nextEvaluate) where context is just { format }.

Besides consistency, if you made this change perhaps you wouldn't need the special buildEvaluateHooks that this PR adds, and evaluate could use the same chaining helpers as the other hooks?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My hesitance about it is primarily that module should not be changed when nextEvaluate is invoked in an evaluate hook invoked for a specific module (it is already a bit problematic for resolve or load hooks to switch to a different argument in the middle of the chain, but that's usually more okay because resolution and loading tend to be side-effect-free. Evaluation, however, is not). But I guess with a module first argument it's still possible to check this.

I don't think buildEvaluateHooks can reuse the previous chaining helpers though, because nextEvaluate should not accept change of arguments like the others, for the reason mentioned above. It doesn't make a lot of sense to attempt evaluating a source text module as addon, for example (what does that even mean?), so a check on the arguments will need to be specific to the evaluate hook.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My hesitance about it is primarily that module should not be changed when nextEvaluate is invoked in an evaluate hook invoked for a specific module

I would think that whatever check you do that context.module isn't changed could just as easily be done against module as the first argument. It hadn't occurred to me that users might change the resolve specifier or the load url values when calling next, so maybe it makes sense to add a similar check for those too.

The buildEvaluateHooks comment was just an idea for an optimization, if it were possible. If not, so it is.

* `nextEvaluate` {Function} The subsequent `evaluate` hook in the chain, or the
Node.js default `evaluate` hook after the last user-supplied `evaluate` hook.
This hook does not take any arguments.
* Returns: {Object}
* `returned` {any} When the module is being required and it is a CommonJS module,
this contains the value that the CommonJS module returns, if
it uses any top-level `return` statement.
* `shortCircuit` {undefined|boolean} A signal that this hook intends to
terminate the chain of `evaluate` hooks. **Default:** `false`

The `evaluate` hook is run after the `resolve` and `load` hook,
abstracting the final execution of the code in the module. It is only
available to `module.registerHooks`. It is currently only run for the
execution of the following modules:

1. CommonJS modules, either being `require`d or `import`ed. In this
case, `context.module` equals to the `module` object in the CommonJS
module being evaluated, and `context.module.exports` is mutable.
2. ECMAScript modules that are `require`d. This hook would only be run
for the evaluation of the module being directly `require`d, but
could not be run for each of its inner modules being `import`ed. In
this case, `context.module` is a `Module` wrapper around the
ECMAScript modules. Reassigning `context.module.exports` to
something else only affects the result of `require()` call, but
would not affect access within the ECMAScript module. Properties of
`context.module.exports` (exports of the ECMAScript module) are not
mutable.

In future versions it may cover more module types, but the following
are unlikely to be supported due to restrictions in the ECMAScript
specifications:

1. The ability to skip evaluation of an inner ECMAScript module being
`import`ed by ECMAScript modules.
2. The ability to mutate the exports of a ECMAScript module.

For the ability to customize execution and exports of all the
ECMAScript modules in the graph, consider patching the source code of
the ECMAScript modules using the [`load` hook][load hook] as an imperfect
workaround.

#### `initialize()`

<!-- YAML
Expand Down
1 change: 1 addition & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -1578,6 +1578,7 @@ E('ERR_MISSING_ARGS',
return `${msg} must be specified`;
}, TypeError);
E('ERR_MISSING_OPTION', '%s is required', TypeError);
E('ERR_MODULE_ALREADY_EVALUATED', 'Module cannot be evaluated twice', Error);
E('ERR_MODULE_NOT_FOUND', function(path, base, exactUrl) {
if (exactUrl) {
lazyInternalUtil().setOwnProperty(this, 'url', `${exactUrl}`);
Expand Down
71 changes: 53 additions & 18 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ const kIsMainSymbol = Symbol('kIsMainSymbol');
const kIsCachedByESMLoader = Symbol('kIsCachedByESMLoader');
const kRequiredModuleSymbol = Symbol('kRequiredModuleSymbol');
const kIsExecuting = Symbol('kIsExecuting');
const kHasBeenEvaluated = Symbol('kHasBeenEvaluated');

const kURL = Symbol('kURL');
const kFormat = Symbol('kFormat');
Expand All @@ -122,6 +123,7 @@ module.exports = {
kIsCachedByESMLoader,
kRequiredModuleSymbol,
kIsExecuting,
kHasBeenEvaluated,
};

const { BuiltinModule } = require('internal/bootstrap/realm');
Expand Down Expand Up @@ -169,6 +171,8 @@ const {
registerHooks,
resolveHooks,
resolveWithHooks,
evaluateWithHooks,
evaluateHooks,
} = require('internal/modules/customization_hooks');
const { stripTypeScriptModuleTypes } = require('internal/modules/typescript');
const packageJsonReader = require('internal/modules/package_json_reader');
Expand All @@ -184,6 +188,7 @@ const {
ERR_INVALID_ARG_TYPE,
ERR_INVALID_ARG_VALUE,
ERR_INVALID_MODULE_SPECIFIER,
ERR_MODULE_ALREADY_EVALUATED,
ERR_REQUIRE_CYCLE_MODULE,
ERR_REQUIRE_ESM,
ERR_UNKNOWN_BUILTIN_MODULE,
Expand Down Expand Up @@ -1712,31 +1717,59 @@ Module.prototype._compile = function(content, filename, format) {
}
}

if (format === 'module') {
loadESMFromCJS(this, filename, format, content);
return;
function defaultEvaluate(context) {
const mod = context.module;
if (mod[kHasBeenEvaluated]) {
throw new ERR_MODULE_ALREADY_EVALUATED();
}
const filename = mod.filename;
if (context.format === 'module') {
// TODO(joyeecheung): consider putting content in the context and this doesn't
// need to be a closure.
loadESMFromCJS(mod, filename, format, content);
mod[kHasBeenEvaluated] = true;
// ESM do not have top-level returns.
return { __proto__: null, returned: undefined };
}

const dirname = path.dirname(filename);
const require = makeRequireFunction(mod, redirects);
const expts = mod.exports;
const thisValue = expts;
if (requireDepth === 0) { statCache = new SafeMap(); }
const args = [expts, require, mod, filename, dirname];

// TODO(joyeecheung): consider putting compiledWrapper in the context and this doesn't
// need to be a closure.
// This is whatever returned by the wrapped function, note that users might not assign it to exports
// in case they put a return statement directly in CJS.
let returned;
if (mod[kIsMainSymbol] && getOptionValue('--inspect-brk')) {
const { callAndPauseOnStart } = internalBinding('inspector');
returned = callAndPauseOnStart(compiledWrapper, thisValue, ...args);
} else {
returned = ReflectApply(compiledWrapper, thisValue, args);
}
mod[kHasBeenEvaluated] = true;

if (requireDepth === 0) { statCache = null; }
return { __proto__: null, returned };
}

const dirname = path.dirname(filename);
const require = makeRequireFunction(this, redirects);
let result;
const exports = this.exports;
const thisValue = exports;
const module = this;
if (requireDepth === 0) { statCache = new SafeMap(); }
setHasStartedUserCJSExecution();
this[kIsExecuting] = true;
if (this[kIsMainSymbol] && getOptionValue('--inspect-brk')) {
const { callAndPauseOnStart } = internalBinding('inspector');
result = callAndPauseOnStart(compiledWrapper, thisValue, exports,
require, module, filename, dirname);
format ||= 'commonjs'; // At this point, it's considered CommonJS.
let returned;
if (evaluateHooks.length > 0) {
const result = evaluateWithHooks(this, format, defaultEvaluate);
returned = result?.returned;
} else {
result = ReflectApply(compiledWrapper, thisValue,
[exports, require, module, filename, dirname]);
// Avoid creating a full ModuleEvaluateContext for fully internal code.
const result = defaultEvaluate({ __proto__: null, format, module: this });
returned = result?.returned;
}
this[kIsExecuting] = false;
if (requireDepth === 0) { statCache = null; }
return result;
return returned;
};

/**
Expand Down Expand Up @@ -1907,6 +1940,7 @@ Module._extensions['.js'] = function(module, filename) {
Module._extensions['.json'] = function(module, filename) {
const { source: content } = loadSource(module, filename, 'json');

// TODO(joyeecheung): invoke evaluate hook with 'json' format.
try {
setOwnProperty(module, 'exports', JSONParse(stripBOM(content)));
} catch (err) {
Expand All @@ -1922,6 +1956,7 @@ Module._extensions['.json'] = function(module, filename) {
*/
Module._extensions['.node'] = function(module, filename) {
// Be aware this doesn't use `content`
// TODO(joyeecheung): invoke evaluate hook with 'addon' format.
return process.dlopen(module, path.toNamespacedPath(filename));
};

Expand Down
Loading
Loading