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 withworker_thread? Can I set a loader specific to a given thread?

How would this work withworker_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'awaitregisterLoader('some-loader')awaitimport('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, becauseregisterLoader only applies to modules importedafter the registration call; which would be only dynamicimport() 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 bederegisterLoader. 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 ofexport { load } from './other-loader.js' to have a loader that’sjust some other loader’sload 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 (egregisterLoader)
2. module.registerLoader() needs to point to the "main" module loader, which I believe is accessible viaconst esmLoader = require('internal/process/esm_loader')) when at least 1 custom loader is specified via CLI, node will instantiateCustomizedModuleLoader^[1] (which is what you need here).
3. CustomizedModuleLoader needs a specific way to communicate the registration toHooksProxy (so aregister method probably needs to be added toCustomizedModuleLoader).
4. HooksProxy needs a specific message type (eg simplyregister) to communicate the registration to the esm worker)
5. esm/worker needs to handle that message and callHooks::addCustomLoader (which already exists)^[2].

Perhapsmodule.registerLoader() should be "synchronous" likeimport.meta.resolve() because forgetting toawait 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, andCustomizedModuleLoader::register() would just need to callHooksProxy::makeSyncRequest() (instead ofHooksProxy::makeAsyncRequest()); seeCustomizedModuleLoader::resolve() andCustomizedModuleLoader::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 likepackage.json). When that condition is not met, I would makemodule.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 instantiateDefaultModuleLoader vs the much more expensiveCustomizedModuleLoader (which was a hard requirement).
2. initializeHooks inlib/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 essentiallyimport()-ing a package from disk. I don’t think it would be too much of a footgun; if you doregisterLoader('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 instantiateDefaultModuleLoader vs the much more expensiveCustomizedModuleLoader

If the user needs to pass--loader regardless of whether or not they’re going to useregisterLoader, then there’s not much point to creatingregisterLoader. Maybe what we need is a way forDefaultModuleLoader to “convert” intoCustomizedModuleLoader on demand when it’s told to register a loader.

it’s essentiallyimport()-ing a package from disk

It loads it in a different thread, comparing it withimport 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 forDefaultModuleLoader to “convert” intoCustomizedModuleLoader 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 essentiallyimport()-ing a package from disk

It loads it in a different thread, comparing it withimport 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 anawait. 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 meansimport()-ing them, which is async.

I don’t necessarilywant 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 theimport() machinery and therefore would need to be async.

Maybe what we need is a way forDefaultModuleLoader to “convert” intoCustomizedModuleLoader 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 tonot require flags, whether--require or--import or--loader.

I think we just need to undo the separation ofDefaultModuleLoader andCustomizedModuleLoader, where we merge them back together into oneModuleLoader that by default initializes likeDefaultModuleLoader 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 oldESMLoader used to be before the refactor (but let’s keep the nameModuleLoader 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 meansimport()-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 forimport.meta.resolve.

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