Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Follow ecosystem convention for __esModule on require(esm) #52134

Closed
nicolo-ribaudo opened this issue Mar 18, 2024 · 7 comments
Closed

Follow ecosystem convention for __esModule on require(esm) #52134

nicolo-ribaudo opened this issue Mar 18, 2024 · 7 comments
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. feature request Issues that request new features to be added to Node.js. module Issues and PRs related to the module subsystem. stale

Comments

@nicolo-ribaudo
Copy link
Contributor

What is the problem this feature will solve?

First of all, I'm incredibly excited for #51977 :) It's great to finally see the biggest pain point when using Node.js being addressed.

However, Node.js is not the first tool to implement require(esm) and it would be great if it could follow the existing ecosystem convention. Given this code:

// main.cjs
const mod = require("./dep.mjs");
console.log(Object.getOwnPropertyDescriptors(mod));
// dep.mjs
export let foo = 1;

Webpack, Rollup, Parcel, esbuild and Bun will all log an object with an __esModule: true property. This convention makes sure that the code will behave the same whether dep.mjs runs as native ESM or as ESM-compiled-to-CJS.

While this doesn't really matter when the CJS and ESM files are in the same project, it's very helpful at the boundary between one library and another. Consider this case:

import add from "add-numbers";

console.log("2 + 2 is", add(2, 2));
// add-numbers
export default (x, y) => x + y;

When running as ESM, this obviously logs 2 + 2 is 4. When running as ESM-compiled-to-CJS, this also logs the same result because the two files are compiled to (roughly)

const _addNumbers1 = require("add-numbers");
const _addNumbers = _addNumbers1.__esModule ? _addNumbers1 : { default: _addNumbers1 };

console.log("2 + 2 is", _addNumbers.default(2, 2));
// add-numbers
exports.__esModule = true;
exports.default = (x, y) => x + y;

Now, let's assume that the maintainer of add-numbers learns that Node.js now supports require(esm), and decides to release the new version of their library as ESM since this won't have anymore the annoying effect of forcing its consumers to migrate to ESM.
If their user update to the new version, when running in Node.js their code will now stop working, throwing something like _addNumbers.default is not a function (because _addNumbers1 is not the module namespace object). Instead, they have to change their code to this:

import add from "add-numbers";

console.log("2 + 2 is", add.default(2, 2));

so that their existing build process transpiles it to

const _addNumbers1 = require("add-numbers");
const _addNumbers = _addNumbers1.__esModule ? _addNumbers1 : { default: _addNumbers1 };

console.log("2 + 2 is", _addNumbers.default.default(2, 2));

Their code will now work with the ESM version of add-numbers when running in Node.js, but:

  • it required extra code changes
  • it now won't work outside of Node.js (i.e. in bundlers), because when bundled the namespace object will be _addNumbers and not _addNumbers.default

Note that Node.js already has this problem when migrating to ESM the other way around (i.e. when migrating the consumer before the library), but it is for reasonable historic reasons (that is, the original Node.js CJS-ESM integration was just "assign module.exports to the default export" without trying to make CJS being importable as if it was an ESM file with various exports), and it looks like it would be unfixable even through some opt-in mechanism.

What is the feature you are proposing to solve the problem?

Instead of returning the module namespace as-is, Node.js should wrap it in something that adds an __esModule: true property to it. There are three ways of doing it:

  1. Cloning the namespace object: { __proto__: null, __esModule: true, ...namespace }
  2. Wrapping the module in an intermediate module: export let __esModule = true; export { default } from "./that-module"; export * from "./that-module";
  3. Wrapping the namespace object: Object.create(namespace, { __esModule: { value: true } })

I would personally recommend the third one, because:

  • it supports live bindings (the first one does not)
  • it makes __esModule non-enumerable (the second one does not)

This wrapping could either be done unconditionally, or only if the module doesn't already have an __esModule export (which is very rare, as it's incompatible with all tools that compile ESM to CJS).

What alternatives have you considered?

Joyee suggested to instead update the detection in tools from

