Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: preImport loader hook #83

Closed
guybedford opened this issue May 29, 2022 · 3 comments
Closed

Proposal: preImport loader hook #83

guybedford opened this issue May 29, 2022 · 3 comments

Comments

@guybedford
Copy link

A preImport hook can take the place of the async use cases for having an async resolver hook by allowing any async work to be done upfront before triggering the further pipeline steps. For example, loading an import map could be an async operation prior to synchronously using that import map in resolution (which is exactly what the browser does).

Ultimately, the goal would be that by covering these needs, this should allow for a synchronous core resolver which would enable unification with browser resolution.

The initial hook PR would not deprecate the resolver yet, and could be released to get feedback on this first, before following up with a subsequent async resolver deprecation.

There is a basic draft API in nodejs/node#43245, with the following documentation:

preImport(specifier, context)
* `specifier` {string}
* `context` {Object}
  * `conditions` {string\[]} Resolution conditions of the current environment,
    as defined for the `package.json` imports and exports fields
  * `dynamic` {boolean} Whether this import is a dynamic `import()`
  * `importAssertions` {Object}
  * `parentURL` {string|undefined} The module importing this one, or undefined
    if this is the Node.js entry point

The preImport hook allows for tracking and asynchronous setup work for every
top-level import operation.
The preImport hook is called for each top-level import operation by the
module loader, both for the host-called imports (ie for the main entry) and for
dynamic import() imports. These are distinguished by the dynamic context.
All preImport hooks for all loaders are run asynchronously in parallel,
and block any further load operations (ie resolve and load) for the module graph
being imported until they all complete successfully.
Multiple import calls to the same import specifier will re-call the hook
multiple times. The first error thrown by the preImport hooks will be directly
returned to the specific import operation as the load failure.

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}`);
}
@JakobJingleheimer JakobJingleheimer added the loaders-agenda Issues and PRs to discuss during the meetings of the Loaders team label Jun 19, 2022
@JakobJingleheimer
Copy link
Contributor

JakobJingleheimer commented Jun 19, 2022

This is an interesting idea! I have a couple questions:

  • All preImport hooks for all loaders are run asynchronously in parallel,
    and block any further load operations (ie resolve and load) for the module graph
    being imported until they all complete successfully.

    Is this effectively a Promise.all() or Promise.race()? Also, why? I'm a little confused by why they should run in parallel.

  • In your example, you have if (context.topLevel), but topLevel is not in context currently or in your proposed additions. What does it mean? I presume top-level await, but that's what dynamic intends to identify, no?

I'm a little concerned by the pre prefixes: I already find globalPreload confusing (it's effectively a “setup” step). I think if we introduce this new hook, we should re-visit the globalPreload name (especially because it does not behave like the other hooks).

P.S. Sorry for the delayed reply; I'm only just seeing this now.

@guybedford
Copy link
Author

Is this effectively a Promise.all() or Promise.race()? Also, why? I'm a little confused by why they should run in parallel.

It's a Promise.all, the reason being that is the most performant design when we don't want to unnecessarily serialize to slow down imports. If two loaders both want to do fs / network / other async operations when a new import is initiated, there's no reason they need to block eachother.

In your example, you have if (context.topLevel), but topLevel is not in context currently or in your proposed additions. What does it mean? I presume top-level await, but that's what dynamic intends to identify, no?

Ahh thanks for spotting the issue in the example, that should be context.dynamic yes. I've gone back and fourth on the wording to get something clear. A top-level import is basically an import which will run the ECMA-262 top-level Evaluation function - https://tc39.es/ecma262/#sec-moduleevaluation. Not all modules go through a whole graph execution operation (eg most dependencies). topLevel is perhaps more descriptive than dynamic, but both likely need clarification. I have a better definition in the current readme update I'll add to the PR now.

I'm a little concerned by the pre prefixes: I already find globalPreload confusing (it's effectively a “setup” step). I think if we introduce this new hook, we should re-visit the globalPreload name (especially because it does not behave like the other hooks).

My preference would have been to just call it the import hook, but export function import() isn't supported in ECMA-262 unfortunately. I'm open to alternative names, eg beforeImport or importHook or prepareImport etc.

Yes, both globalPreload and preImport are effectively non-chainable hooks which can run without blocking other hooks. They fall under a different parallel hook model. In the loader documentation it could be worth eg explicitly documenting the "hook type" of each hook something like Hook Type: Sync Chained or Hook Type: Async Parallel, and there may also be other variants in future.

@GeoffreyBooth GeoffreyBooth removed the loaders-agenda Issues and PRs to discuss during the meetings of the Loaders team label Jul 1, 2022
@GeoffreyBooth
Copy link
Member

Closing in favor of #89

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants