Review requested:

• @nodejs/loaders
• @nodejs/modules

This would need to be rebased now that the other PR has landed. Let me know if you need help with the process.

How would this work with worker_thread? Can I set a loader specific to a given thread?

How would this work with worker_thread? Can I set a loader specific to a given thread?

I think the way this works is it would add loaders to the main loaders thread that applies to the main application thread and all worker threads. (And it would create that main loaders thread if it doesn’t already exist at startup time.) Any modules loaded prior to this registration would be unaffected, of course, so people would need to be careful if they want this to apply to their main app, like:

import { registerLoader } from 'module'

await registerLoader('some-loader')
await import('app-entry.js')

So long as there’s just one loaders thread that applies to both the main thread and worker threads, it’s all or nothing: registerLoader adds loaders to apply everywhere.

That's perfect for me.

Not something that needs to be done in this PR, but I think we should consider adding a few supporting APIs:

• Ability to unregister a hook.
• Ability to inspect the currently registered hooks.
• An addition to the permission system to prevent loaders from being registered/unregistered programmatically.

• An addition to the permission system to prevent loaders from being registered/unregistered programmatically.

Arguably this should happen at this PR, so there isn’t a gap between being able to register loaders programmatically and preventing people from doing so. Or two PRs but this one is blocked from being released until the permission is added.

The risk is relatively low though, because registerLoader only applies to modules imported after the registration call; which would be only dynamic import() and any resulting new downstream imports (static or dynamic).

• Ability to unregister a hook.
• Ability to inspect the currently registered hooks.

I assume you mean loader/loaders here, not hooks? This API registers an entire loader, so presumably the complementary API would be deregisterLoader. Registering or deregistering individual hooks is an interesting idea, but I’m not sure what the use cases would be. A loader can always be broken apart into its component hooks, like you could create a new loader that consists of export { load } from './other-loader.js' to have a loader that’s just some other loader’s load hook, so I’m not sure we need our own API to provide an easier way.

I assume you mean loader/loaders here, not hooks?

I did mean loaders instead of hooks here. I apologize for that.

Okee, sorry for the dump, but I think this is too big to hide in a file comment:

I think the tech design here needs to change quite a bit to actually work:

esm/worker autonomously handles loader registration for loaders specified via CLI. it currently does not expose this; it would need to do. So what I think needs to happen is:

1. node:module needs to expose a util (eg registerLoader)
2. module.registerLoader() needs to point to the "main" module loader, which I believe is accessible via const esmLoader = require('internal/process/esm_loader')) when at least 1 custom loader is specified via CLI, node will instantiate CustomizedModuleLoader [1] (which is what you need here).
3. CustomizedModuleLoader needs a specific way to communicate the registration to HooksProxy (so a register method probably needs to be added to CustomizedModuleLoader).
4. HooksProxy needs a specific message type (eg simply register) to communicate the registration to the esm worker)
5. esm/worker needs to handle that message and call Hooks::addCustomLoader (which already exists) [2].

Perhaps module.registerLoader() should be "synchronous" like import.meta.resolve() because forgetting to await is quite a foot-gun that may be very difficult for a user to troubleshoot, and I believe there is no scenario where it would be desirable for the programme to continue before the registration has finished. If so, this is already possible, and CustomizedModuleLoader::register() would just need to call HooksProxy::makeSyncRequest() (instead of HooksProxy::makeAsyncRequest()); see CustomizedModuleLoader::resolve() and CustomizedModuleLoader::load() respectively).

Footnotes:

1. A limitation to this design is that at least 1 custom loader must be supplied at startup (be it environment var / CLI, or, if supported in future, via something like package.json). When that condition is not met, I would make module.registerLoader somehow indicate (probably throw) to the user that it can't do what it's supposed to do. I believe there is no way to get around this limitation as node needs some indication at startup to know whether to instantiate DefaultModuleLoader vs the much more expensive CustomizedModuleLoader (which was a hard requirement).
2. initializeHooks in lib/internal/modules/esm/utils.js does some stuff with adding custom loaders; that may be helpful (or you may need to tweak it).

