esm: 'module.exports' export on ESM CJS wrapper

[guybedford]

This provides a 'module.exports' export on the wrapper namespaces created for CommonJS modules when they are imported into ES modules. This mirrors the PR at #54563, which was split out from the original version of this PR which makes require(esm) take the value of this export in its interop pattern when that is used.

By adding this to the module namespaces for CJS in ESM, this effectively allows tools which transpile CommonJS into ESM to be able to consistently follow the same require(esm) interop for CJS externals, despite the importer being transpiled from CJS into ESM.

This is marked as a major change, since it changes the shape of the CJS wrapper.

//cc @nodejs/loaders

For details on the exact transpilation problem this solves, it is the following:

Consider a CommonJS module importing a dependency in Node.js that is then transpiled into ESM, where that dependency might be any unknown module format:

const depMod = require('dep');
console.log(depMod); // returns class Dep {} say

Now if I run this module in Node.js, and dep is an ESM module, dep will go through the require(esm) interop checks:

• If it has a module.exports export per module: support 'module.exports' interop export name in require(esm) #54563, this will be used
• Or, if it doesn't have an __esModule marker already and has a default export, it will be wrapped with an __esModule marker automatically added by Node.js

So if I want to transpile this module into ESM, with that same dynamic dependency linkage, I end up with something like this:

import * as depNs from 'dep';

// New interop layer to mimic Node.js require ESM semantics (in reality you'd use a proxy for live bindings)
const depMod = 'module.exports' in depNs ? depNs['module.exports'] : !('__esModule' in depNs) && ('default' in depNs) ? { ...depNs, __esModule: true } : depNs;

console.log(depMod); // class Dep {} still regardless of interop pattern

So we now have a scheme to transpile arbitrary CJS to ESM in Node.js, while retaining the exact same interop semantics.

But there is now a new interop problem with such a scheme - in the case where depMod is itself a CJS module, instead of an ESM module, say:

module.exports = class Dep {}

When importing this module from ESM, Node.js will interpret this as a module namespace of the shape depNs having Module { default: Dep }, which will then through the inlined interop code give the value of depMod as { __esModule: true, default: Dep } instead of class Dep, the expected module.exports value!

The fix, is to add this module.exports marker in the CJS ESM representation, then 'module.exports' in depMod is true and we we hit this first check in the inlined require CJS interop pattern of the transpiled code, and will get that back directly as we desired.

Therefore fully solving the dynamic externalization against all of the cases where dep might respectively be CJS transpiled to ESM and real ESM with the conditional faux ESM interop layer for the consuming faux ESM as CJS transpiled into ESM per #52166.

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[mcollina]

lgtm

[legendecas]

I may not follow with the motivation. If this feature is going to be used in "transpile a CJS module (which was transpiled from ESM) back into ESM and run it on the web", can this be added to transpilers instead?

[guybedford]

I may not follow with the motivation. If this feature is going to be used in "transpile a CJS module (which was transpiled from ESM) back into ESM and run it on the web", can this be added to transpilers instead?

It is also helpful to support CJS transpiled into ESM (ie modern bundled code) running in Node.js against an external that is unknown as to whether it is CJS or ESM, where being able to know if the external is a CJS wrapper can allow lowering the required require(esm) interop pattern.

[joyeecheung]

The reason why we introduced the __esModule was:

1. It has been a well-established technique used by most transpilers, and is known to work
2. There are already many popular packages on npm transpiled from ESM to CJS and therefore already needs the __esModule construct if they start to ship ESM, and many popular code base using existing transpilers that rely on __esModule.

I think to implement a counterpart, we at least need 1 - there should at least be some integration in CJS -> ESM transpilers adopting this pattern to run things in browsers, which is what this primarily is useful for - it's very rare to see CJS code transpiled into ESM to be run in Node.js, as far as I know. Suppose transpilers disagree and some adopt a different flavor, or if this trick doesn't turn out to be actually working because no integration test has been written yet (this is all on paper for now), we'd be left with something that doesn't work and nobody uses. #52166 included an integration test using output from TypeScript, I think we need something similar in this PR too.

[guybedford]

@joyeecheung I think the requirement that this new pattern already be in use before we have created the pattern is an unrealistic requirement to make.

I have given two clear use cases for this as a feature, with the direct use case in #53848 (comment) immediately being solved by landing this PR.

it's very rare to see CJS code transpiled into ESM to be run in Node.js, as far as I know

RollupJS is both a Node.js and browser bundler, so this is not quite true. When using RollupJS in Node.js applications to, say, speed up startup times, this is very much a real use case.

Suppose transpilers disagree and some adopt a different flavor, or if this trick doesn't turn out to be actually working because no integration test has been written yet (this is all on paper for now), we'd be left with something that doesn't work and nobody uses.

There's no trick or flavour here, simply the question - is the ES module namespace that I have imported an ESM wrapper around a CJS module? And I think that is a useful question to answer by supporting this flag to answer it. What more would you like to see tested with regards to this?

[guybedford]

