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

PR-URL: https://github.com/nodejs/node/pull/53848
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

doc/api/esm.md

@@ -474,8 +474,31 @@ See [Loading ECMAScript modules using `require()`][] for details.
 
 ### CommonJS Namespaces
 
+<!-- YAML
+added: v14.13.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/53848
+    description: Added `'module.exports'` export marker to CJS namespaces.
+-->
+
 CommonJS modules consist of a `module.exports` object which can be of any type.
 
+To support this, when importing CommonJS from an ECMAScript module, a namespace
+wrapper for the CommonJS module is constructed, which always provides a
+`default` export key pointing to the CommonJS `module.exports` value.
+
+In addition, a heuristic static analysis is performed against the source text of
+the CommonJS module to get a best-effort static list of exports to provide on
+the namespace from values on `module.exports`. This is necessary since these
+namespaces must be constructed prior to the evaluation of the CJS module.
+
+These CommonJS namespace objects also provide the `default` export as a
+`'module.exports'` named export, in order to unambiguously indicate that their
+representation in CommonJS uses this value, and not the namespace value. This
+mirrors the semantics of the handling of the `'module.exports'` export name in
+[`require(esm)`][] interop support.
+
 When importing a CommonJS module, it can be reliably imported using the ES
 module default import or its corresponding sugar syntax:
 
@@ -483,9 +506,7 @@ module default import or its corresponding sugar syntax:
 
 ```js
 import { default as cjs } from 'cjs';
-
-// The following import statement is "syntax sugar" (equivalent but sweeter)
-// for `{ default as cjsSugar }` in the above import statement:
+// identical to the above
 import cjsSugar from 'cjs';
 
 console.log(cjs);
@@ -495,10 +516,6 @@ console.log(cjs === cjsSugar);
 //   true
 ```
 
-The ECMAScript Module Namespace representation of a CommonJS module is always
-a namespace with a `default` export key pointing to the CommonJS
-`module.exports` value.
-
 This Module Namespace Exotic Object can be directly observed either when using
 `import * as m from 'cjs'` or a dynamic import:
 
@@ -509,7 +526,7 @@ import * as m from 'cjs';
 console.log(m);
 console.log(m === await import('cjs'));
 // Prints:
-//   [Module] { default: <module.exports> }
+//   [Module] { default: <module.exports>, 'module.exports': <module.exports> }
 //   true
 ```
 
@@ -540,7 +557,12 @@ console.log(cjs);
 
 import * as m from './cjs.cjs';
 console.log(m);
-// Prints: [Module] { default: { name: 'exported' }, name: 'exported' }
+// Prints:
+//   [Module] {
+//     default: { name: 'exported' },
+//     'module.exports': { name: 'exported' },
+//     name: 'exported'
+//   }
 ```
 
 As can be seen from the last example of the Module Namespace Exotic Object being
@@ -1103,6 +1125,7 @@ resolution for ESM specifiers is [commonjs-extension-resolution-loader][].
 [`package.json`]: packages.md#nodejs-packagejson-field-definitions
 [`path.dirname()`]: path.md#pathdirnamepath
 [`process.dlopen`]: process.md#processdlopenmodule-filename-flags
+[`require(esm)`]: modules.md#loading-ecmascript-modules-using-require
 [`url.fileURLToPath()`]: url.md#urlfileurltopathurl-options
 [cjs-module-lexer]: https://github.com/nodejs/cjs-module-lexer/tree/1.2.2
 [commonjs-extension-resolution-loader]: https://github.com/nodejs/loaders-test/tree/main/commonjs-extension-resolution-loader

lib/internal/modules/esm/translators.js

