module: add __esModule to require()'d ESM #52166

joyeecheung · 2024-03-20T20:45:45Z

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 · 2024-03-20T20:45:50Z

Review requested:

@nodejs/loaders

nodejs-github-bot · 2024-03-20T20:50:12Z

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

joyeecheung · 2024-03-20T20:52:04Z

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 · 2024-03-20T21:56:50Z

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

guybedford · 2024-03-21T01:28:54Z

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

lib/internal/modules/cjs/loader.js

joyeecheung · 2024-03-21T06:42:55Z

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:

__esModule would be enumerable
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.

lib/internal/modules/cjs/loader.js

mcollina

lgtm

joyeecheung · 2024-03-21T14:03:24Z

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 · 2024-03-21T17:05:28Z

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 · 2024-03-22T16:03:46Z

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 · 2024-03-22T16:09:36Z

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 · 2024-03-25T19:20:39Z

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 · 2024-03-29T16:38:05Z

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 · 2024-04-14T10:21:58Z

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 · 2024-04-14T12:29:19Z

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 · 2024-04-14T12:36:23Z

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

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

justinfagnani · 2024-04-14T14:07:27Z

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 · 2024-04-14T14:29:06Z

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 · 2024-04-14T14:37:14Z

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 · 2024-04-14T15:35:35Z

@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 · 2024-04-14T15:43:55Z

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 · 2024-04-14T15:46:36Z

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 · 2024-04-14T16:22:50Z

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.

nodejs-github-bot · 2024-07-10T14:22:24Z

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

joyeecheung · 2024-07-10T17:47:03Z

CI is green. If there are no more concerns that cannot be resolved in follow ups, I would like to land this by the end of the week.

Pinging previous reviewers for another look. @ExE-Boss @nicolo-ribaudo @RedYetiDev @mcollina @panva @jasnell @guybedford @legendecas

nodejs-github-bot · 2024-07-11T18:51:22Z

Commit Queue failed