Specifically the problem is needing to know when in Node.js if an arbitrary ESM namespace is an ESM CJS wrapper or not. With that answer, yes, new CJS to ESM interop patterns can emerge as I have discussed, since they very much need this information. But the check is useful in its own right, and does not depend on their future emergence either.

[joyeecheung]

I think the requirement that this new pattern already be in use before we have created the pattern is an unrealistic requirement to make.

Isn't this a pattern primarily meant for code run in the browser? Then it should be possible to create a bundler integration that demonstrate how this can be used in the browser first? __esModule is also a pattern that existed before we add it to Node.js, wasn't that in the same position?

When using RollupJS in Node.js applications to, say, speed up startup times, this is very much a real use case.

RollupJS supports CommonJS output too. As far as I know typically people bundle code into CommonJS format to speed up startup times (ESM loading is slower than CommonJS loading in the first place), or to use the bundle with SEA or snapshots since they do not support ESM. But choosing the ESM output for a Node.js environment is rare. If it's common enough it seems strange that this need for __cjsModule only appears after we decided to add __esModule, instead of emerging on its own long before, or that transpilers didn't implement it themselves already, like what happened with __esModule.

What more would you like to see tested with regards to this?

I was referring to this use case:

It is also helpful to support CJS transpiled into ESM (ie modern bundled code) running in Node.js against an external that is unknown as to whether it is CJS or ESM, where being able to know if the external is a CJS wrapper can allow lowering the required require(esm) interop pattern.

In #52166 we added a test fixture with the output of the actual TypeScript compiler. Before the PR, loading default exports of real ESM in transpiled ESM -> CJS code leads to an error. After it works intuitively. I think for this PR we'd need CJS -> ESM code emitted by an existing transpiler that doesn't load an external CJS without __cjsModule, but loads with the PR, and we commit the transpiler output as a test fixture.

[joyeecheung]

But the check is useful in its own right, and does not depend on their future emergence either.

I agree that it's useful in its own right, though currently for code only targeting Node.js this can also be worked around by checking whether the path exists in module.Module._cache, or try-catch require(cjs) to see if it loads and the default is equal to the required result. The reason why we decided to add __esModule directly in core was that asking all transpilers to use the alternative approach (checking required[Symbol.toStringTag] or util.isModuleNamespace(required) and then asking all their users to update their transpilers may be a great hassle. But since __cjsModule is not yet a convention picked up by any transpilers, it doesn't seem we are in a rush? It would be better to see transpilers actually pick this up and prove that it works before we add yet another underscore underscore property. Having one was unfortunate enough but was something we did for the sake of existing code that already relies on __esModule. Having another even though no code in the real world rely on __cjsModule convention and we only get some vague promise that it would be used seems even more unfortunate.

[joyeecheung]

For the specific case in OP, it seems the problem all stems from the proposed interop layer, which AFAIK no transpiler has implemented yet. The layer seems redundant for ESM -> CJS code, however. If you don't do that layer, and simply transpile this:

const depMod = require('dep');
const depNs = depMod.__esModule ? depMod : { default: depMod };
console.log(depNs.default);

to just this (IIUC, this is already what Rollup currently produce with interop: 'auto')

import depMod from 'dep';
const depNs = depMod.__esModule ? depMod : { default: depMod };
console.log(depNs.default);

Things already just work automatically. If dep is faux ESM it takes the first branch. If it's CJS, it takes the second branch and depNs.default will be the module.exports. If it's ESM, it also takes the second branch and depNS.default works just fine, since depMod would be the default export anyway. It only starts breaking if you add that layer, which doesn't seem necessary in the first place - the hack is only useful for transpiling import to require, not the other way around.

[guybedford]

The entire ecosystem does not need to move to support __cjsModule. While it might, I've only laid out reasons why this is a useful check to be able to make. The interop cases are real interop problems that __cjsModule can solve whatever tools or users or code builders hit them.

Your scheme in #53848 (comment) does not work for a real ES module (not faux ESM) without a default export, since it will bail that 'dep' does not export a 'default' export. This is a new case created by supporting require(esm) for real ES modules that we need the __cjsModule check for.

[joyeecheung]

If what you need is the ability fully comparable with require(esm), can't you just use globalThis.process.getBuiltinModule('node:module').createRequire(import.meta.url)? That would be Node.js specific and only works on newer versions of Node.js, but the same can be said about this __cjsModule thing unless you can convince browsers to support it too.

[guybedford]

The globalThis.process.getBuiltinModule('node:module').createRequire(import.meta.url) pattern does not support rebundling the module as part of another build process (say you later decide to inline dep). That is, it breaks transitivity in the workflow of applying transform and analysis to code, where tools generally want to operate on ESM modules. On the other hand, the __cjsModule pattern as I've described maintains this transitivity in workflows.

I think we're focusing a little too much on some hypothetical major adoption of this feature (I'm not really not sure what you mean by browser adoption), and a little too less on the problems it solves, and what reasons we might not want to land it. @joyeecheung if you are against this flag, perhaps you can more clearly state your underlying concerns?

[GeoffreyBooth]

RollupJS supports CommonJS output too. As far as I know typically people bundle code into CommonJS format to speed up startup times (ESM loading is slower than CommonJS loading in the first place), or to use the bundle with SEA or snapshots since they do not support ESM. But choosing the ESM output for a Node.js environment is rare.

