doc: add synchronous hooks proposal

[joyeecheung]

trafficstars

cc @nodejs/loaders

Spinned from https://github.com/nodejs/node/issues/52219 - I ended up writing it down as a proposal here, because there are some high-level questions and re-design ideas after I looked into existing usages of CJS monkey-patching. While resolve and load hooks work well enough for some cases, it also seems common in the wild to just override specifier -> module mapping directly, so I sketched out some higher level hooks as alternatives here as well.

[joyeecheung]

After some thoughts I think the higher level requires()/link() hooks that span across resolve/load make more sense than lower level exports()/link() hooks that run after load because they are more powerful. The lower level hooks can only run things after load. If they need to get information (e.g. specifier) from previous steps the hook author needs to also override the earlier hooks (resolve/load) to save them somewhere. But with the higher level hooks they can choose to either run things before resolve() or after load() or they can do both in one closure which is a lot more convenient. Or it's possible to re-implement the lower-level exports()/link() using the higher-level hook, but not the other way around. Take require for example, it's possible to re-implement the after-load exports() with requires() like this:

function exports(url, context) { ... }

function requires(specifier, context, nextRequires) {
  // requires hook can choose to run something here, exports can't.
  // If hooks want to simply hijack a request and return early, skipping
  // resolve and load, they can do it here. They also don't need to care
  // about how different specifiers can be mapped to the same URL.
  if (specifier === 'pnpapi') {
    return { url: 'some://dummy/url', exports: pnpApiObject };
  }
  const result = nextRequires(specifier, context);  // Invokes resolve and load steps
  exports(result.url, result);
}

Or to re-implement the after-load link hook using the high-level link hook for import requests:

function linkAfterLoad(url, context) { ... }

const pnpApiModule = new vm.SyntheticModule([...], () => {
   pnpApiModule.setExports('default', pnpApiObject);
});

function link(specifier, context, nextLink) {
  // link hook can choose to run something here, linkAfterLoad can't.
  // If hooks want to simply hijack a request and return early, skipping
  // resolve and load, they can do it here. They also don't need to care
  // about how different specifiers can be mapped to the same URL.
  if (specifier === 'pnpapi') {
    return { url: 'some://dummy/url', module: pnpApiModule };
  }
  const result = nextLink(specifier, context);  // Invokes resolve and load steps
  linkAfterLoad(result.url, result);
}

In addition, if these hooks need to invoke say require.resolve() in an algorithm to determine whether they need to hijack a request, since they don't need to override resolve hooks to retain any information themselves, they won't run into weird infinite recursions.

[jsumners-nr]

After some thoughts I think the higher level...

I like this assessment.

[GeoffreyBooth]

After some thoughts I think the higher level requires()/link() hooks that span across resolve/load make more sense than lower level exports()/link() hooks

Let’s say I want to write CoffeeScript and have it instrumented. The CoffeeScript hooks might implement just load, or resolve and load, per https://nodejs.org/api/module.html#transpilation. Then if I also want to register my instrumentation library, and it uses requires/link, do the CoffeeScript hooks never get called because they’ve been bypassed? Or are you suggesting that we don’t have resolve/load anymore, just requires and link?

As an author, personally what I find easiest to follow is hooks that correspond to steps in the process. So if there’s a step before resolution, and a step after loading, then those can be additional hooks that still run in the sequence: something before resolution, resolve, load, something after loading.

[joyeecheung]

Then if I also want to register my instrumentation library, and it uses requires/link, do the CoffeeScript hooks never get called because they’ve been bypassed? Or are you suggesting that we don’t have resolve/load anymore, just requires and link?