- Loading data for nodejs/node/pull/52166 ✔ Done loading data for nodejs/node/pull/52166 ----------------------------------- PR info ------------------------------------ Title module: add __esModule to require()'d ESM (#52166) Author Joyee Cheung <joyeec9h3@gmail.com> (@joyeecheung) Branch joyeecheung:namespace -> nodejs:main Labels notable-change, experimental, esm, needs-ci Commits 4 - benchmark: add require-esm benchmark - module: add __esModule to require()'d ESM - fixup! module: add __esModule to require()'d ESM - fixup! fixup! module: add __esModule to require()'d ESM Committers 1 - Joyee Cheung <joyeec9h3@gmail.com> PR-URL: https://github.com/nodejs/node/pull/52166 Refs: https://github.com/nodejs/node/issues/52134 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/52166 Refs: https://github.com/nodejs/node/issues/52134 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> -------------------------------------------------------------------------------- ℹ This PR was created on Wed, 20 Mar 2024 20:45:45 GMT ✔ Approvals: 6 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/52166#pullrequestreview-1951831589 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/52166#pullrequestreview-2114809241 ✔ - Filip Skokan (@panva): https://github.com/nodejs/node/pull/52166#pullrequestreview-2161011901 ✔ - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/52166#pullrequestreview-2161031143 ✔ - Guy Bedford (@guybedford): https://github.com/nodejs/node/pull/52166#pullrequestreview-2161288038 ✔ - Geoffrey Booth (@GeoffreyBooth) (TSC): https://github.com/nodejs/node/pull/52166#pullrequestreview-2172765608 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-07-10T14:22:24Z: https://ci.nodejs.org/job/node-test-pull-request/60214/ - Querying data for job/node-test-pull-request/60214/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 52166 From https://github.com/nodejs/node * branch refs/pull/52166/merge -> FETCH_HEAD ✔ Fetched commits as 05bb4a716b40..ae54d52b1282 -------------------------------------------------------------------------------- [main 3030bddce1] benchmark: add require-esm benchmark Author: Joyee Cheung <joyeec9h3@gmail.com> Date: Sat May 4 00:04:11 2024 +0200 1 file changed, 79 insertions(+) create mode 100644 benchmark/esm/require-esm.js Auto-merging lib/internal/modules/cjs/loader.js Auto-merging src/env.h Auto-merging src/env_properties.h [main 654d0691ad] module: add __esModule to require()'d ESM Author: Joyee Cheung <joyeec9h3@gmail.com> Date: Wed Mar 20 21:41:22 2024 +0100 16 files changed, 158 insertions(+), 46 deletions(-) Auto-merging doc/api/modules.md Auto-merging lib/internal/modules/cjs/loader.js [main 547d8de514] fixup! module: add __esModule to require()'d ESM Author: Joyee Cheung <joyeec9h3@gmail.com> Date: Fri Jul 5 18:23:56 2024 +0200 16 files changed, 157 insertions(+), 22 deletions(-) create mode 100644 test/es-module/test-require-module-defined-esmodule.js create mode 100644 test/es-module/test-require-module-transpiled.js create mode 100644 test/fixtures/es-modules/export-es-module.mjs create mode 100644 test/fixtures/es-modules/transpiled-cjs-require-module/dist/import-both.cjs create mode 100644 test/fixtures/es-modules/transpiled-cjs-require-module/dist/import-default.cjs create mode 100644 test/fixtures/es-modules/transpiled-cjs-require-module/dist/import-named.cjs create mode 100644 test/fixtures/es-modules/transpiled-cjs-require-module/node_modules/logger/logger.mjs create mode 100644 test/fixtures/es-modules/transpiled-cjs-require-module/node_modules/logger/package.json create mode 100644 test/fixtures/es-modules/transpiled-cjs-require-module/src/import-both.mjs create mode 100644 test/fixtures/es-modules/transpiled-cjs-require-module/src/import-default.mjs create mode 100644 test/fixtures/es-modules/transpiled-cjs-require-module/src/import-named.mjs create mode 100644 test/fixtures/es-modules/transpiled-cjs-require-module/transpile.cjs [main 6f5eb2824f] fixup! fixup! module: add __esModule to require()'d ESM Author: Joyee Cheung <joyeec9h3@gmail.com> Date: Wed Jul 10 16:15:27 2024 +0200 2 files changed, 19 insertions(+), 6 deletions(-) create mode 100644 test/fixtures/es-modules/export-es-module-2.mjs ✔ Patches applied There are 4 commits in the PR. Attempting autorebase. Rebasing (2/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
benchmark: add require-esm benchmark

PR-URL: #52166
Refs: #52134
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

