module: refactor and clarify async loader hook customizations #60278
Conversation
- This updates the comments that assume loader hooks must be async
- Differentiate the sync/async loader hook paths in naming
`#customizations` is now `#asyncLoaderHooks` to make it clear
it's from the async APIs.
- Differentiate the paths running on the loader hook thread
(affects the loading of async other loader hooks and are async)
v.s. paths on the main thread calling out to code on the loader
hook thread (do not handle loading of other async loader hooks, and
can be sync by blocking).
- `Hooks` is now `AsyncLoaderHooksOnLoaderHookWorker`
- `CustomizedModuleLoader` is now
`AsyncLoaderHooksProxiedToLoaderHookWorker` and moved into
`lib/internal/modules/esm/hooks.js` as it implements the same
interface as `AsyncLoaderHooksOnLoaderHookWorker`
- `HooksProxy` is now `AsyncLoaderHookWorker`
- Adjust the JSDoc accordingly
- Clarify the "loader worker" as the "async loader hook worker"
i.e. when there's no _async_ loader hook registered, there won't
be this worker, to avoid the misconception that this worker
is spawned unconditionally.
- The code run on the loader hook worker to process
`--experimental-loader` is moved into
`lib/internal/modules/esm/worker.js` for clarity.
- The initialization configuration `forceDefaultLoader` is split
into `shouldSpawnLoaderHookWorker` and `shouldPreloadModules`
as those can be separate.
- `--experimental-vm-modules` is now processed during pre-execution
and no longer part of the initialization of the built-in ESM
loader, as it only exposes the vm APIs of ESM, and is unrelated
to built-in ESM loading.
|
Review requested:
|
nodejs-github-bot
added
lib / src
| switch (doEval) { | ||
| case 'internal': { | ||
| // Create this WeakMap in js-land because V8 has no C++ API for WeakMap. | ||
| internalBinding('module_wrap').callbackMap = new SafeWeakMap(); |
There was a problem hiding this comment.
This is removed as a drive-by, the callbackMap is no longer used after #48510
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #60278 +/- ##
==========================================
+ Coverage 88.56% 88.58% +0.01%
==========================================
Files 704 704
Lines 208323 208369 +46
Branches 40033 40038 +5
==========================================
+ Hits 184501 184581 +80
+ Misses 15854 15823 -31
+ Partials 7968 7965 -3
🚀 New features to boost your workflow:
|
| /** | ||
| * Interface for classes that implement asynchronous loader hooks that can be attached to the ModuleLoader | ||
| * via `ModuleLoader.#setAsyncLoaderHooks()`. | ||
| * @typedef {object} AsyncLoaderHooks |
There was a problem hiding this comment.
Nit: the reason for the “customization“ and “customization hooks“ names is to avoid confusion where some might think that these hooks only affect loading, when they also impact resolution and other things. It hasn’t been a priority to update the code, but as long as we’re adding new code and defining new names, I think the “customization“ name is more informative and correct.
| * @typedef {object} AsyncLoaderHooks | |
| * @typedef {object} AsyncCustomizationHooks |
And elsewhere that the code uses “loader hook“ that this branch touches.
There was a problem hiding this comment.
In that case should AsyncLoaderHooksOnLoaderHookWorker be AsyncCustomizationHooksOnCustomizationHookWorker? That feels rather lengthy..
I feel that "loader hooks" contain already ample information about it: it's loader, which implies it's not just "loading" phase but everything you'd see in loader.js and the ModuleLoader class. I can see that "load" would be ambuiguous, but "loader" isn't, and "hooks" implies customizations (or the hooks aren't necessarily for customizations - they can also be for observations).
There was a problem hiding this comment.
It's just a nit, do with it as you will. AsyncCustomizationHooksOnCustomizationHookWorker is the same number of words as with Loader, it's just a longer word. It's a mouthful in either version. Perhaps just AsyncOffThreadCustomizationHooks?
Another name instead of “customization hooks“ is “module hooks“ which is just as descriptive and the same number of letters as “loader hooks”.
There was a problem hiding this comment.
I appreciate the brainstorm ;) though to me "foo hooks" means "it's hooking into foo/hook placed in foo", so I'd have some questions when I see "module hooks" - is it hooking into the code of user modules/hook placed in the modules? While it can be used in such a way that inject code into user modules, that is more of the output, instead of the process. "loader hooks" seem more intuitive because, there's a ModuleLoader in loader.js next to it, which either new AsyncLoaderHooksProxiedToLoaderHookWorker itself, or get a new AsyncLoaderHooksOnLoaderHookWorker passed into it, and the two classes have methods that are hooking into the methods of ModuleLoader with the same name, so that makes a lot of sense.
(I thought about just calling it async hooks but then unfortunately that combination might be a bit too well known as something completely unrelated in Node.js..)
3 participants
Paving the way for #59666, this patch does not introduce observable changes, mostly clarifying which paths are sync/async, or run on the async loader hook thread/on the non-async-loader-hook thread, and updating the comments so that it's easier to sync-ify the paths later.
#customizationsis now#asyncLoaderHooksto make it clear it's from the async APIs.Hooksis nowAsyncLoaderHooksOnLoaderHookWorkerCustomizedModuleLoaderis nowAsyncLoaderHooksProxiedToLoaderHookWorkerand moved intolib/internal/modules/esm/hooks.jsas it implements the same interface asAsyncLoaderHooksOnLoaderHookWorkerHooksProxyis nowAsyncLoaderHookWorker--experimental-loaderis moved intolib/internal/modules/esm/worker.jsfor clarity.forceDefaultLoaderis split intoshouldSpawnLoaderHookWorkerandshouldPreloadModulesas those can be separate.--experimental-vm-modulesis now processed during pre-execution and no longer part of the initialization of the built-in ESM loader, as it only exposes the vm APIs of ESM, and is unrelated to built-in ESM loading.