module.registerHooks() tracking issue

[joyeecheung]

• [x] Initial proposal: https://github.com/nodejs/node/issues/52219
• [x] Loaders design proposal: https://github.com/nodejs/loaders/pull/198
• [x] Initial implementation: https://github.com/nodejs/node/pull/55698
• [x] Make the internal resolution and loading paths synchronous, to reduce the chance of races https://github.com/nodejs/node/pull/60380 so that module.register() can be an helper built on top of module.registerHooks(), and internally we only have one set of hooking points to take care of we can't really polyfill module.register with it if we want to keep the internals to ourselves, oh well
• [x] Handle with the nullish sources of CommonJS caused by module.register() quirk during interop: https://github.com/nodejs/node/pull/59929
• [ ] Implement evaluation hook for at least CJS to help use cases like require-in-the-middle, which was supposed to be based on https://github.com/nodejs/loaders/pull/198 but we had some discussions over the collaboration summit/NodeConf EU about changing the hook to be "wrapping around evaluation", not "after evaluation": WIP
  • For ESM, there's currently no way for Node.js to hook into the evaluation of child modules, will need to discuss with V8 about it (issue). Allowing mutation of the exports would require a spec change so out of scope for module.registerHooks() or Node.js in general
• [ ] Implement link hook for ESM, as proposed in https://github.com/nodejs/loaders/pull/198 to make use cases like import-in-the-middle less hacky
• [ ] Symbol.dispose integration (requested in https://github.com/nodejs/node/pull/55698#discussion_r1855211481)
• [x] Reordering of the documentation when module.registerHooks() is battle tested enough and should be preferred over module.register to avoid various caveats https://github.com/nodejs/node/issues/56241
• [ ] Advocating it to popular npm packages doing CJS monkey-patching to reduce the overall dependency of CJS loader internals in the ecosystem
• [ ] Support of unknown extensions (learned about it when I was initially proposing https://github.com/nodejs/node/issues/52219)
• [ ] Support node:builtin?param=val
• [ ] Figure out how to make search params work with CJS cache
• [-] Create a polyfill of module.register() built on top of module.registerHooks() (investigated in https://github.com/nodejs/node/issues/59666, not feasible without exposing too many internals)
• [ ] startGraph hook proposed in https://github.com/nodejs/loaders/pull/205

[joyeecheung]

cc @nodejs/loaders

[jsumners-nr]

FYI https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/adding-sub-issues

[legendecas]

Off-topic: the new sub-issue feature is not enabled for the nodejs org yet: https://github.com/nodejs/admin/issues/921

[joyeecheung]

I have a WIP for the evaluate hook which already works for a mock of require-in-the-middle. Pending on resolution about whether/how ESM can play into it. Opened an issue in Chromium to discuss about ESM evaluation hook: https://issues.chromium.org/u/1/issues/384413088 (mutability of exports would probably be out of scope of V8 as that's mandated by the spec, but at least we can address the use case where the hook does not need to patch and simply wants to observe).

[timokoessler]

Thanks for your great work! Are there any plans to officially support modifying the exports of builtins (to instrument them)? One workaround we are exploring is to return commonjs code for builtins. In this code we import, modify and re-export the original builtin module. This does not seem to be bulletproof as Node.js e.g. adds a module.exports property to the exports when importing in ESM. Also returning code with the module format does not work because Node.js tries to read the non-existing package.json of the builtin. We want to use the new hook system to add ESM support to AikidoSec/firewall-node.

[joyeecheung]

Are there any plans to officially support modifying the exports of builtins (to instrument them)?

The current priority is mostly testing out module.registerHooks() and making it an equivalent replacement for module.register(), and then re-implementing module.register() on top of it. I don't think there is active work being put into the use case of modifying builtins, though volunteers are welcomed to drive it. If it requires a new hook, it probably deserves some thoughts into the design considering how caching and ESM namespace detection complicates things. An approach based on the load hook may be easier to implement off the top of my head.