const _addNumbers = _addNumbers1.__esModule ? _addNumbers1 : { default: _addNumbers1 };

to

const _addNumbers = _addNumbers1.__esModule || require("util").types.isModuleNamespaceObject(_addNumbers1) ? _addNumbers1 : { default: _addNumbers1 };

However, this a few disadvantages:

  • the major (more social than technical) one is that it requires every single tool out there that compiles ESM to CommonJS to be updated.
  • the second (minor) one is that it makes the "was this a module?" detection CommonJS-specific. Right now tools use the same require() wrapper when compiling to CommonJS, AMD or UMD -- CommonJS and AMD/UMD would now need different detections to accomodate the difference of one of the platforms with ESM-CJS interop.
  • the third one is that require("util").types.isModuleNamespaceObject is Node.js-specific. That check will not work when bundled and sent to the browser, unless users figure out how to polyfill Node.js builtin modules in their tool (and still, googling for "webpack require util not found" leads to a StackOverflow answer that suggests a node:util polyfill that does not support isModuleNamespaceObject, because it's unpolyfillable).
@nicolo-ribaudo nicolo-ribaudo added the feature request Issues that request new features to be added to Node.js. label Mar 18, 2024
@GeoffreyBooth
Copy link
Member

@nodejs/loaders

@nicolo-ribaudo
Copy link
Contributor Author

They are for the two opposite directions: that one is for importing CJS, this one is for requiring ESM. None of the concerns listed there apply here, because the concerns there are about compatibility with older Node.js versions.

@guybedford
Copy link
Contributor

An alternative isModule check that can be done here is mod[Symbol.toStringTag] === 'Module'. Adding this check alongside an __esModule check might work for both ESM externals in browsers and ESM externals in Node.js.

@Jarred-Sumner
Copy link

const _addNumbers = _addNumbers1.__esModule || require("util").types.isModuleNamespaceObject(_addNumbers1) ? _addNumbers1 : { default: _addNumbers1 };

If I'm understanding correctly, this approach adds extra work at each re-export site. IMO, it's really important to not make apps take longer to start. For apps that have lots of exports, this will make apps start slower, especially in files with a large number of exports (like barrel files / icons)

@joyeecheung
Copy link
Member

joyeecheung commented Mar 19, 2024

I think when I saw #51977 (comment) I probably misread it as option 1 here. Option 3 definitely looks better than other alternatives. It doesn't feel as nice if import() results are not reference equal to require() results, but I guess that probably hurts nobody(?).

@joyeecheung joyeecheung added module Issues and PRs related to the module subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. labels Mar 20, 2024
nodejs-github-bot pushed a commit that referenced this issue Jul 11, 2024
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 pushed a commit that referenced this issue Jul 11, 2024
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>
aduh95 pushed a commit that referenced this issue Jul 12, 2024
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>
aduh95 pushed a commit that referenced this issue Jul 12, 2024
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>
aduh95 pushed a commit that referenced this issue Jul 16, 2024
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>
aduh95 pushed a commit that referenced this issue Jul 16, 2024
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>
ehsankhfr pushed a commit to ehsankhfr/node that referenced this issue Jul 18, 2024
PR-URL: nodejs#52166
Refs: nodejs#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>
ehsankhfr pushed a commit to ehsankhfr/node that referenced this issue Jul 18, 2024
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: nodejs#52166
Refs: nodejs#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>
marco-ippolito pushed a commit that referenced this issue Aug 19, 2024
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>
Copy link
Contributor

There has been no activity on this feature request for 5 months. To help maintain relevant open issues, please add the never-stale Mark issue so that it is never considered stale label or close this issue if it should be closed. If not, the issue will be automatically closed 6 months after the last non-automated comment.
For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Sep 17, 2024
@joyeecheung
Copy link
Member

Fixed by #52166

targos pushed a commit that referenced this issue Sep 21, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. feature request Issues that request new features to be added to Node.js. module Issues and PRs related to the module subsystem. stale
Projects
None yet
Development

No branches or pull requests

5 participants