Review requested:

• @nodejs/loaders

I'm +1 in general, but I think this is a bit premature: we should first finish providing applicable CJS functionality in ESM.

I don't think we should do that. ESM has AFAIK a big performance overhead compared to CJS.

Mm. Currently ESM's startup begins after CJS's startup finishes. Removing that delay would dramatically speed up ESM's startup.

Plus things like Antoine's port of import.meta to C++.

I'm also +1 on this.
This will hopefully encourage people to migrate to ESM and eventually gather more bug/performance reports.

This kind of thing should be a trailing indicator, not a leading one. Wait until the ecosystem has actually migrated to ESM (the vast majority of downloaded npm packages remain CJS by a very large margin) before marking CJS as legacy.

I don't think we should do that. ESM has AFAIK a big performance overhead compared to CJS.

That's true and also observability and many other things, but it's like saying we should not use electric cars because have less range. There are pros and cons of both otherwise commonjs would have been wiped completely.
The standardization imho is what makes it worth. There are no short term effects of this change. Only long term benefits.
We are not deprecating it and saying dont use it. We say there is a newer alternative and you should use esm.

I’m a big proponent of trying to make ESM our default and promoted recommendation, and that’s what nodejs/TSC#1445 was about. But our docs define legacy as:

Stability: 3 - Legacy. Although this feature is unlikely to be removed and is still covered by semantic versioning guarantees, it is no longer actively maintained, and other alternatives are available.

And this is not an accurate description of CommonJS. It is very much actively maintained, and there are features in progress for it like the synchronous customization hooks that @joyeecheung is spearheading.

I’m a big proponent of trying to make ESM our default and promoted recommendation, and that’s what nodejs/TSC#1445 was about. But our docs define legacy as:

Stability: 3 - Legacy. Although this feature is unlikely to be removed and is still covered by semantic versioning guarantees, it is no longer actively maintained, and other alternatives are available.

And this is not an accurate description of CommonJS. It is very much actively maintained, and there are features in progress for it like the synchronous customization hooks that @joyeecheung is spearheading.

It should be changed to

it is no longer actively maintained or better alternatives are available.

Per your OP, "better" is currently inaccurate:

ESM is not a 1-1 replacement and there are still some use cases where commonjs performs better

What needs to happen specifically to make it happen?
The two module systems will always be different and have pros and cons
Saying that when esm will have the same cold startup time than cjs means probably never, because of the core difference.
Do we really keep telling our users, use commonjs?

What needs to happen specifically to make it happen?

Really just two things, that I can think of:

1. We need to finish the synchronous module customization hooks, and remove the off-thread ones. After some baking/bugfixing time, this should let us finally reach stability for the hooks, which would mean for the first time in the history of Node.js we have a stable, officially supported way to instrument Node applications (and do things like transpilation, beyond the built-in TypeScript support).

2. We need to syncify the ESM loader. This should bring its performance close to parity with CommonJS, and also allow the async_hooks tests to pass which would let the ESM loader start handling all entry points. This sets the stage for merging the code between the two loaders and removing duplication, and improving performance further by moving some of the code paths into C++.

I concur with @GeoffreyBooth on both accounts.

I do think this is eventually appropriate but I don't think we're quite there yet. Consider also that "Legacy" does not JUST mean that there is a standard alternative available but also heavily implies that the thing being marked as Legacy likely will not receive any further or significant changes and I'm not certain we can claim that yet for CJS, at least not for a while. I'm not going to -1 this but I would suggest that we table it and revisit after 25.0.0 is cut later this year.

If we all use the same thing it will be easier to optimize it.

From my experience touching the ESM loader, I don't think the optimization would be any easier unless we completely remove support for CommonJS and only need to support ESM. Only at that point would we be able to actually re-architect the module loaders to fully utilize what the design of ESM offers. So if the OP also envisions:

Commonjs will still be supported forever.

Then I don't think this would move the needle in terms of optimization work in any meaningful way, since we aren't going to get any optimization opportunities from a completely new design.

That said if what we aim for is just to bring it on par with CJS performance, not beyond, then I don't think removing CJS support is a must in the first place. And technically I don't really see anything stopping us from making the ESM loader faster than CJS - it doesn't even need to care about monkey patching or breaking internals used in the wild. IMO the ESM loader is just slower due to lack of investment into its optimization and the code being too "organic", we haven't yet got to the point where the bottleneck lies in backwards compatibility. (Case in point: require(esm) was about 22% faster than import esm and my guess is that it was because I did not reuse a big chunk of the old ESM loading code and wrote a different fast path for the ease of backportability)

the module detection tries esm first etc....

Again, as long as we are not actually removing support for CJS, I don't think this would move the needle in terms of module detection practices, because a CJS module is a valid ESM module, syntax-wise, but not vice-versa. We can afford trying to parse CJS and rollback if it contains ESM syntax - at that point no code is executed and we only swallow parse errors. But a CJS module can very well parse as ESM and CJS is not detectable just from the syntax (some code in another module can assign globalThis.require, for example, so the use of require() is not a valid indicator that a module is actually CJS). It's only possible to confirm after we execute the code, that means it's not really a side effect free syntax detection anymore and the code will get executed twice.