I’ve been building SvelteKit apps for a few years now, and SvelteKit uses Vite with Rollup to generate ESM production builds. Startup time isn’t a concern for long-lived servers, and these builds aren’t bundling the external dependencies; I still need my node_modules folder. The production build needs to be ESM so that I can import ESM dependencies.

So no, I wouldn’t say that building for ESM output in a Node.js environment is rare.

[joyeecheung]

perhaps you can more clearly state your underlying concerns?

I've already stated this in #53848 (comment)

It would be better to see transpilers actually pick this up and prove that it works before we add yet another underscore underscore property. Having one was unfortunate enough but was something we did for the sake of existing code that already relies on __esModule. Having another even though no code in the real world rely on __cjsModule convention and we only get some vague promise that it would be used seems even more unfortunate.

In general, adding more underscore underscore property should be frowned upon. __esModule should just be an exception that we had to make to ease the migration of a huge amount of existing configurations, given that it's already a de-facto standard, but not a precedent that should be casually followed, especially when there aren't even any integration tests and we don't know for sure whether this underscore underscore property would actually be used and work as intended.

[joyeecheung]

I’ve been building SvelteKit apps for a few years now, and SvelteKit uses Vite with Rollup to generate ESM production builds. Startup time isn’t a concern for long-lived servers, and these builds aren’t bundling the external dependencies; I still need my node_modules folder. The production build needs to be ESM so that I can import ESM dependencies.

So it isn't transpiling CJS to ESM either, which is what this PR is about?

[guybedford]

This PR is not claiming to set a convention that will work for all bundlers, rather it is providing a solution to the problem that when building CJS requires as externals into ESM imports, we have an interop problem in distinguishing CJS ESM wrappers from real ESM and that currently has no viable solution.

And I would quite like to be able to solve this problem for our Node.js users.

I've already stated this in #53848 (comment)

This argument is circular - you are asking for adoption, but then also claiming you are concerned about adoption. The concern and request cannot be for the same thing?

Would you be less concerned if we renamed the flag to nodeCommonJsModule or something like that without a __ prefix? We have full control over how the CJS ESM shell is defined in Node.js.

There is also another side to this PR and that is that using this flag, it is possible to define the representation of an ESM module in require(esm). I would have thought you'd be interested in that given it can be used as another tool to solve require(esm) interop issues.

[joyeecheung]

And I would quite like to be able to solve this problem for our Node.js users.

Not saying that it isn't a problem, but it seems very well like an issue we can follow up later. This path needs code originally in CJS being transpiled to ESM and loading an external ESM to be hit. That doesn't seem to be an urgent issue - for one not that many people actually configure to transpile CJS to ESM for it to be run in Node.js, if they need to ship ESM they likely would just write in ESM in the first place and never hit this path (probably what @GeoffreyBooth does too), let alone attempting to loading real external ESM from said CJS code (considering how hacky CJS -> ESM can be in terms of handling dynamic exports and imports and all the wonky require extensions, this practice already has bigger fishes to fry anyway). This is unlike the problem that __esModule intends to solve because transpiling ESM to CJS is a much more common and stable practice, attempting to load an external real ESM from it is also fairly common, which can be shown by the mass ERR_REQUIRE_ESM confusion out there on the Internet.

This argument is circular - you are asking for adoption, but then also claiming you are concerned about adoption.

I don't think I claimed to be concerned about adoption, rather I am concerned about the lack of adoption of a premature solution that's currently purely on paper, and we have no integration tests to verify that this solution actually works as intended.

I would have thought you'd be interested in that given it can be used as another tool to solve require(esm) interop issues.

I'd be more interested to see working prototypes, the current description of the solution is too vague and abstract, and we don't know for sure whether it actually works as intended.

[joyeecheung]

Actually I realized that this is a breaking change, because unlike require(esm) import(cjs) is not experimental, and people might have been expecting the namespace object to be pristine e.g. people may have been effectively writing tests the way we did in the tests being updated in the PR here - and the tests had not changed for 5 years, but now this change would start breaking them and users would have to update their code:

import * as empty from './empty.cjs';
assert.deepStrictEqual({...empty}, { default: {} });    // it will throw after this change

Unlike __esModule, which we had to add to the required result directly to ease migration for existing configurations (and it's only effective in a new path that previously would just error), nobody is using __cjsModule anyway, so I think we could introduce something like this instead to avoid the breakage:

import { isCJSModule } from 'node:module';  // Or use process.getBuiltinModule();
import * as empty from './empty.cjs';
isCJSModule(empty);  // true

isCJSModule() (or other names) can just tag the module namespace using a weakmap to have fake private fields (weakmap semantics). If we go with this I am fine without integration tests since the compat risk of a side API is significantly lower.

[aduh95]

nit: the commit message does not meet the guidelines (the first word after the subsystem should be an imperative verb). This can be addressed when landing.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/62896/

[guybedford]

Will aim to land this in the next couple of hours, unless there are any further comments.

[guybedford]

Landed in 1d5ed72.