[detached HEAD f3b823b824] benchmark: add require-esm benchmark
Author: Joyee Cheung <joyeec9h3@gmail.com>
Date: Sat May 4 00:04:11 2024 +0200
1 file changed, 79 insertions(+)
create mode 100644 benchmark/esm/require-esm.js
Rebasing (3/6)
Rebasing (4/6)
Rebasing (5/6)
Rebasing (6/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
module: add __esModule to require()'d ESM

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.

PR-URL: #52166
Refs: #52134
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

[detached HEAD 55b1ebdc05] module: add __esModule to require()'d ESM
Author: Joyee Cheung <joyeec9h3@gmail.com>
Date: Wed Mar 20 21:41:22 2024 +0100
30 files changed, 315 insertions(+), 55 deletions(-)
create mode 100644 test/es-module/test-require-module-defined-esmodule.js
create mode 100644 test/es-module/test-require-module-transpiled.js
create mode 100644 test/fixtures/es-modules/export-es-module-2.mjs
create mode 100644 test/fixtures/es-modules/export-es-module.mjs
create mode 100644 test/fixtures/es-modules/transpiled-cjs-require-module/dist/import-both.cjs
create mode 100644 test/fixtures/es-modules/transpiled-cjs-require-module/dist/import-default.cjs
create mode 100644 test/fixtures/es-modules/transpiled-cjs-require-module/dist/import-named.cjs
create mode 100644 test/fixtures/es-modules/transpiled-cjs-require-module/node_modules/logger/logger.mjs
create mode 100644 test/fixtures/es-modules/transpiled-cjs-require-module/node_modules/logger/package.json
create mode 100644 test/fixtures/es-modules/transpiled-cjs-require-module/src/import-both.mjs
create mode 100644 test/fixtures/es-modules/transpiled-cjs-require-module/src/import-default.mjs
create mode 100644 test/fixtures/es-modules/transpiled-cjs-require-module/src/import-named.mjs
create mode 100644 test/fixtures/es-modules/transpiled-cjs-require-module/transpile.cjs

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/9896900575

PR-URL: #52166 Refs: #52134 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

nodejs-github-bot · 2024-07-11T18:59:33Z

Landed in b6ca3d7...e77aac2

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. PR-URL: #52166 Refs: #52134 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

joyeecheung · 2024-07-11T21:47:48Z

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 (***)

PR-URL: #52166 Refs: #52134 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

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. PR-URL: #52166 Refs: #52134 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

Notable changes: http: * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721 module: * add __esModule to require()'d ESM (Joyee Cheung) #52166 path: * (SEMVER-MINOR) add `matchGlob` method (Aviv Keller) #52881 process: * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239 stream: * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462 worker: * (SEMVER-MINOR) add postMessageToThread (Paolo Insogna) #53682 PR-URL: TODO

Notable changes: http: * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721 module: * add __esModule to require()'d ESM (Joyee Cheung) #52166 path: * (SEMVER-MINOR) add `matchGlob` method (Aviv Keller) #52881 process: * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239 stream: * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462 worker: * (SEMVER-MINOR) add postMessageToThread (Paolo Insogna) #53682 PR-URL: #53826

PR-URL: #52166 Refs: #52134 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

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. PR-URL: #52166 Refs: #52134 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

Notable changes: http: * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721 lib: * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) #53752 module: * add `__esModule` to `require()`'d ESM (Joyee Cheung) #52166 path: * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) #52881 process: * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239 stream: * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462 test_runner: * support glob matching coverage files (Aviv Keller) #53553 worker: * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) #53682 PR-URL: #53826

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 20, 2024

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 20, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 20, 2024

legendecas reviewed Mar 21, 2024

View reviewed changes

lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved

legendecas added the esm Issues and PRs related to the ECMAScript Modules implementation. label Mar 21, 2024

nicolo-ribaudo mentioned this pull request Mar 21, 2024

module: increased surface for hazards with require(esm) experimental flag #52173

Open

nicolo-ribaudo reviewed Mar 21, 2024

View reviewed changes

lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved

mcollina approved these changes Mar 21, 2024

View reviewed changes

GeoffreyBooth approved these changes Jul 11, 2024

View reviewed changes

GeoffreyBooth added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 11, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 11, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 11, 2024

nodejs-github-bot closed this Jul 11, 2024

RedYetiDev added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 11, 2024

aduh95 mentioned this pull request Jul 12, 2024

V22.5.0 proposal #53826

Merged

guybedford mentioned this pull request Jul 14, 2024

module: __cjsModule on ESM CJS wrapper and in require ESM #53848

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: add __esModule to require()'d ESM #52166

module: add __esModule to require()'d ESM #52166

joyeecheung commented Mar 20, 2024 •

edited

Loading

nodejs-github-bot commented Mar 20, 2024

nodejs-github-bot commented Mar 20, 2024

joyeecheung commented Mar 20, 2024 •

edited

Loading

aduh95 commented Mar 20, 2024

guybedford commented Mar 21, 2024

