Repository navigation
module: remove async module customization hooks - #67090
Conversation
|
Review requested:
|
nodejs-github-bot
added
lib / src
a98403b to
95c14fc
Compare
6cea4a9 to
355da9b
Compare
355da9b to
aab37fe
Compare
aab37fe to
ada3473
Compare
GeoffreyBooth
added
module
ada3473 to
937a866
Compare
937a866 to
a825144
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #67090 +/- ##
==========================================
- Coverage 90.70% 90.67% -0.04%
==========================================
Files 801 798 -3
Lines 281881 279829 -2052
Branches 54315 54018 -297
==========================================
- Hits 255675 253725 -1950
+ Misses 16353 16238 -115
- Partials 9853 9866 +13
🚀 New features to boost your workflow:
|
|
since it's simplify loading phase, it's there are an positive impact in term of perf ? |
|
I think this should be treated as semver major due to the large impact |
I ran Node’s
So there’s no regression, and at most a small gain in the import path, because loading no longer has to check for async hooks. The larger win is for hook users. In a separate hyperfine run, a script using benchmark results (
|
| Scenario | Before | After |
|---|---|---|
node -e 0 |
99.1 ± 5.0 ms | 99.0 ± 5.7 ms |
| ESM entry (three modules) | 107.1 ± 7.7 ms | 105.6 ± 5.6 ms |
| CJS entry | 104.8 ± 4.3 ms | 103.1 ± 4.6 ms |
The removal mainly benefits hook users. A script using module.register() async hooks took about 141 ms, versus about 107 ms with the equivalent registerHooks() (about 24% faster). That gain comes from the sync API, which is already on main.
|
@marco-ippolito I’m happy for this to land only in the 27 line with I’d still argue it isn’t semver-major in the policy sense, because
We also went well beyond what an experimental API requires. After the discussion in #62395 (e.g. “experimental status is already a warning”, and the concerns about existing adoption), we shipped a docs-only deprecation in v25.9.0/v24.15.0 (#62395) and a runtime deprecation in v26.0.0 (#62401). Users have had a full major of runtime warnings, and the plan to remove in 27 has been on record since #62401. |
GeoffreyBooth
added
the
dont-land-on-v26.x
|
Yes that works for me |
db42af1 to
93a8f1b
Compare
GeoffreyBooth
added
author ready
Remove `module.register()` (DEP0205), the `--experimental-loader` and `--loader` flags, and the machinery that supported them: the loader hooks worker thread, the hooks proxy, the async module customization hook chains for resolving and loading, the internal worker class used to run the loader thread, and the asynchronous module job linking. Module customization is now provided only by `module.registerHooks()`, which runs synchronously on the thread that loads the modules. As the loader thread no longer exists, the ES module loader always loads and links modules synchronously, so the code that handled promises and the re-invented `require()` for imported CommonJS customized by the async module customization hooks is removed too. `import()` still defers evaluation of the module to a later microtask, as before. `worker_threads.isInternalThread` is kept for compatibility and is now always `false`; removing it is left for a future semver-major change. The `ERR_ASYNC_LOADER_REQUEST_NEVER_SETTLED` and `ERR_LOADER_CHAIN_INCOMPLETE` error codes are no longer used and are documented as removed. Tests that exist only to exercise the async module customization hooks are removed. Assisted-by: Claude Signed-off-by: Geoffrey Booth <[email protected]>
93a8f1b to
96518bf
Compare
|
I needed to rebase to resolve conflicts. Can I please get a fresh approval? |
GeoffreyBooth
added
the
commit-queue
|
Landed in cb05e4d |
nodejs-github-bot
removed
the
commit-queue
GeoffreyBooth
added
the
notable-change
|
The
notable-change
Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section. |
|
I should've added |
Renegade334
added
dont-land-on-v22.x
Removes
module.register()(DEP0205), the--experimental-loaderand--loaderflags, and the machinery that supported them: the loader hooks worker thread, the hooks proxy, the async module customization hook chains for resolving and loading, the internal worker class used to run the loader thread, and the asynchronous module job linking. Module customization is now provided only bymodule.registerHooks(), which runs synchronously on the thread that loads the modules.require()for imported CommonJS are gone.import()still defers evaluation to a later microtask.worker_threads.isInternalThreadis kept and alwaysfalse; removing it is left for a semver-major change.deprecations.mdas End-of-Life.ERR_ASYNC_LOADER_REQUEST_NEVER_SETTLEDandERR_LOADER_CHAIN_INCOMPLETEmove to Legacy inerrors.md.require()after a failedimport()and thatimport()does not evaluate the module before returning.REPLACEMEplaceholders for the version are left for landing.Notable change
Suggested text for the release notes:
Removal of
module.register()and--experimental-loadermodule.register()(DEP0205) and the--experimental-loaderand--loaderflags have been removed, together with the separate thread that ran asynchronous module customization hooks. Module customization is now provided bymodule.registerHooks(), which runs synchronously on the thread that loads the modules. It has been available since v22.15.0 and v23.5.0.What changes for existing code:
module.registeris no longer exported. In CommonJS it isundefined, so feature detection such asif (module.register)keeps working, and in ES modulesimport { register } from 'node:module'fails with aSyntaxErrorbecause the export no longer exists.--experimental-loaderand--loaderare no longer recognized options, so Node.js exits with a “bad option” error when they are passed.awaitasynchronous work, need to do that work another way.