@@ -2,6 +2,7 @@
 
 const {
   ArrayPrototypeMap,
+  ArrayPrototypePush,
   Boolean,
   FunctionPrototypeCall,
   JSONParse,
@@ -182,14 +183,17 @@ function createCJSModuleWrap(url, source, isMain, loadCJS = loadCJSModule) {
 
   const { exportNames, module } = cjsPreparseModuleExports(filename, source);
   cjsCache.set(url, module);
-  const namesWithDefault = exportNames.has('default') ?
-    [...exportNames] : ['default', ...exportNames];
+
+  const wrapperNames = [...exportNames, 'module.exports'];
+  if (!exportNames.has('default')) {
+    ArrayPrototypePush(wrapperNames, 'default');
+  }
 
   if (isMain) {
     setOwnProperty(process, 'mainModule', module);
   }
 
-  return new ModuleWrap(url, undefined, namesWithDefault, function() {
+  return new ModuleWrap(url, undefined, wrapperNames, function() {
     debug(`Loading CJSModule ${url}`);
 
     if (!module.loaded) {
@@ -204,8 +208,7 @@ function createCJSModuleWrap(url, source, isMain, loadCJS = loadCJSModule) {
       ({ exports } = module);
     }
     for (const exportName of exportNames) {
-      if (!ObjectPrototypeHasOwnProperty(exports, exportName) ||
-          exportName === 'default') {
+      if (!ObjectPrototypeHasOwnProperty(exports, exportName) || exportName === 'default') {
         continue;
       }
       // We might trigger a getter -> dont fail.
@@ -218,6 +221,7 @@ function createCJSModuleWrap(url, source, isMain, loadCJS = loadCJSModule) {
       this.setExport(exportName, value);
     }
     this.setExport('default', exports);
+    this.setExport('module.exports', exports);
   }, module);
 }
 

test/es-module/test-esm-exports.mjs

@@ -8,50 +8,53 @@ import fromInside from '../fixtures/node_modules/pkgexports/lib/hole.js';
 [requireFixture, importFixture].forEach((loadFixture) => {
   const isRequire = loadFixture === requireFixture;
 
+  const maybeWrapped = isRequire ? (exports) => exports :
+    (exports) => ({ ...exports, 'module.exports': exports.default });
+
   const validSpecifiers = new Map([
     // A simple mapping of a path.
-    ['pkgexports/valid-cjs', { default: 'asdf' }],
+    ['pkgexports/valid-cjs', maybeWrapped({ default: 'asdf' })],
     // A mapping pointing to a file that needs special encoding (%20) in URLs.
-    ['pkgexports/space', { default: 'encoded path' }],
+    ['pkgexports/space', maybeWrapped({ default: 'encoded path' })],
     // Verifying that normal packages still work with exports turned on.
     isRequire ? ['baz/index', { default: 'eye catcher' }] : [null],
     // Fallbacks
-    ['pkgexports/fallbackdir/asdf.js', { default: 'asdf' }],
-    ['pkgexports/fallbackfile', { default: 'asdf' }],
+    ['pkgexports/fallbackdir/asdf.js', maybeWrapped({ default: 'asdf' })],
+    ['pkgexports/fallbackfile', maybeWrapped({ default: 'asdf' })],
     // Conditional split for require
     ['pkgexports/condition', isRequire ? { default: 'encoded path' } :
-      { default: 'asdf' }],
+      maybeWrapped({ default: 'asdf' })],
     // String exports sugar
-    ['pkgexports-sugar', { default: 'main' }],
+    ['pkgexports-sugar', maybeWrapped({ default: 'main' })],
     // Conditional object exports sugar
     ['pkgexports-sugar2', isRequire ? { default: 'not-exported' } :
-      { default: 'main' }],
+      maybeWrapped({ default: 'main' })],
     // Resolve self
     ['pkgexports/resolve-self', isRequire ?
       { default: 'self-cjs' } : { default: 'self-mjs' }],
     // Resolve self sugar
-    ['pkgexports-sugar', { default: 'main' }],
+    ['pkgexports-sugar', maybeWrapped({ default: 'main' })],
     // Path patterns
-    ['pkgexports/subpath/sub-dir1', { default: 'main' }],
-    ['pkgexports/subpath/sub-dir1.js', { default: 'main' }],
-    ['pkgexports/features/dir1', { default: 'main' }],
-    ['pkgexports/dir1/dir1/trailer', { default: 'main' }],
-    ['pkgexports/dir2/dir2/trailer', { default: 'index' }],
-    ['pkgexports/a/dir1/dir1', { default: 'main' }],
-    ['pkgexports/a/b/dir1/dir1', { default: 'main' }],
+    ['pkgexports/subpath/sub-dir1', maybeWrapped({ default: 'main' })],
+    ['pkgexports/subpath/sub-dir1.js', maybeWrapped({ default: 'main' })],
+    ['pkgexports/features/dir1', maybeWrapped({ default: 'main' })],
+    ['pkgexports/dir1/dir1/trailer', maybeWrapped({ default: 'main' })],
+    ['pkgexports/dir2/dir2/trailer', maybeWrapped({ default: 'index' })],
+    ['pkgexports/a/dir1/dir1', maybeWrapped({ default: 'main' })],
+    ['pkgexports/a/b/dir1/dir1', maybeWrapped({ default: 'main' })],
 
     // Deprecated:
     // Double slashes:
-    ['pkgexports/a//dir1/dir1', { default: 'main' }],
+    ['pkgexports/a//dir1/dir1', maybeWrapped({ default: 'main' })],
     // double slash target
-    ['pkgexports/doubleslash', { default: 'asdf' }],
+    ['pkgexports/doubleslash', maybeWrapped({ default: 'asdf' })],
     // Null target with several slashes
-    ['pkgexports/sub//internal/test.js', { default: 'internal only' }],
-    ['pkgexports/sub//internal//test.js', { default: 'internal only' }],
-    ['pkgexports/sub/////internal/////test.js', { default: 'internal only' }],
+    ['pkgexports/sub//internal/test.js', maybeWrapped({ default: 'internal only' })],
+    ['pkgexports/sub//internal//test.js', maybeWrapped({ default: 'internal only' })],
+    ['pkgexports/sub/////internal/////test.js', maybeWrapped({ default: 'internal only' })],
     // trailing slash
     ['pkgexports/trailing-pattern-slash/',
-     { default: 'trailing-pattern-slash' }],
+     maybeWrapped({ default: 'trailing-pattern-slash' })],
   ]);
 
   if (!isRequire) {
@@ -214,11 +217,15 @@ import fromInside from '../fixtures/node_modules/pkgexports/lib/hole.js';
 
 const { requireFromInside, importFromInside } = fromInside;
 [importFromInside, requireFromInside].forEach((loadFromInside) => {
+  const isRequire = loadFromInside === requireFromInside;
+  const maybeWrapped = isRequire ? (exports) => exports :
+    (exports) => ({ ...exports, 'module.exports': exports.default });
+
   const validSpecifiers = new Map([
     // A file not visible from outside of the package
-    ['../not-exported.js', { default: 'not-exported' }],
+    ['../not-exported.js', maybeWrapped({ default: 'not-exported' })],
     // Part of the public interface
-    ['pkgexports/valid-cjs', { default: 'asdf' }],
+    ['pkgexports/valid-cjs', maybeWrapped({ default: 'asdf' })],
   ]);
   for (const [validSpecifier, expected] of validSpecifiers) {
     if (validSpecifier === null) continue;

test/es-module/test-esm-imports.mjs

@@ -9,23 +9,26 @@ const { requireImport, importImport } = importer;
 [requireImport, importImport].forEach((loadFixture) => {
   const isRequire = loadFixture === requireImport;
 
+  const maybeWrapped = isRequire ? (exports) => exports :
+    (exports) => ({ ...exports, 'module.exports': exports.default });
+
   const internalImports = new Map([
     // Base case
-    ['#test', { default: 'test' }],
+    ['#test', maybeWrapped({ default: 'test' })],
     // import / require conditions
-    ['#branch', { default: isRequire ? 'requirebranch' : 'importbranch' }],
+    ['#branch', maybeWrapped({ default: isRequire ? 'requirebranch' : 'importbranch' })],
     // Subpath imports
-    ['#subpath/x.js', { default: 'xsubpath' }],
+    ['#subpath/x.js', maybeWrapped({ default: 'xsubpath' })],
     // External imports
-    ['#external', { default: 'asdf' }],
+    ['#external', maybeWrapped({ default: 'asdf' })],
     // External subpath imports
-    ['#external/subpath/asdf.js', { default: 'asdf' }],
+    ['#external/subpath/asdf.js', maybeWrapped({ default: 'asdf' })],
     // Trailing pattern imports
-    ['#subpath/asdf.asdf', { default: 'test' }],
+    ['#subpath/asdf.asdf', maybeWrapped({ default: 'test' })],
     // Leading slash
-    ['#subpath//asdf.asdf', { default: 'test' }],
+    ['#subpath//asdf.asdf', maybeWrapped({ default: 'test' })],
     // Double slash
-    ['#subpath/as//df.asdf', { default: 'test' }],
+    ['#subpath/as//df.asdf', maybeWrapped({ default: 'test' })],
   ]);
 
   for (const [validSpecifier, expected] of internalImports) {

test/fixtures/es-modules/cjs-exports.mjs

@@ -3,7 +3,8 @@ import { strictEqual, deepEqual } from 'assert';
 import m, { π } from './exports-cases.js';
 import * as ns from './exports-cases.js';
 
-deepEqual(Object.keys(ns), ['?invalid', 'default', 'invalid identifier', 'isObject', 'package', 'z', 'π', '\u{d83c}\u{df10}']);
+deepEqual(Object.keys(ns), ['?invalid', 'default', 'invalid identifier', 'isObject', 'module.exports', 'package', 'z', 'π', '\u{d83c}\u{df10}']);
+strictEqual(ns['module.exports'], ns.default);
 strictEqual(π, 'yes');
 strictEqual(typeof m.isObject, 'undefined');
 strictEqual(m.π, 'yes');
@@ -21,7 +22,8 @@ strictEqual(typeof m2, 'object');
 strictEqual(m2.default, 'the default');
 strictEqual(ns2.__esModule, true);
 strictEqual(ns2.name, 'name');
-deepEqual(Object.keys(ns2), ['__esModule', 'case2', 'default', 'name', 'pi']);
+strictEqual(ns2['module.exports'], ns2.default);
+deepEqual(Object.keys(ns2), ['__esModule', 'case2', 'default', 'module.exports', 'name', 'pi']);
 
 import m3, { __esModule as __esModule3, name as name3 } from './exports-cases3.js';
 import * as ns3 from './exports-cases3.js';
@@ -32,5 +34,6 @@ deepEqual(Object.keys(m3), ['name', 'default', 'pi', 'case2']);
 strictEqual(ns3.__esModule, true);
 strictEqual(ns3.name, 'name');
 strictEqual(ns3.case2, 'case2');
+strictEqual(ns3['module.exports'], ns3.default);
 
 console.log('ok');