module: add __esModule to require()'d ESM

[joyeecheung]

Before this PR, trying to load real ESM from transpiled ESM would throw errors like this with --experimental-require-module

// 'logger' package being loaded as real ESM
export default class Logger { log(val) { console.log(val); } }
export function log(logger, val) { logger.log(val) };

// Consuming code originally authored in ESM, but transpiled to CommonJS before being loaded by Node.js
import Logger, { log } from 'logger';
log(new Logger(), 'import both');

/Users/joyee/projects/node/test/fixtures/es-modules/transpiled-cjs-require-module/dist/import-both.cjs:27
(0, logger_1.log)(new logger_1.default(), 'import both');
                  ^

TypeError: logger_1.default is not a constructor
    at Object.<anonymous> (/Users/joyee/projects/node/test/fixtures/es-modules/transpiled-cjs-require-module/dist/import-both.cjs:27:19)
    at Module._compile (node:internal/modules/cjs/loader:1460:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1544:10)
    at Module.load (node:internal/modules/cjs/loader:1275:32)
    at Module._load (node:internal/modules/cjs/loader:1091:12)
    at wrapModuleLoad (node:internal/modules/cjs/loader:212:19)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:158:5)
    at node:internal/main/run_main_module:30:49

After this PR it logs 'import both'.

Tooling in the ecosystem have been using the __esModule property to
recognize transpiled ESM in consuming code. For example, a 'log'
package written in ESM:

export function log(val) { console.log(val); }

Can be transpiled as:

exports.__esModule = true;
exports.default = function log(val) { console.log(val); }

The consuming code may be written like this in ESM:

import log from 'log'

Which gets transpiled to:

const _mod = require('log');
const log = _mod.__esModule ? _mod.default : _mod;

So to allow transpiled consuming code to recognize require()'d real ESM
as ESM and pick up the default exports, we add a __esModule property by
building a source text module facade for any module that has a default
export and add .__esModule = true to the exports. We don't do this to
modules that don't have default exports to avoid the unnecessary
overhead. This maintains the enumerability of the re-exported names
and the live binding of the exports.

The source of the facade is defined as a constant per-isolate property
required_module_facade_source_string, which looks like this

export * from 'original';
export { default } from 'original';
export const __esModule = true;

And the 'original' module request is always resolved by
createRequiredModuleFacade() to wrap which is a ModuleWrap wrapping
over the original module.

This PR originally used the same trick that Bun did (h/t @Jarred-Sumner) by putting the original module namespace in the prototype chain. Upon closer examination this now switched to use a SourceTextModule facade only when the exports contain default to 1) reduce the performance impact 2) ensure that the exported names are still enumerable and can be copied by tools.

Refs: #51977 (comment)
Refs: #52134

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[nodejs-github-bot]

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

[joyeecheung]

cc @nicolo-ribaudo

While this approach strikes a balance between performance and compatibility, so I think we should do this for now, there are some glitches that may or may not matter: because the namespace is in the prototype chain, users can't easily spread or copy the exports. Object.keys() won't return the keys of the exports either. So if users want to copy the exports or get the keys (maybe to create a mock or something), they probably will end up finding their way to the namespace in the prototype chain and do what they want. I suspect it's not that rare a use case. But maybe it's inevitable to work with some quirks to support required ESM.

I wonder if it's possible to get V8 to allow the host (not random users) to customize the prototype of module namespace objects - doesn't seem that hard to do, implementation wise, and V8 already allows embedders to customize e.g. the global object template - then we can put { __esModule: true } on the prototype of the namespace object instead, which probably would be a lot more natural. But perhaps that requires a spec change and is a bit far-fetched. So this is probably as good as we can get for now.

[aduh95]

IIUC, the spec requires the module exotic objects to have null as prototype: https://tc39.es/ecma262/#sec-module-namespace-exotic-objects-getprototypeof

[guybedford]

Could we create a synthetic module wrapper here effectively like export * from 'mod'; export const __esModule = true?

[joyeecheung]

Could we create a synthetic module wrapper here effectively like export * from 'mod'; export const __esModule = true?

That was brought up in #52134 but has the following cons:

1. __esModule would be enumerable
2. Module facade can lead to a non-trivial overhead. Take lib: only build the ESM facade for builtins when they are needed #51669 (comment) for example, it regressed 9% of the startup even with only the facades created for the handful of builtins (I think SourceTextModule facade would be even more expensive than SyntheticModule ones). Part of the reason why we are doing this in core instead of letting transpilers to handle it is for performance. The facade would cancel that.

[nicolo-ribaudo]

Thanks for this :) Note that I believe that this is much more important because it makes Node.js implementation compatible with the existing ecosystem, rather than because of the performance of a hypothetical feature in which every single tool in the ecosystem aligns to Node.js's non-__esModule implementation.

