From 1d5ed725e9b0b1d75cd5ff1559f9534287757e71 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Sun, 14 Jul 2024 13:00:49 -0700 Subject: [PATCH] esm: export 'module.exports' on ESM CJS wrapper PR-URL: https://github.com/nodejs/node/pull/53848 Reviewed-By: Matteo Collina Reviewed-By: Paolo Insogna Reviewed-By: Antoine du Hamel --- doc/api/esm.md | 41 ++++++++++++++---- lib/internal/modules/esm/translators.js | 14 ++++--- test/es-module/test-esm-exports.mjs | 53 ++++++++++++++---------- test/es-module/test-esm-imports.mjs | 19 +++++---- test/fixtures/es-modules/cjs-exports.mjs | 7 +++- 5 files changed, 87 insertions(+), 47 deletions(-) diff --git a/doc/api/esm.md b/doc/api/esm.md index 9f7f2b094e1ab2..a610bf5b4ea7be 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -474,8 +474,31 @@ See [Loading ECMAScript modules using `require()`][] for details. ### CommonJS 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] { default: , '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 diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index 9b89c3e1d52a0f..37a2b997f89214 100644 --- a/lib/internal/modules/esm/translators.js +++ b/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); } diff --git a/test/es-module/test-esm-exports.mjs b/test/es-module/test-esm-exports.mjs index 490e91a78dd21f..21ed3d383e64e9 100644 --- a/test/es-module/test-esm-exports.mjs +++ b/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; diff --git a/test/es-module/test-esm-imports.mjs b/test/es-module/test-esm-imports.mjs index 4b7e97eb8d2c5c..d005355e4ab5af 100644 --- a/test/es-module/test-esm-imports.mjs +++ b/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) { diff --git a/test/fixtures/es-modules/cjs-exports.mjs b/test/fixtures/es-modules/cjs-exports.mjs index 6001ce06364454..b89420baa0cf62 100644 --- a/test/fixtures/es-modules/cjs-exports.mjs +++ b/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');