joyeecheung commented Mar 21, 2024

nicolo-ribaudo left a comment •

edited

Loading

mcollina left a comment

joyeecheung commented Mar 21, 2024

nicolo-ribaudo commented Mar 21, 2024

justinfagnani commented Mar 22, 2024

nicolo-ribaudo commented Mar 22, 2024

joyeecheung commented Mar 25, 2024

joyeecheung commented Mar 29, 2024 •

edited

Loading

JakobJingleheimer commented Apr 14, 2024

joyeecheung commented Apr 14, 2024 •

edited

Loading

JakobJingleheimer commented Apr 14, 2024

justinfagnani commented Apr 14, 2024

joyeecheung commented Apr 14, 2024 •

edited

Loading

JakobJingleheimer commented Apr 14, 2024 •

edited

Loading

joyeecheung commented Apr 14, 2024 •

edited

Loading

justinfagnani commented Apr 14, 2024

joyeecheung commented Apr 14, 2024 •

edited

Loading

JakobJingleheimer commented Apr 14, 2024 •

edited

Loading

nodejs-github-bot commented Jul 10, 2024

joyeecheung commented Jul 10, 2024

nodejs-github-bot commented Jul 11, 2024

Successfully rebased and updated refs/heads/main.

nodejs-github-bot commented Jul 11, 2024

joyeecheung commented Jul 11, 2024 •

edited

Loading

module: add __esModule to require()'d ESM #52166

module: add __esModule to require()'d ESM #52166

Conversation

joyeecheung commented Mar 20, 2024 • edited Loading

nodejs-github-bot commented Mar 20, 2024

nodejs-github-bot commented Mar 20, 2024

joyeecheung commented Mar 20, 2024 • edited Loading

aduh95 commented Mar 20, 2024

guybedford commented Mar 21, 2024

joyeecheung commented Mar 21, 2024

nicolo-ribaudo left a comment • edited Loading

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

joyeecheung commented Mar 21, 2024

nicolo-ribaudo commented Mar 21, 2024

justinfagnani commented Mar 22, 2024

nicolo-ribaudo commented Mar 22, 2024

joyeecheung commented Mar 25, 2024

joyeecheung commented Mar 29, 2024 • edited Loading

JakobJingleheimer commented Apr 14, 2024

joyeecheung commented Apr 14, 2024 • edited Loading

JakobJingleheimer commented Apr 14, 2024

justinfagnani commented Apr 14, 2024

joyeecheung commented Apr 14, 2024 • edited Loading

JakobJingleheimer commented Apr 14, 2024 • edited Loading

joyeecheung commented Apr 14, 2024 • edited Loading

justinfagnani commented Apr 14, 2024

joyeecheung commented Apr 14, 2024 • edited Loading

JakobJingleheimer commented Apr 14, 2024 • edited Loading

nodejs-github-bot commented Jul 10, 2024

joyeecheung commented Jul 10, 2024

nodejs-github-bot commented Jul 11, 2024

Successfully rebased and updated refs/heads/main.

nodejs-github-bot commented Jul 11, 2024

joyeecheung commented Jul 11, 2024 • edited Loading

joyeecheung commented Mar 20, 2024 •

edited

Loading

joyeecheung commented Mar 20, 2024 •

edited

Loading

nicolo-ribaudo left a comment •

edited

Loading

joyeecheung commented Mar 29, 2024 •

edited

Loading

joyeecheung commented Apr 14, 2024 •

edited

Loading

joyeecheung commented Apr 14, 2024 •

edited

Loading

JakobJingleheimer commented Apr 14, 2024 •

edited

Loading

joyeecheung commented Apr 14, 2024 •

edited

Loading

joyeecheung commented Apr 14, 2024 •

edited

Loading

JakobJingleheimer commented Apr 14, 2024 •

edited

Loading

joyeecheung commented Jul 11, 2024 •

edited

Loading