I am puzzled by the assertion that keeping CJS maintained is going to slow down the migration. At least for code that's still actively maintained, I think the primary source of reluctance is just that migration to ESM hasn't been a win for many people. Assuming that the end game does not actually involve breaking CJS, then the implementation of the two are not really at the expense of each other. I think the past 5 years has already proven that "let's make CJS more painful to use" is not a valid strategy for speeding up ESM adoption (according to https://github.com/wooorm/npm-esm-vs-cjs/ at least), and we should try "let's make ESM more pleasant to use". ESM has been stable since 2020, it wasn't the mainstream shipping format in npm but there was definitely not lack of adopters who have been giving us feedback, we had 5 years to optimize it and give it feature-parity with CJS, yet it is still lacking in various places as mentioned above. I think what we should be doing is to actually get that long overdue optimization and feature parity done first, and only after we make the migration a win for users should we start advertising it as being the better alternative. Otherwise all we have are just empty promises. I would prefer that we just give user more carrots for the migration, not chasing them with a stick - it might be fine to use the stick when the majority have already been lured over by the carrots, but we are not there yet.

Currently ESM's startup begins after CJS's startup finishes. Removing that delay would dramatically speed up ESM's startup.

CJS does not actually get initialized much at all during startup, because most of the initialization is done at build time, and serialized into the snapshot. And run time it mostly gets deserialized directly. ESM loader is slower because it's not in the snapshot, which means the initialization code actually has to be executed at run time, not deserialized from something already done at build time. And the reason why ESM loader is not in snapshot basically comes down to the code being too messy. I have tried cleaning it up and making it ready for snapshot and startup faster in #45828 FWIW, and what stalled it wasn't a technical problem.

What needs to happen specifically to make it happen?

Really just two things, that I can think of:

1. We need to finish the synchronous module customization hooks, and remove the off-thread ones. After some baking/bugfixing time, this should let us finally reach stability for the hooks, which would mean for the first time in the history of Node.js we have a stable, officially supported way to instrument Node applications (and do things like transpilation, beyond the built-in TypeScript support).
2. We need to syncify the ESM loader. This should bring its performance close to parity with CommonJS, and also allow the async_hooks tests to pass which would let the ESM loader start handling all entry points. This sets the stage for merging the code between the two loaders and removing duplication, and improving performance further by moving some of the code paths into C++.

I think these are actionable item and we should turn them into a roadmap.
Even if this doesnt happen now, I would like the outcome of this conversionation to be actionable rather than an obscure path

FWIW, I think

We need to #55782. This should bring its performance close to parity with CommonJS, and also allow the async_hooks tests to pass which would #50356. This sets the stage for merging the code between the two loaders and removing duplication, and improving performance further by moving some of the code paths into C++.

is not technically the correct assessment. Removing the async overhead from the loader may potentially help with performance and provide a unified experience in module customizations. But "let the (current) ESM loader handle all entry points" would go against that goal in if we keep the current design of the ESM loader, and cause breakages in cache handling as well as resolution. The idea of "ESM loader" is pretty artificial - what we have are "helper functions that can be used to resolve and load ESM" + "a module loader that is designed by assuming evaluation always follows full resolution of the entire graph, not just the subgraph it depends on, no matter what module it is". The former is less problematic, we can rewrite them to be more efficient (which is what require(esm) is already using and probably why it's faster than import esm, from what I can tell), but the latter is a lot more problematic and have been causing bugs like #57327 (comment) , and I suspect that's where the async hooks failures is coming from.

But "let the (current) ESM loader handle all entry points" would go against that goal in if we keep the current design of the ESM loader

What I mean by that is that we would could have one unified loader. When I wrote that issue a few years ago, the obvious choice seemed to be the ESM loader since it could load everything, including CommonJS; but lately the CommonJS loader has been catching up. The broader point is that once we have the sync hooks and can sunset monkey-patching, we have the freedom to refactor and/or combine the CommonJS and ESM loaders into something new, and in that process hopefully get some deduplication and performance improvements.

The broader point is that once we have the sync hooks and can sunset monkey-patching, we have the freedom to refactor and/or combine the CommonJS and ESM loaders into something new, and in that process hopefully get some deduplication and performance improvements.

I see, thanks for the clarification, though I feel that considering how widely monkey-patching of CJS loader has been used by popular packages, actually breaking it and taking advantage of being able to break it is even more ambitious than a doc-only change and should happen after we are comfortable with marking commonjs as legacy, not before. It's not very realistic to count on being able to break it in the near future to make significant performance improvements, but then on the other hand, as I described in #57460 (comment), I don't think it's a prerequisite or bottleneck for just "making it as fast as CJS" or even faster, either.

taking advantage of being able to break it is even more ambitious than a doc-only change and should happen after we are comfortable with marking commonjs as legacy, not before.

It's the syncification of the ESM loader that would be the prerequisite for marking CommonJS as legacy, to hopefully get parity or closer to parity on performance. All the other stuff about refactoring freedom would come later and was mentioned just as further benefits that would follow after finishing the sync hooks and syncification.

This pull request has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.