I wonder if it's possible to get V8 to allow the host (not random users) to customize the prototype of module namespace objects - doesn't seem that hard to do, implementation wise, and V8 already allows embedders to customize e.g. the global object template - then we can put { __esModule: true } on the prototype of the namespace object instead, which probably would be a lot more natural. But perhaps that requires a spec change and is a bit far-fetched. So this is probably as good as we can get for now.

Yeah as @aduh95 pointed out, this doesn't only require a change in V8 but also in the spec. Additionally, it has the problem that when a module is both imported and require()d you probably don't want the namespace to have the weird prototype in both cases.

While this approach strikes a balance between performance and compatibility, so I think we should do this for now, there are some glitches that may or may not matter: because the namespace is in the prototype chain, users can't easily spread or copy the exports. Object.keys() won't return the keys of the exports either. So if users want to copy the exports or get the keys (maybe to create a mock or something), they probably will end up finding their way to the namespace in the prototype chain and do what they want. I suspect it's not that rare a use case. But maybe it's inevitable to work with some quirks to support required ESM.

Oh yeah I didn't think about that when I included the object wrapping in the possible alternatives. However, it's already possible for require() to return objects with important stuff on their prototype (module.exports = new MyUtilitiesSingleton()).

I guess it's a question of how common it is to spread/keys the require()d object.

[mcollina]

lgtm

[joyeecheung]

I wonder how important it is for __esModule to be not enumerable? I can also do a prototype and check the performance of the synthetic module. While it's going to be expensive, it more or less already happens with import cjs or import 'node:builtin', we could focus on the most sane behavior first, then think about optimizations we can make (e.g. I think it should be possible to do some optimizations in V8 for this).

[nicolo-ribaudo]

I wonder how important it is for __esModule to be not enumerable?

Babel actually has a loose mode that makes it enumerable (through simple assignment, to get a smaller compiled output). It doesn't matter in most cases, except... when you spread/Object.keys/for...in, which are the same cases in which the object wrapper implemented by this PR is annoying 😬

[justinfagnani]

Could you generate the new object from the namespace object, creating own enumerable getters for every export to support live bindings, and add the non-enumberable __esModule property to that?

Then spread and Object.keys() will work, and the object can have a null prototype - at the cost of more complex initialization.

[nicolo-ribaudo]

If that is done, then I suggest that that object should also be frozen and have [Symbol.toStringTag]: "Module", to look almost like a namespace object (except that namespace objects don't actually have getters).

[joyeecheung]

I am going to do some investigation of the real performance impact before proceeding with this, because this could lead to behavior changes that may be difficult to back out of in the future. Behavior-wise, I still prefer to see reference equal import() and require() results. If not, then at least make the results copy-able or allow Object.keys() to work.

[joyeecheung]

Could you generate the new object from the namespace object, creating own enumerable getters for every export to support live bindings, and add the non-enumberable __esModule property to that?

Experimenting with the idea, I noticed that if we do this, we may be able to allow CJS -> ESM cycles (because we can then lazily return the exports of a ESM), that might be desirable for UX, regardless of the __esModule question?

(One downside of relying on this for cycles is that users might still have race conditions until we make the ESM loader fully conditionally synchronous).

[JakobJingleheimer]

I think this is what people are already advocating here, but just in case and to be explicit: We have precedent for this in import(esm) (and import * as mod from 'some-package'), so I think the return of require(esm) should be the same, and that should facilitate referential equality.

[joyeecheung]

We have precedent for this in import(esm) (and import * as mod from 'some-package')

Not sure if I'm following - what's the precedent specifically?

And, as long as we do want to insert __esModule into the namespace, the result of import(esm) and require(esm) won't be reference equal anymore, unless we also insert it to the result of import(esm).

[JakobJingleheimer]

unless we also insert it to the result of import(esm)

🤔 sure? No reason comes to mind to not do that.

[justinfagnani]

unless we also insert it to the result of import(esm)

🤔 sure? No reason comes to mind to not do that.

The reason not to is that it's not part of the JS modules spec and once shipped could never be removed from import(). If there a way to limit the scope, like it's only added for dynamic import() within CJS modules, then it might be somewhat less impactful?

Do people need require(foo) === await import(foo) anyway? What are they doing with that? Is it some kind of environment detection? Are they using modules in a map that have been imported different ways? I'm not sure why any generic module loading that supports JS modules wouldn't be using import() everywhere.

[joyeecheung]

Do people need require(foo) === await import(foo) anyway?

Actually I don't think it matter that much, because import(cjs) (returns a synthetic module namespace) is not reference equal to require(cjs) already, and no one complains AFAIK (though it might also have to do with that it's probably not too common to import(cjs)).

[JakobJingleheimer]

Do people need require(foo) === await import(foo) anyway?