I am suggesting to have resolve and load and requires/link that wrap around the previous two steps. Hooks that only care about resolution implement resolve, hooks that only care about loading, (this is a bad name for source but meh it's already there) , like transpilers, implement load. These two apply to both CJS and ESM. Hooks that care about the CJS require results implement requires, hooks that care about ESM module import results implement link, hooks that care about both implement both (we can't unify this part in one hook due to design of ESM being essentially different from CJS - everything is immutable after linking, which happens before evaluation, so it's impossible to have a post-evaluation step that modifies the linking results surfaced to import).

[joyeecheung]

After attempting to prototype the high-level wrappers in the ESM loader I realized that there are some nuances in the current design - the nextLoad and nextLink (or similar) in the ESM loader currently isn't easily synchronous. The only case where they have to be asynchronous is when there are http/https imports in the graph. Others are more superficial. So I think we have some options:

1. Don't go with the nextHook design but instead split the hooks into something like beforeLoad/afterLoad, so users don't have to invoke the next hook. If they wish to skip the rest of the hooks, they can just return shortCircuit: true.
2. Keep the nextHook design but throw an error or ignore it for http/https imports.
3. Do a combination of both - we provide both beforeLoad/afterLoad that work with http/https imports, as well as load(..., nextLoad) which throws when http/https imports are encountered. Hooks that need to support http/https can implement the former, hooks that don't implement the latter. This also means we don't need to split resolve hooks which are still synchronous in all cases.

[arcanis]

1. Don't go with the nextHook design but instead split the hooks into something like beforeLoad/afterLoad, so users don't have to invoke the next hook. If they wish to skip the rest of the hooks, they can just return shortCircuit: true.

How does that work for loaders that are prerequisite for subsequent loaders to work? For example let's say a TS loader needing to resolve & load files from within zip archives?

[jsumners-nr]

1. Don't go with the nextHook design but instead split the hooks into something like beforeLoad/afterLoad, so users don't have to invoke the next hook. If they wish to skip the rest of the hooks, they can just return shortCircuit: true.

I like this idea. I find the nextHook pattern difficult to reason through.

[mcollina]

The only case where they have to be asynchronous is when there are http/https imports in the graph.

The alternative is to provide the DeasyncWorker we talked about, and use it to implement a synchronous http client we use for this. In the end that's what we will be telling our users to do.

[joyeecheung]

How does that work for loaders that are prerequisite for subsequent loaders to work? For example let's say a TS loader needing to resolve & load files from within zip archives?

Can you describe how the module request would look like in this case? Would that just override the resolution/loading completely or still use part of the default logic? For example assuming that the request is require('foo') or import 'foo' and you want to make it load from a foo.ts in /path/to/package.zip, which gets transpiled to commonjs (so everything gets overridden completely):

function beforeResolve(specifier, context) {
  if (specifier === 'foo') {
    const url = 'file:///path/to/package.zip?file=foo.ts';  // Custom resolution
    return { format: 'zip+typescript', url, shortCircuit: true };
  }
  return { };  // do nothing, and the default resolution logic gets used.
}

function beforeLoad(url, context) {
  if (context.format === 'zip+typescript') {
    const source = unzipAndTranspile(url);  // Unzip path/to/package.zip,  and transpile foo.ts
    return { source, format: 'commonjs', shortCircuit: true };
  }
  return { };  // do nothing, and the default loading logic gets called.
}

(Not sure if the search params can be preserved in CJS loader yet, given the compat issue of Module._cache keys already described in the doc - if not, the best we can do is to pass some data via context on first load of the module that gets mapped to a specific file path - CJS loader only supports file URLs, after all, or hooks can customize the URL as something like file:///path/to/package.zip/foo.ts and they know how to extract a foo.ts out of /path/to/package.zip/ using this pattern).

Or if you are thinking about composing a zip loader + a typescript loader:

// Zip loader
function beforeResolve(specifier, context) {
  if (specifier === 'foo') {
    const url = 'file:///path/to/package.zip/foo.ts';  // Custom resolution
    return { format: 'zip', url, shortCircuit: true };
  }
  return { };  // do nothing, and the default resolution logic gets used.
}

function beforeLoad(url, context) {
  if (context.format === 'zip') {
    const source = unzip(url);  // Unzip path/to/package.zip, and locate foo.ts
    return { source, shortCircuit: true };
  }
  return { };  // do nothing, and the default loading logic gets called.
}

// TypeScript loader
function afterLoad(url, source, context) {
  if (url.endsWith('.ts')) {
    return { source: transpile(source), format: 'commonjs' };
  }
  return { };  // do nothing, and the default loading logic gets called.
}

(I think we might need to separate shortCircuit options into two: one that skips other hooks of the same kind and the default logic, one in before hooks that also skip after hooks).

[joyeecheung]

The alternative is to provide the DeasyncWorker we talked about, and use it to implement a synchronous http client we use for this. In the end that's what we will be telling our users to do.

Yeah one alternative is to provide load(url, context, nextLoad) where the default nextLoad only works with non-network loading; Some user-land hook can be developed to support synchronous network loading which can be something like:

function load(url, context, nextLoad) {
   if (url.startsWith('http')) {
      const source = fetchSync(url);
      return { source, shortCircuit: true };
   }
   return nextLoad(url, context);
}

..which needs to be ensured to be at the bottom of the hook chain (next to the default one).

One downside of this is the fetching would be sequential and blocking even for ESM which is designed to make parallel loading of module requests in the same module possible. Theoretically with a slightly different design I think it's still possible to make the fetching part almost parallel:

function load(url, context, nextLoad) {
   if (url.startsWith('http')) {
      const syncResponse = fetchSyncPooled(url);  // This adds a fetch request to a pool
      return {
          // This blocks until the result is fetched; by default if this is the only load hook, the ESM loader
          // runs the hooks to get the source provider functions first (then the requests are all started)
          // and then run over the source provider functions to get the source (and start blocking on the
          // first module request).
          getSource() { return syncResponse.readAsBuffer(); } ,
          shortCircuit: true
      };
   }
   return nextLoad(url, context);
}

This would require significant refactoring in the ESM loader to make the creation of ModuleWraps lazy but it allows the loader to start the fetch requests sequentially fairly quickly in a pool (without waiting for the previous module request to actually complete), and then the worker pool can handle the requests in parallel. Any getSource() invocation from the main thread loader would block until the response is fetched, but at the mean time other threads in the worker pool can fetch other URLs just fine. Although with this the throughput is still not fully on par with fully asynchronous loading (uncle/aunt modules need to block on the loading of nephew modules) but at least this can offer parallelism on a single-module level.

On the other hand I think this might be a more compelling reason to split the hooks - with the nextLoad design, if the other hook invokes nextLoad to get the source, it can still end up too eager and few requests get parallelized; But if the hooks are split, the hooks that need the source would override afterLoad instead while the http loader can override beforeLoad, and the http loader can still parallelize as much as possible. Or it can do something like this to increase throughput:

// This needs to be the last of the beforeLoad chain:
function beforeLoad(url, context) {
   if (url.startsWith('http')) {
      const syncResponse = fetchSyncPooled(url);  // This adds a fetch request to a pool
      responseMap.set(url, syncResponse);
      return {
          source: 'dummy',
          shortCircuit: true
      };
   }
   return {};
}

// Node.js can run the beforeLoad eagerly for the module requests, and
// only start running afterLoad when the all beforeLoads of parallel module
// requests are run

// This needs to be the first of the afterLoad chain:
function afterLoad(url, source, context, nextLoad) {
   if (url.startsWith('http')) {
      // source was dummy at this point.
      const syncResponse = responseMap.get(url);
      return {
          source: syncResponse.readAsBuffer(),
      };
   }
   return {};
}

[Qard]

Would it make sense to have a think like the "link" hook that works for both module formats? I think we would generally prefer a single interface to work with, even if it's a bit sub-optimal for certain cases. We want to be able to apply CJS-based patches to ESM rewrites when they happen without needing to rewrite the patches. That's not a huge deal though.

[joyeecheung]

Would it make sense to have a think like the "link" hook that works for both module formats?

I don't think so, because this is where one of the key difference of CJS and ESM lies: ESM linking (dependency resolution) is designed to be done before evaluation, for all module requests in the module, which relies on static analysis based on the import syntax, and it's also what allows parallel loading of ESM dependencies; CJS dependency resolution is done during evaluation (in the form of require() invocations), and therefore forces everything to be done sequentially. It's not impossible do some static analysis in CJS to detect dependencies, but it's bound to be hacky and can lead to corner cases (e.g. if the module renames require() or do a bunch of runtime magic with it, then the static analysis is likely to fail, not to mention that require() can take a string computed at run time instead of a static string, so any require(foo + bar) is not analyzable), at that point it would've been better to just do multiple hooks to ensure correctness.

[Qard]

I'm aware of the difference in order/timing. I was thinking more if some interface could exist which abstracts that away. Like if you allow the CJS side to see what the export names are without directly accessing them you could do a similar "replace when available" kind of semantic as we have to do with ESM. I feel like a similar shaped API should be possible between the two, it just might not be quite as friendly. 🤔

Anyway, two APIs are probably liveable, we'll just have to make sure our instrumentations can handle both scenarios with (hopefully) minimal breakage.

[joyeecheung]

Like if you allow the CJS side to see what the export names are without directly accessing them you could do a similar "replace when available" kind of semantic as we have to do with ESM. I feel like a similar shaped API should be possible between the two, it just might not be quite as friendly. 🤔

That sounds cool but I think it would be more flexible if the built-in loader API provides the low-level hooks, and if a unified hook is desired, some user-land package can be developed on top of them (like a package that just unify import-in-the-middle and require-in-the-middle). Otherwise by only providing high-level unified hooks in core we will deprive the ability to do low-level customization completely from developers who need them, and only cater to the needs of developers who don't care about the differences.

[Qard]

Fair point. I'd be happy to help with constructing whatever that higher-level thing is later, but probably would want that to live in the Node.js org just to have an unbiased home for it that all the APM vendors can contribute to it. (Similar to what we're trying to do by donating IITM at the moment.)

[bmeck]

You should feel free to ignore http/s imports most likely. Having a sync shared worker behind the hook seems fine and to my knowledge no adoption of network imports gathered steam

[guybedford]

One of the major benefits of module.register is that it works for all threads. I'm a little concerned that this proposal would not as easily share the sync hooks with newly created worker threads unless it can be fitted into the existing module.register model. I'd be very weary of supporting a hook model that is not default-shareable to threads.

Then in terms of unification and since breaking changes are still on the table, can we not still just turn the existing ESM hooks into sync-only hooks? I'd support this as well provided we could implement a preImport hook or similar that runs as the sole async step for all top-level import operations. It should be sufficient to implement any speculative network fetching so long as one can invoke the other hooks within that hook via the context.

[arcanis]

I'd be very weary of supporting a hook model that is not default-shareable to threads.

Imo that's more of a feature than a drawback. Worker isolation is a model easy to reason about. Having a main controller supposed to handle the resolution of everything in all threads is significantly more complex.

Also note that even with the current patching model, one just needs to set NODE_OPTIONS="--require my-loader" for my-loader to be installed in both the main thread and any child workers. I presume a similar strategy would be possible if someone wanted to have an all-encompassing loader (which Yarn PnP needs, so I'm not saying the use case isn't valid - but I'm not sure it's worth making it central to the design).

[Qard]

Yeah, I would prefer use of cli flags to load into all workers, or a more explicit method of mounting hooks into workers in parent thread code. Having APIs just magically impact all threads without any explicit control over that seems problematic and possibly creates security issues when people don't realize that is happening.

[ggoodman]

It seems quite plausible that there will be use-cases where hook authors want to propagate hooks to workers and where codebases that leverage workers will want to opt-out. While there is some tension between the two, it seems like an API could support both without compromising too much on the DX.

[joyeecheung]

Updated the doc a bit before I lose track of all the comments (thanks for the feedback!)

1. Certain fields in the hooks become mandatory, and shortCircuit: true is required if the next hook isn't invoked, like the module.register() hooks
2. Default load down the chain will just throw an error if http imports are encountered
3. exports span across CJS module evaluation (perhaps it needs a better name) so that it can be used to deal with the contextual exports modification. It also makes the hook more powerful this way.
4. link is still after load because there is a use case for it to override source maps (not sure if it's the best timing yet, but it's better than load). For CJS that'll be exports (since both are post-parsing).
5. @nicolo-ribaudo helped me find a solution to address the ESM concurrent loading problem although this would require BFS invocation order for ESM hooks to allow concurrency.
6. I also added a complementary proposal for preloading a script in child worker/processes to deal with the hook inheritance problem. I think that's more flexible than making it part of the module API and I am pretty sure it has use cases beyond module hooks.

[JakobJingleheimer]

I did some preliminary testing to eyeball what performance implications this may have, and it seems to have a non-trivial degradation to performance:

https://github.com/JakobJingleheimer/nodejs-loaders/pull/16

Hook execution time (on node v22.4.1) increases ~14%.

This is potentially offset by eliminating startup cost from the loader worker thread, but something to consciously consider.

The cause behind the increase may perhaps be specific to esbuild's implementation: they do have warnings in the docs that transformSync instead of transform may have a negative impact on performance, citing an inability to leverage parallelisation within esbuild. I suspect other transpilers will likely have sufficiently similar implementations to similarly suffer.

[joyeecheung]

This is potentially offset by eliminating startup cost from the loader worker thread, but something to consciously consider.

I suspect that's already where the cost is coming from, because spawning a worker is not cheap, and this test is spawning that worker for nothing. Also, with this model you are going to do parallelisation differently - you can spawn multiple workers, and do the transform actually in parallel using the BFS iteration order. In the off-thread model you are not getting actual parallelisation, because there is only one loader worker, and the computation there is single threaded. Even if you post transpilation for 3 files to that worker, the single loader worker would have to chew the bytes of them one by one. The right way to do parallelisation is to spawn 3 workers (more or less) and you post transpilation to them using a worker pool abstraction, so that you actually have 3 threads doing transpilation in parallel, not just one loader thread choking on all the computational work.

[joyeecheung]

For tsx in particular, I think you should compare their CJS hooks v.s. the ESM hooks, because the CJS hooks are built on top of monkey patching, and is what this proposal is effectively replacing (or that's the top priority of this proposal).

My local tests show that at least the synchronous CJS hooks are currently ~2.5x faster than the asynchronous ESM hooks to load a very basic file.

➜  test-tsx cp ../node/test/fixtures/snapshot/ts-example.ts ./file.ts
➜  test-tsx hyperfine "node --require tsx/cjs ./file.ts" "node --import tsx/esm ./file.ts" --warmup 3
Benchmark 1: node --require tsx/cjs ./file.ts
  Time (mean ± σ):      42.7 ms ±   0.2 ms    [User: 32.8 ms, System: 11.0 ms]
  Range (min … max):    42.2 ms …  43.3 ms    68 runs

Benchmark 2: node --import tsx/esm ./file.ts
  Time (mean ± σ):     106.3 ms ±   0.4 ms    [User: 93.2 ms, System: 25.0 ms]
  Range (min … max):   105.5 ms … 106.8 ms    27 runs

Summary
  'node --require tsx/cjs ./file.ts' ran
    2.49 ± 0.02 times faster than 'node --import tsx/esm ./file.ts'

[joyeecheung]

The latest prototype is in https://github.com/joyeecheung/node/tree/sync-hooks-6 - I've got it working for ESM too, still need a bunch of testing to be opened as PR tough.

[Mars7005]

Great to be hear