re-implement module.register() on top of module.registerHooks()

[joyeecheung]

This was an idea discussed in the loaders group a few times in the past as well as in the last collaboration summit. Opening an issue to track it properly.

The high-level idea is something like this:

module.register = function(specifier, parentURL, options) {
  loader.hooksProxy.makeSyncRequest('register', specifier, parentURL, options);
  module.registerHooks({
    resolve(specifier, context, nextResolve) {
      const result = loader.hooksProxy.makeSyncRequest('resolve', specifier, context);
      return result.shortCircuit ? result : nextResolve(specifier, context);
    }
    load(url, context, nextLoad) {
      const result = loader.hooksProxy.makeSyncRequest('load', url, context);
      return result.shortCircuit ? result : nextLoad(url, context);
    }
  });
}

Then we will be able to:

1. Eliminate the races in ESM cache handling when require(esm) needs the linking to be synchronous but import esm may populate an asynchronous linking job in the cache. There were multiple band-aids applied to work around this, like esm: link modules synchronously when no async loader hooks are used #59519 and module: handle cached linked async jobs in require(esm) #57187, but to fully eliminate the races, this would be necessary to finish Tracking Issue: Syncify the ESM Loader #55782
2. Go back to using the authentic require() in imported CJS instead of re-inventing a separate require() and eliminate its quirks, which currently includes
   • Invoking the hooks twice for CJS, the additional one from within the re-invented require

     node/lib/internal/modules/esm/translators.js

     Line 151 in af5d1c9

     const job = cascadedLoader.getModuleJobForRequireInImportedCJS(specifier, url, importAttributes);

   • The re-invented require lacks a lot of properties like require.cache, which is a documented caveat https://nodejs.org/api/module.html#caveat-in-the-asynchronous-load-hook
   • The source code for CJS ended up being loaded twice

     node/lib/internal/modules/esm/load.js

     Lines 97 to 99 in 0fd1ecd

     	// For backward compatibility reasons, we need to discard the source in
     	// order for the CJS loader to re-fetch it.
     	source = null;

   • As a side-effect of the previous quirk, when the module format is CJS, the user hook has to also deal with source being null and usually also has to load it twice or more, like what's being done in import-in-the-middle, see ERR_INVALID_RETURN_PROPERTY_VALUE when using module.register and module.registerHooks #57327 (comment)
3. Potentially this means module.register can be a user-land helper that we can publish as a separate npm package and version independently.

Refs: #57327
Refs: #55808
Refs: #52697
Refs: #56241
Refs: #55782

cc @nodejs/loaders

[GeoffreyBooth]

Then we will be able to:

Yes please get rid of all that awfulness.

How do you intend to achieve this, though? Use everysync to add the off-thread aspects, to mimic the current behavior?

[joyeecheung]

How do you intend to achieve this, though? Use everysync to add the off-thread aspects, to mimic the current behavior?

Basically just the pseudocode in the OP.

I am not sure how the module hooks should behave on the loader thread though - the module loading is currently already extra quirky on the loader thread, for example require() is not customized on the loader thread at all because, well, you can't block on yourself. I wonder if there are that many people actually relying on the loaders affecting the loading of other loaders or do most existing users just don't care. IMO it would be less of a footgun to just let the off-thread loaders not affect the loading of each other and only affect the loading on the main thread. But if we really want to maintain compatibility with the existing behavior, we can leave the module loader on the loader thread as-is..with all the existing quirks, and only fix the main thread loading behavior.

[GeoffreyBooth]

But if we really want to maintain compatibility with the existing behavior

I think this is the crux of the issue. If you want to keep module.register at all, then presumably it should be preserved without breaking changes until it's ready to be removed when registerHooks is ready; but if you're going to refactor it so significantly that the changes will break most module.register users, then why bother? Maybe it's better to just remove module.register completely now, even if registerHooks is still experimental, rather than breaking register a few more times before its eventual removal?

I fully support whatever you want to do, and if the work you describe here somehow helps us get to a stable registerHooks faster, then by all means please proceed. Maybe the changes you're describing here are technically breaking but in practical terms they won't break too many users, and so it's worth doing this small break rather than the larger break of just deleting register. I trust your judgment.

[joyeecheung]

From what I can tell, it doesn't have to be breaking - just that if we want to maintain the current loader thread behavior, the quirks mentioned in the OP would still exist in the module loading of the loader thread (i.e. if an async loader depends on require(esm) in its own dependency, this is still subject to races; also all CJS dependencies of async loaders still won't be affected by other async loaders). We'll just only fix module loading for the main thread, and if any loader author want to avoid those quirks in their own graph, the best they can do is to switch to module.registerHooks.

[joyeecheung]

I tried to refactor the loader locally and I have a clearer picture of how feasible this is: I think we can achieve 1 and 2 for the non-loader thread. For module loading within the loader thread, 1 will still remain (but that might be a bit niche - to make that happen a async loader hook would need to hit require(esm) on the loader thread). 2 just never worked on the loader thread anyway (i.e. hooks can't customize require() in the module loading on the loader thread, all they can do is to customize require() on the non-loader thread) and we can get rid of that on the non-loader thread. I think 3 would be difficult to do - the loader thread needs the ability to "asynchronously customize the module loading of the current thread from the current thread" (i.e. the old loader behavior) back. Internally, we can kind of make it work by "if this is running on the loader thread, allow these paths to return a promise and do things asynchrnously and when it comes from a use case where sychronicity is needed e.g. require() and import.meta.resolve(), just ignore the async hooks", which is also the current behavior. But hoisting this part out to user land might reveal too many internal details that it's better to keep it internal.