It's not for comparison; it facilitates avoiding the dual-package hazard (and thus can vastly simplify a package's internals):

const mod = require('some-package');

mod.foo++;

import * as mod from 'some-package';

mod.foo++;

In the above, assuming the package is done right, foo will be the same foo and thus be incremented twice (instead of two different foos each getting incremented once each). Absolutely no-one wants two different foo here.

[joyeecheung]

@JakobJingleheimer I don't think this use case needs reference equality of "the thing that gets returned by require() or import()", what it needs is live binding of what's inside those returned objects. The example still works even if mod.foo are done from re-exported getter/setters (solution proposed #52166 (comment)), or if mod.foo comes from the prototype (the original solution that's still in this PR). Although I do think this suggests that we shouldn't do #52166 (comment) because then you'll be modifying the live binding of the facade, not the original module.

[justinfagnani]

import * as mod from 'some-package';

mod.foo++;

In the above, assuming the package is done right, foo will be the same foo and thus be incremented twice (instead of two different foos each getting incremented once each). Absolutely no-one wants two different foo here.

mod.foo++ would throw a TypeError because JS module exports are readonly. But the point about the underlying module instance stands - and is met by the proposed solutions here.

It's just that the example needs to be:

const mod = require('some-package');
mod.incrementFoo();

import * as mod from 'some-package';
mod.incrementFoo();

where mod.incrementFoo() mutates the same module instance in both cases.

[joyeecheung]

Right actually I don't think it matters for #52166 (comment) either since the module namespace is not mutable. And if the mutation is done via a method or on an exported object's properties, all the solutions proposed so far behave the same, they will just modify what's in the underlying module.

[JakobJingleheimer]

If it's not a pointer, that'll be hell to manage. Yes we can, but surely a pointer is the most simple and desirable, no?

It's just that the example needs to be:

const mod = require('some-package');
mod.incrementFoo();
import * as mod from 'some-package';
mod.incrementFoo();
where mod.incrementFoo() mutates the same module instance in both cases

Ah, yes—fair point (I oversimplified). As long as they point to the same underlying state, great 😊 but hopefully there is as little in the middle as possible/necessary.

[joyeecheung]

BTW I ran the benchmarks on Apple M1 with the last iteration (the funny results in #52166 (comment) were from AWS server, not sure what's going on there), the last one is the landed version.

❯ node-benchmark-compare esm-proxy.csv
                                                          confidence improvement accuracy (*)   (**)  (***)
esm/require-esm.js n=1000 exports='default' type='access'        ***    -68.33 %       ±0.81% ±1.09% ±1.45%
esm/require-esm.js n=1000 exports='default' type='all'            **     -1.74 %       ±1.06% ±1.41% ±1.84%
esm/require-esm.js n=1000 exports='default' type='load'                  -0.18 %       ±1.25% ±1.66% ±2.16%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 3 comparisons, you can thus expect the following amount of false-positive results:
  0.15 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.03 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)
❯ node-benchmark-compare esm-desc.csv
                                                          confidence improvement accuracy (*)   (**)  (***)
esm/require-esm.js n=1000 exports='default' type='access'        ***    -54.01 %       ±0.85% ±1.15% ±1.51%
esm/require-esm.js n=1000 exports='default' type='all'           ***     -3.79 %       ±1.09% ±1.44% ±1.88%
esm/require-esm.js n=1000 exports='default' type='load'          ***     -4.24 %       ±1.11% ±1.47% ±1.92%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 3 comparisons, you can thus expect the following amount of false-positive results:
  0.15 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.03 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)
❯ node-benchmark-compare esm-proto.csv
                                                          confidence improvement accuracy (*)   (**)  (***)
esm/require-esm.js n=1000 exports='default' type='access'        ***     -3.15 %       ±0.80% ±1.07% ±1.40%
esm/require-esm.js n=1000 exports='default' type='all'            **     -2.68 %       ±1.59% ±2.11% ±2.75%
esm/require-esm.js n=1000 exports='default' type='load'           **     -5.36 %       ±3.41% ±4.59% ±6.07%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 3 comparisons, you can thus expect the following amount of false-positive results:
  0.15 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.03 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)
❯ node-benchmark-compare esm-stm.csv
                                                          confidence improvement accuracy (*)   (**)  (***)
esm/require-esm.js n=1000 exports='default' type='access'        ***     -3.18 %       ±0.98% ±1.30% ±1.69%
esm/require-esm.js n=1000 exports='default' type='all'           ***     -3.78 %       ±1.13% ±1.51% ±1.97%
esm/require-esm.js n=1000 exports='default' type='load'          ***     -4.13 %       ±1.13% ±1.51% ±1.96%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 3 comparisons, you can thus expect the following amount of false-positive results:
  0.15 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.03 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)