module: implement synchronous module evaluate hooks #57139
Conversation
|
Review requested:
|
nodejs-github-bot
added
errors
joyeecheung
force-pushed
the
evaluate-hooks
branch
from
079b279 to
077965a
Compare
This patch adds an `evaluate` hook that can be used as a supported
alternative to monkey-patching the CJS module loader, when the
primary use case is to modify the exports of modules, with the
goal of reducing dependency on CJS loader monkey-patching in
the wild.
```js
// mod.js
exports.prop = 'original';
// hook.js
require('module').registerHooks({
evaluate(context, nextEvaluate) {
context.module.exports; // Access the exports before evaluation.
nextEvaluate(); // Invokes default evaluation. Can be skipped.
context.module.exports.prop; // 'original'
context.module.exports.prop = 'updated'; // Update exports.
},
});
require('./mod.js'); // { prop: 'updated' }
```
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 as an imperfect
workaround.
joyeecheung
force-pushed
the
evaluate-hooks
branch
from
077965a to
9a7985c
Compare
github-actions
bot
removed
the
request-ci
|
Given this is CJS-only I think we should clearly call the hook |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #57139 +/- ##
==========================================
- Coverage 89.03% 89.03% -0.01%
==========================================
Files 665 665
Lines 193408 193595 +187
Branches 37279 37305 +26
==========================================
+ Hits 172206 172369 +163
- Misses 13886 13893 +7
- Partials 7316 7333 +17
|
The idea is that we'll have non-mutable support for ESM (e.g. for tracing perf or diagnostics) when we have https://issues.chromium.org/issues/384413088, not that it will never support ESM. Also I am not sure if you have noticed, but this supports require(esm) as well as import cjs. It will also be able to support import json, import wasm and import addon. The only thing it cannot support with just Node.js changes is import esm, which requires a V8 change, which is still possible, just that the hook has to pretend that ESM has a frozen |
|
I see that it supports My feedback is just that a specific name might be useful to make the distinction clear to end users that this hook only applies to CJS and ESM CJS wrappers. |
I am not sure why this is not clear from OP or from the comment above, I guess I will have to repeat again, this will be able to support
It's just not done in this PR, and left as TODOs. Does it make sense to ask people to use a hook named cjsEvaluate to customize the edges above? |
|
This would not apply to JSON, Wasm and Addons imported into ES modules though right? |
I did have the same thought as Guy, that perhaps people would be confused about what this applies to, but I agree that if it's not CommonJS-specific then we shouldn't name it If the purpose of this is to modify the exports, we could potentially call it |
| #### `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]. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
My hesitance about it is primarily that
moduleshould not be changed whennextEvaluateis invoked in an evaluate hook invoked for a specificmodule
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.
It would. They are actually easier to cover because they are mutable (since synthetic module exports are mutable externally) and there is already a evaluation callback phase, and I already left TODO in this PR where the hook can be implemented for these. Only import SourceTextModule need a V8 change and would have a bit more restriction (exports won't be directly mutable. But the hook can still be invoked, and at least can modify deeper properies of the exports..). If you absolutely have to see it I can e.g. implement the hook for import addons in this PR and demonstrate how to skip addons initialization when they are imported into a source text module and point the exports to something else (or I can think of some interesting use case like toggling Linux perf before and after the initialization of an addon or the invocation of its method to trace syscalls made by it). But I think it should be rather obvious in the TODO I left in the code here how it can be implemented without actually implementing it to prove that it's implementable (and I mostly do not want to implement it here because I want to consolidate the addon evaluation in |
It will also allow skipping default evaluation entirely for most modules except source text modules (synthetic module evaluation, or wrapper evaluation like imported WASM, is still controlled by Node.js and skippable). Just that the primary motivation of this for existing use case is still modifying exports. But I can see it being used for e.g. implementing stubs, or tracing evaluation of modules for diagnostics. |
What if the ultimate way source text modules are supported does not align with this hook design? Do we then need an |
I think the default evaluate for source text module can just be a no-op, or throw an error, or just be undefined, with some additional context parameter signaling this case and a documentation note about the limitation. In any case, it's going to be experimental, if we do not version all the other experimental APIs like this when we change them, I don't see why this has to be an exception? (we didn't even rename the hooks when they were moved off-thread, whatever changes made for ESM here can't be any more drastic than that..) |
|
My honest opinion here is I don't think there is a future where this hook can work for source text modules, therefore I would prefer an API that takes that into account from the start. Whether or not you agree with me, if I am right, then it seems strange to me to have a hook called "evaluate" that will never apply to source text modules. |
We do have I think at worst this could still be a post-evaluate hook for source text modules (that does not even strictly require a V8 change...but it would be less hacky if it is). IMO
If you think |
Since it is operating on a I also have concerns about it applying for JSON and Wasm in that it would act on a CJS-style |
I don't think it will be a |
|
Right, but until it's clear there's a path forward for such a construct, perhaps let's leave the design space open for now, to avoid the future breaking change that would be needed, even though the API is experimental. |
|
Since we are operating on |
I am not sure what you mean by leaving the design space open - do you mean not adding an evaluate hook, and prefer to let user land continue monkey patching the CJS loader instead? I wonder if we can have a compromise of naming the CJS module wrapper |
|
I think that something like you describe might be closer to the right direction here. Properly plotting what the path looks like for a more general hook. But then what about a My original feedback was very simple - let's name this something specific to the CommonJS |
Yes.
I think none of these are better than
Even for imported source text module, we can at least invoke it post-evaluation. I don't think it's a much of a problem to just document that for imported source text modules, default evaluation cannot be controlled, or just add different hooks for imported source text modules, and say that it's an exception. It seems rather unfortunate to name the hook based on one exception, and is not consistent with how we name the async load/resolve hooks even though they also have their own exceptions. |
There was a problem hiding this comment.
@joyeecheung per the discussion above and since no compromise has been reached, I'm marking my feedback to make the name less general than "evaluate" so long as the design does not incorporate support for source text module.
The design does incorporate support for source text modules, for example, this would be possible: evaluate(context, nextEvaluate) {
if (context.esModule) {
if (context.esModule.isSourceTextModule)
if (context.esModule.status === 'evaluated') { // It's already evaluated at this point, because it's imported
nextEvaluate(); // This just invokes other evaluate hooks, which also need to handle this case.
context.esModule.namespace.test = 1; // This does not work, because STM exports are frozen.
context.esModule.namespace.test.prop = 2; // This does work, if user doesn't freeze it
} else { // context.esModule.status === 'evaluating', it's being required for the first time
nextEvaluate(); // This may invoke default evaluation - can be skipped.
context.esModule.namespace.test.prop = 2;
}
}
if (!context.esModule.isSourceTextModule) {
context.esModule.setExport('test', 1); // This works, because it's a synthetic module.
}
}
} |
This hook has two properties: (1) monkey patching existing instances, and (2) intercepting the execution of ESM modules down the graph including this @joyeecheung I'd be interested to hear further about why the Will be great to follow up in the loaders meeting. |
I am not sure if you have read the comments, but it's spec-complaint - when imported source text modules arrive in this hook, it's status will always already be 'evaluated'. In this case,
I think this is the most intuitive name, while others are all more confusing in their own ways. As said before this is not a CJS-only hook, so using
I won't be able to go to the loaders meeting next week, the other one in 3 weeks might work. |
The problem here is So there's a lot more engagement needed to figure out the general spec compatible path forward here. I don't want to slow things down though which is why I suggested a rename so this can land immediately and then we can follow up with discussions. But if you want to go through the discussions first and let this sit for a while I'm happy to do that too. |
If we just say that
This hook is not planned to support skipping evaluation of imported source text modules, as already mentioned in the OP and in the documentation added in the PR, and several times in the comments abbove. |
An evaluation hook that does not apply at all in applications that only use ES modules is not an evaluation hook, so should have a different name. |
I would prefer landing it with an intuitive name instead of landing it with a confusing name, only to rename later. With this design it's still possible to extend support in an additive manner, for example, adding
Prior to |
|
@joyeecheung I really think something like |
|
Sure, but I won't be working on this next week. I can get back to it later - there isn't really rush for this, just that something like this has been long overdue and not providing it can keep deeper in the mud of monkey patching at the mean time. My plan really was to present it during the collab summit to get some input from the APM folks and people who maintain other kind of monkey patching, and see if there are other unknown use cases that can be covered without dragging others down, or ability that could've been provided but got missed out in this design. While we already know that it likely won't be the panacea that solves all of the pain points in module customization (e.g. fixing all the edge cases in import-in-the-middle, because our hands are tied), at least it hopefully improves the situation for others (e.g. allowing require-in-the-middle and all sorts of mocking libraries to be less brittle). Personally I don't mind waiting to get more input to make sure we are doing the right thing, though I am not really looking for philosophical questions or how we can generalize it for all use cases - that already seems to be a lost cause to me after reading about the woes of the maintainers of import-in-the-middle trying to handle the edge cases in customizing ESM. I think at this point, we can only either add a hook that is as powerful as what can be offered for the modules whose evaluation is controlled by Node.js, or a hook that works the same for all modules but end up being too restricted due to having to support modules whose evaluation is not controlled by Node.js. I'd prefer the former, since if users do need an abstraction, they can and do already compose an abstraction with a powerful evaluate hook (nowadays just monkey patching) and a load hook to generalize for cases that they care about, but they can't get the power a proper evaluate hook offers with only high-level abstractions - that basically is how we are still stuck with widespread usage of CJS monkey patching today, because the need for that customization power is there. |
This patch adds an
evaluatehook that can be used as a supported alternative to monkey-patching the CJS module loader, when the primary use case is to modify the exports of modules, with the goal of reducing dependency on CJS loader monkey-patching in the wild.The
evaluatehook is run after theresolveandloadhook, abstracting the final execution of the code in the module. It is only available tomodule.registerHooks. It is currently only run for the execution of the following modules:required orimported. In this case,context.moduleequals to themoduleobject in the CommonJS module being evaluated, andcontext.module.exportsis mutable.required. This hook would only be run for the evaluation of the module being directlyrequired, but could not be run for each of its inner modules beingimported. In this case,context.moduleis aModulewrapper around the ECMAScript modules. Reassigningcontext.module.exportsto something else only affects the result ofrequire()call, but would not affect access within the ECMAScript module. Properties ofcontext.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:
imported by ECMAScript modules.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
loadhook as an imperfect workaround.Refs: #56241
In the test I included a POC for building https://www.npmjs.com/package/require-in-the-middle on top of it instead of monkey-patching. Support for (non-mutable) ESM evaluation depends on https://issues.chromium.org/u/1/issues/384413088 - mutability of ESM exports is off the table for V8 without spec support, so it's a non-goal/out of scope for this hook, and this mostly focus on at least sunsetting CJS monkey-patching.