module: add preImport loader hook #43245
Conversation
|
Review requested:
|
nodejs-github-bot
added
lib / src
|
Can you open a PR on the loaders repo to suggest a change to the design doc? We should discuss and agree upon a design first before landing new APIs. |
|
With the sync resolve PR now merged, I've marked this PR as ready for further review on the API. The loaders repo API proposal has now been open for three weeks as well in nodejs/loaders#83. Based on feedback I will then prepare the final work for landing further in terms of fleshing out tests for the various edge cases. |
guybedford
changed the title
guybedford
changed the title
|
@GeoffreyBooth having gone through the process as requested, and had three weeks of zero response, I would appreciate if you could advise with respect to next steps. I would have blocked the resolve hook sync PR from landing had I know we would ship that without this. |
There was a problem hiding this comment.
I discussed this with @guybedford. I’d like this hook added to the loaders API design doc via a PR on that repo that gets reviewed by folks interested in loaders who may be missing the now-removed async resolve hook. I want to make sure the design of the proposed hook satisfies the use cases that they’re trying to achieve. Most (possibly all) of the relevant stakeholders are the people who participated in the discussion on this thread: nodejs/loaders#64.
| export async function preImport(specifier, context) { | ||
| if (context.topLevel) | ||
| console.log(`Top-level load of ${specifier}`); | ||
| else | ||
| console.log(`Dynamic import of ${specifier} in ${context.parentURL}`); | ||
| } |
There was a problem hiding this comment.
Can this please be a realistic example that shows how this hook would be used for one of its primary use cases? Since the hook doesn’t return anything, I’m confused as to how this would affect a resolve hook.
|
Woh! Sorry @guybedford! I don't know how I missed both this PR and the |
| /** | ||
| * Whether this loader has userland hooks. | ||
| * @private | ||
| * @property {boolean} hasLoaderHooks | ||
| */ | ||
| #hasLoaderHooks = false; | ||
|
|
||
| /** | ||
| * Prior to ESM loading. These are called once before any modules are started. | ||
| * @private | ||
| * @property {KeyedHook[]} globalPreloaders Last-in-first-out | ||
| * @property {KeyedHook[]} globalPreloadHooks Last-in-first-out | ||
| * list of preload hooks. | ||
| */ | ||
| #globalPreloaders = []; | ||
| #globalPreloadHooks = []; |
There was a problem hiding this comment.
@guybedford I like the refactoring you did in this PR. Can it be split off into its own PR, without the added preImport hook? cc @JakobJingleheimer
And per https://github.com/nodejs/loaders/blob/main/doc/meetings/2022-06-21.md#plan-per-today we should probably close this PR, at least for now while we pursue the threads approach.
There was a problem hiding this comment.
Oow. #hasCustomHooks 🙂 (but actually, I think that's not as useful as this.#HOOK_NAME.length > 1?)
FYI, I was about to tidy the hooks up to something like:
#chains = {
globalPreload: [],
resolve: [],
load: [],
}
Adds a new
preImportloader hook which is called before each top-level import operation (dynamic and host-defined).In the chaining model these hooks run in parallel and block subsequent module pipeline work for the graph until they all resolve. This allows tracking and resolution cache preparation.
The goal for this PR is to land an acceptable API that can then lead to the async resolve hook being entirely deprecated. If we can get to such a point we will then be able to obtain full alignment on
import.meta.resolvebeing sync.This is a draft PR for now, with the docs and API changes untested simply to illustrate the concept. Tests will be fleshed out after further review if there is agreement on the overall shape.
//cc @nodejs/modules @nodejs/loaders