Regarding making the API sync, I’ve been thinking about that and I’m having conflicting thoughts about it: while I 💯 agree that a sync API would produce fewer surprises, but on the same time we probably want the API to work from --require scripts, and there a sync API might cause problems because the ESM loader is not loaded yet.
Maybe it would be easier to start with converting a smaller use case, and I think that PR is doing a good job at that (only covering the use case "I want to register additional loaders in the runtime when I already have added at least one from the CLI"). I think it would work to first land this PR and iterate on it on follow up PRs. But we can already discuss the other use cases of course!

while I 💯 agree that a sync API would produce fewer surprises, but on the same time we probably want the API to work from --require scripts, and there a sync API might cause problems because the ESM loader is not loaded yet.

I always assumed it would be an async API; it’s essentially import()-ing a package from disk. I don’t think it would be too much of a footgun; if you do registerLoader('typescript'); import('./app.ts') the latter would immediately error. I feel like that would be the case for most loaders that users would register. And making it async from the start preserves the most flexibility.

1. I believe there is no way to get around this limitation as node needs some indication at startup to know whether to instantiate DefaultModuleLoader vs the much more expensive CustomizedModuleLoader

If the user needs to pass --loader regardless of whether or not they’re going to use registerLoader, then there’s not much point to creating registerLoader. Maybe what we need is a way for DefaultModuleLoader to “convert” into CustomizedModuleLoader on demand when it’s told to register a loader.

it’s essentially import()-ing a package from disk

It loads it in a different thread, comparing it with import is not quite right IMO. Also it doesn't have to be from disk, it can be from the network or another loader.

registerLoader('typescript'); import('./app.ts') the latter would immediately error

If it's async, it would race, so sometimes error, sometimes succeeds. But that sounds like a terrible API 😅

And making it async from the start preserves the most flexibility.

Do you have a use case in mind that would require the loader registration to be async? 🤔

Maybe what we need is a way for DefaultModuleLoader to “convert” into CustomizedModuleLoader on demand when it’s told to register a loader.

Maybe we should support it only in scripts loaded from --require on the main thread, before the ESM loader is initiated? We have to reduce its scope for the initial implementation somehow, it's not reasonable to try to address all at once IMO.

it’s essentially import()-ing a package from disk

It loads it in a different thread, comparing it with import is not quite right IMO. Also it doesn't have to be from disk, it can be from the network or another loader.

Sure, but loading modules (whether from disk or network etc) is typically an async operation.

registerLoader('typescript'); import('./app.ts') the latter would immediately error

If it’s async, it would race, so sometimes error, sometimes succeeds. But that sounds like a terrible API 😅

Okay, fair enough, but this is no different than any developer forgetting to await an async API. If the code somehow doesn’t error for them, hopefully their linter or typechecker warns them that they forgot an await. I don't think we should constrain our API because developers might forget to await a promise.

And making it async from the start preserves the most flexibility.

Do you have a use case in mind that would require the loader registration to be async? 🤔

Because many loaders will presumably be written as ESM, and loading them means import()-ing them, which is async.

I don’t necessarily want this API to be async; if it can be sync, great, do it that way. I just assumed that somewhere under the hood it would use the import() machinery and therefore would need to be async.

Maybe what we need is a way for DefaultModuleLoader to “convert” into CustomizedModuleLoader on demand when it’s told to register a loader.

Maybe we should support it only in scripts loaded from --require on the main thread, before the ESM loader is initiated? We have to reduce its scope for the initial implementation somehow, it’s not reasonable to try to address all at once IMO.

Needing this to happen from CommonJS and/or from --require really reduces a lot of the usefulness, to the point where we’ve defeated the purpose. The goal of creating this API is to not require flags, whether --require or --import or --loader.

I think we just need to undo the separation of DefaultModuleLoader and CustomizedModuleLoader, where we merge them back together into one ModuleLoader that by default initializes like DefaultModuleLoader does today, and loaders get registered via a method rather than the constructor. And then only when a loader is registered does it create the loaders thread and so on. It would be a lot like the old ESMLoader used to be before the refactor (but let’s keep the name ModuleLoader so that eventually it can supplant the CommonJS loader). This is probably too much to expect a newcomer to the codebase to tackle, so we’ll need to do a lot of this work.

Because many loaders will presumably be written as ESM, and loading them means import()-ing them, which is async.

But the "import" happens in a different thread, so there’s no reason we couldn’t make it sync, like we do for import.meta.resolve.

This does not land cleanly in v18.x-staging and will need manual backport in case you want it to land in v18.