From b45319c25983d7026c5873754fe1a2cb3066a9ab Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 26 Sep 2024 16:21:37 +0200 Subject: [PATCH 01/10] module: unflag --experimental-require-module This unflags --experimental-require-module so require(esm) can be used without the flag. For now, when require() actually encounters an ESM, it will still emit an experimental warning. To opt out of the feature, --no-experimental-require-module can be used. There are some tests specifically testing ERR_REQUIRE_ESM. Some of them are repurposed to test --no-experimental-require-module. Some of them are modified to just expect loading require(esm) to work, when it's appropriate. PR-URL: https://github.com/nodejs/node/pull/55085 Backport-PR-URL: https://github.com/nodejs/node/pull/55217 Refs: https://github.com/nodejs/node/issues/52697 Reviewed-By: Matteo Collina Reviewed-By: Marco Ippolito Reviewed-By: Rafael Gonzaga Reviewed-By: Yagiz Nizipli Reviewed-By: LiviaMedeiros Reviewed-By: Antoine du Hamel Reviewed-By: Filip Skokan Reviewed-By: Michael Dawson Reviewed-By: Richard Lau --- doc/api/cli.md | 24 +++++++++++++-- doc/api/errors.md | 23 ++++++++++----- doc/api/esm.md | 2 +- doc/api/modules.md | 29 ++++++++++--------- doc/api/packages.md | 6 ++-- lib/internal/modules/cjs/loader.js | 2 +- lib/internal/modules/esm/load.js | 10 ------- lib/internal/modules/esm/loader.js | 8 ++--- lib/internal/modules/esm/module_job.js | 1 + lib/internal/modules/esm/utils.js | 6 ++-- src/node_options.cc | 5 ++-- src/node_options.h | 2 +- test/es-module/test-cjs-esm-warn.js | 11 +++++-- test/es-module/test-esm-detect-ambiguous.mjs | 7 +++-- test/es-module/test-esm-loader-hooks.mjs | 1 + .../es-module/test-esm-type-field-errors-2.js | 17 +++++++++++ test/es-module/test-esm-type-field-errors.js | 6 ---- .../test-require-module-feature-detect.js | 5 ++-- test/es-module/test-require-module-preload.js | 7 +++-- test/es-module/test-typescript-commonjs.mjs | 2 ++ test/es-module/test-typescript-module.mjs | 13 +++++++++ test/es-module/test-typescript.mjs | 9 +++--- .../loader-with-custom-condition.mjs | 1 + test/parallel/test-require-mjs.js | 5 ++++ 24 files changed, 131 insertions(+), 71 deletions(-) create mode 100644 test/es-module/test-esm-type-field-errors-2.js diff --git a/doc/api/cli.md b/doc/api/cli.md index 62d3d37a061f78..104905d6740c3c 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -1054,6 +1054,10 @@ following permissions are restricted: > Stability: 1.1 - Active Development @@ -1695,6 +1699,22 @@ added: v16.6.0 Use this flag to disable top-level await in REPL. +### `--no-experimental-require-module` + + + +> Stability: 1.1 - Active Development + +Disable support for loading a synchronous ES module graph in `require()`. + +See [Loading ECMAScript modules using `require()`][]. + ### `--no-experimental-websocket` -This flag is only useful when `--experimental-require-module` is enabled. - -If the ES module being `require()`'d contains top-level await, this flag +If the ES module being `require()`'d contains top-level `await`, this flag allows Node.js to evaluate the module, try to locate the top-level awaits, and print their location to help users find them. diff --git a/doc/api/errors.md b/doc/api/errors.md index 11aa1b56e9cd4d..64cad66ab50e68 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -2478,8 +2478,8 @@ Opening a QUIC stream failed. > Stability: 1 - Experimental -When trying to `require()` a [ES Module][] under `--experimental-require-module`, -a CommonJS to ESM or ESM to CommonJS edge participates in an immediate cycle. +When trying to `require()` a [ES Module][], a CommonJS to ESM or ESM to CommonJS edge +participates in an immediate cycle. This is not allowed because ES Modules cannot be evaluated while they are already being evaluated. @@ -2493,8 +2493,8 @@ module, and should be done lazily in an inner function. > Stability: 1 - Experimental -When trying to `require()` a [ES Module][] under `--experimental-require-module`, -the module turns out to be asynchronous. That is, it contains top-level await. +When trying to `require()` a [ES Module][], the module turns out to be asynchronous. +That is, it contains top-level await. To see where the top-level await is, use `--experimental-print-required-tla` (this would execute the modules @@ -2504,12 +2504,20 @@ before looking for the top-level awaits). ### `ERR_REQUIRE_ESM` -> Stability: 1 - Experimental + + +> Stability: 0 - Deprecated An attempt was made to `require()` an [ES Module][]. -To enable `require()` for synchronous module graphs (without -top-level `await`), use `--experimental-require-module`. +This error has been deprecated since `require()` now supports loading synchronous +ES modules. When `require()` encounters an ES module that contains top-level +`await`, it will throw [`ERR_REQUIRE_ASYNC_MODULE`][] instead. @@ -4123,6 +4131,7 @@ An error occurred trying to allocate memory. This should never happen. [`ERR_INVALID_ARG_TYPE`]: #err_invalid_arg_type [`ERR_MISSING_MESSAGE_PORT_IN_TRANSFER_LIST`]: #err_missing_message_port_in_transfer_list [`ERR_MISSING_TRANSFERABLE_IN_TRANSFER_LIST`]: #err_missing_transferable_in_transfer_list +[`ERR_REQUIRE_ASYNC_MODULE`]: #err_require_async_module [`EventEmitter`]: events.md#class-eventemitter [`MessagePort`]: worker_threads.md#class-messageport [`Object.getPrototypeOf`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/getPrototypeOf diff --git a/doc/api/esm.md b/doc/api/esm.md index 8f2b43563f4c6b..dcb33f512add1d 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -468,7 +468,7 @@ compatibility. ### `require` The CommonJS module `require` currently only supports loading synchronous ES -modules when `--experimental-require-module` is enabled. +modules (that is, ES modules that do not use top-level `await`). See [Loading ECMAScript modules using `require()`][] for details. diff --git a/doc/api/modules.md b/doc/api/modules.md index 5005bbb85eec2d..7aa84fe15e8020 100644 --- a/doc/api/modules.md +++ b/doc/api/modules.md @@ -172,20 +172,20 @@ relative, and based on the real path of the files making the calls to > Stability: 1.1 - Active Development. Enable this API with the > [`--experimental-require-module`][] CLI flag. The `.mjs` extension is reserved for [ECMAScript Modules][]. -Currently, if the flag `--experimental-require-module` is not used, loading -an ECMAScript module using `require()` will throw a [`ERR_REQUIRE_ESM`][] -error, and users need to use [`import()`][] instead. See -[Determining module system][] section for more info +See [Determining module system][] section for more info regarding which files are parsed as ECMAScript modules. -If `--experimental-require-module` is enabled, and the ECMAScript module being -loaded by `require()` meets the following requirements: +`require()` only supports loading ECMAScript modules that meet the following requirements: * The module is fully synchronous (contains no top-level `await`); and * One of these conditions are met: @@ -194,8 +194,8 @@ loaded by `require()` meets the following requirements: 3. The file has a `.js` extension, the closest `package.json` does not contain `"type": "commonjs"`, and the module contains ES module syntax. -`require()` will load the requested module as an ES Module, and return -the module namespace object. In this case it is similar to dynamic +If the ES Module being loaded meet the requirements, `require()` can load it and +return the module namespace object. In this case it is similar to dynamic `import()` but is run synchronously and returns the name space object directly. @@ -214,7 +214,7 @@ class Point { export default Point; ``` -A CommonJS module can load them with `require()` under `--experimental-detect-module`: +A CommonJS module can load them with `require()`: ```cjs const distance = require('./distance.mjs'); @@ -243,13 +243,18 @@ conventions. Code authored directly in CommonJS should avoid depending on it. If the module being `require()`'d contains top-level `await`, or the module graph it `import`s contains top-level `await`, [`ERR_REQUIRE_ASYNC_MODULE`][] will be thrown. In this case, users should -load the asynchronous module using `import()`. +load the asynchronous module using [`import()`][]. If `--experimental-print-required-tla` is enabled, instead of throwing `ERR_REQUIRE_ASYNC_MODULE` before evaluation, Node.js will evaluate the module, try to locate the top-level awaits, and print their location to help users fix them. +Support for loading ES modules using `require()` is currently +experimental and can be disabled using `--no-experimental-require-module`. +When `require()` actually encounters an ES module for the +first time in the process, it will emit an experimental warning. The +warning is expected to be removed when this feature stablizes. This feature can be detected by checking if [`process.features.require_module`][] is `true`. @@ -282,8 +287,7 @@ require(X) from module at path Y MAYBE_DETECT_AND_LOAD(X) 1. If X parses as a CommonJS module, load X as a CommonJS module. STOP. -2. Else, if `--experimental-require-module` is - enabled, and the source code of X can be parsed as ECMAScript module using +2. Else, if the source code of X can be parsed as ECMAScript module using DETECT_MODULE_SYNTAX defined in the ESM resolver, a. Load X as an ECMAScript module. STOP. @@ -1201,7 +1205,6 @@ This section was moved to [`"type"`]: packages.md#type [`--experimental-require-module`]: cli.md#--experimental-require-module [`ERR_REQUIRE_ASYNC_MODULE`]: errors.md#err_require_async_module -[`ERR_REQUIRE_ESM`]: errors.md#err_require_esm [`ERR_UNSUPPORTED_DIR_IMPORT`]: errors.md#err_unsupported_dir_import [`MODULE_NOT_FOUND`]: errors.md#module_not_found [`__dirname`]: #__dirname diff --git a/doc/api/packages.md b/doc/api/packages.md index 5648f56df68996..13100dd65a33e7 100644 --- a/doc/api/packages.md +++ b/doc/api/packages.md @@ -172,8 +172,7 @@ There is the CommonJS module loader: * It treats all files that lack `.json` or `.node` extensions as JavaScript text files. * It can only be used to [load ECMASCript modules from CommonJS modules][] if - the module graph is synchronous (that contains no top-level `await`) when - `--experimental-require-module` is enabled. + the module graph is synchronous (that contains no top-level `await`). When used to load a JavaScript text file that is not an ECMAScript module, the file will be loaded as a CommonJS module. @@ -662,8 +661,7 @@ specific to least specific as conditions should be defined: * `"require"` - matches when the package is loaded via `require()`. The referenced file should be loadable with `require()` although the condition matches regardless of the module format of the target file. Expected - formats include CommonJS, JSON, native addons, and ES modules - if `--experimental-require-module` is enabled. _Always mutually + formats include CommonJS, JSON, native addons, and ES modules. _Always mutually exclusive with `"import"`._ * `"module-sync"` - matches no matter the package is loaded via `import`, `import()` or `require()`. The format is expected to be ES modules that does diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index b0210e4d96348c..9d3818603f4e2b 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -438,7 +438,6 @@ function initializeCJS() { Module._extensions['.ts'] = loadTS; } if (getOptionValue('--experimental-require-module')) { - emitExperimentalWarning('Support for loading ES Module in require()'); Module._extensions['.mjs'] = loadESMFromCJS; if (tsEnabled) { Module._extensions['.mts'] = loadESMFromCJS; @@ -1375,6 +1374,7 @@ function loadESMFromCJS(mod, filename) { // ESM won't be accessible via process.mainModule. setOwnProperty(process, 'mainModule', undefined); } else { + emitExperimentalWarning('Support for loading ES Module in require()'); const { wrap, namespace, diff --git a/lib/internal/modules/esm/load.js b/lib/internal/modules/esm/load.js index 8b157f0f461c7b..1932dd3c9ca369 100644 --- a/lib/internal/modules/esm/load.js +++ b/lib/internal/modules/esm/load.js @@ -131,11 +131,6 @@ async function defaultLoad(url, context = kEmptyObject) { validateAttributes(url, format, importAttributes); - // Use the synchronous commonjs translator which can deal with cycles. - if (format === 'commonjs' && getOptionValue('--experimental-require-module')) { - format = 'commonjs-sync'; - } - if (getOptionValue('--experimental-strip-types') && (format === 'module-typescript' || format === 'commonjs-typescript') && isUnderNodeModules(url)) { @@ -191,11 +186,6 @@ function defaultLoadSync(url, context = kEmptyObject) { validateAttributes(url, format, importAttributes); - // Use the synchronous commonjs translator which can deal with cycles. - if (format === 'commonjs' && getOptionValue('--experimental-require-module')) { - format = 'commonjs-sync'; - } - return { __proto__: null, format, diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 0337d0ceaa732b..262006f8b3a542 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -373,15 +373,15 @@ class ModuleLoader { defaultLoadSync ??= require('internal/modules/esm/load').defaultLoadSync; const loadResult = defaultLoadSync(url, { format, importAttributes }); - const { - format: finalFormat, - source, - } = loadResult; + + // Use the synchronous commonjs translator which can deal with cycles. + const finalFormat = loadResult.format === 'commonjs' ? 'commonjs-sync' : loadResult.format; if (finalFormat === 'wasm') { assert.fail('WASM is currently unsupported by require(esm)'); } + const { source } = loadResult; const isMain = (parentURL === undefined); const wrap = this.#translate(url, finalFormat, source, isMain); assert(wrap instanceof ModuleWrap, `Translator used for require(${url}) should not be async`); diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index 62206fcc44c2d1..413fcd27703e3e 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -361,6 +361,7 @@ class ModuleJobSync extends ModuleJobBase { } runSync() { + // TODO(joyeecheung): add the error decoration logic from the async instantiate. this.module.instantiateSync(); setHasStartedUserESMExecution(); const namespace = this.module.evaluateSync(); diff --git a/lib/internal/modules/esm/utils.js b/lib/internal/modules/esm/utils.js index 2799af0f8dd492..99061e62976e7c 100644 --- a/lib/internal/modules/esm/utils.js +++ b/lib/internal/modules/esm/utils.js @@ -75,17 +75,15 @@ function initializeDefaultConditions() { const userConditions = getOptionValue('--conditions'); const noAddons = getOptionValue('--no-addons'); const addonConditions = noAddons ? [] : ['node-addons']; - + const moduleConditions = getOptionValue('--experimental-require-module') ? ['module-sync'] : []; defaultConditions = ObjectFreeze([ 'node', 'import', + ...moduleConditions, ...addonConditions, ...userConditions, ]); defaultConditionsSet = new SafeSet(defaultConditions); - if (getOptionValue('--experimental-require-module')) { - defaultConditionsSet.add('module-sync'); - } } /** diff --git a/src/node_options.cc b/src/node_options.cc index d3b59690e917af..4bdb5f97a03ed5 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -374,9 +374,10 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() { &EnvironmentOptions::print_required_tla, kAllowedInEnvvar); AddOption("--experimental-require-module", - "Allow loading explicit ES Modules in require().", + "Allow loading synchronous ES Modules in require().", &EnvironmentOptions::require_module, - kAllowedInEnvvar); + kAllowedInEnvvar, + true); AddOption("--diagnostic-dir", "set dir for all output files" " (default: current working directory)", diff --git a/src/node_options.h b/src/node_options.h index fc7f898a6b9b60..b3246f5dcb3ab1 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -117,7 +117,7 @@ class EnvironmentOptions : public Options { std::vector conditions; bool detect_module = true; bool print_required_tla = false; - bool require_module = false; + bool require_module = true; std::string dns_result_order; bool enable_source_maps = false; bool experimental_eventsource = false; diff --git a/test/es-module/test-cjs-esm-warn.js b/test/es-module/test-cjs-esm-warn.js index ccd4d276fdd535..0945a737fb72c4 100644 --- a/test/es-module/test-cjs-esm-warn.js +++ b/test/es-module/test-cjs-esm-warn.js @@ -1,3 +1,6 @@ +// Previously, this tested that require(esm) throws ERR_REQUIRE_ESM, which is no longer applicable +// since require(esm) is now supported. The test has been repurposed to ensure that the old behavior +// is preserved when the --no-experimental-require-module flag is used. 'use strict'; const { spawnPromisified } = require('../common'); @@ -22,7 +25,9 @@ describe('CJS ↔︎ ESM interop warnings', { concurrency: !process.env.TEST_PAR fixtures.path('/es-modules/package-type-module/cjs.js') ); const basename = 'cjs.js'; - const { code, signal, stderr } = await spawnPromisified(execPath, [requiringCjsAsEsm]); + const { code, signal, stderr } = await spawnPromisified(execPath, [ + '--no-experimental-require-module', requiringCjsAsEsm, + ]); assert.ok( stderr.replaceAll('\r', '').includes( @@ -48,7 +53,9 @@ describe('CJS ↔︎ ESM interop warnings', { concurrency: !process.env.TEST_PAR fixtures.path('/es-modules/package-type-module/esm.js') ); const basename = 'esm.js'; - const { code, signal, stderr } = await spawnPromisified(execPath, [requiringEsm]); + const { code, signal, stderr } = await spawnPromisified(execPath, [ + '--no-experimental-require-module', requiringEsm, + ]); assert.ok( stderr.replace(/\r/g, '').includes( diff --git a/test/es-module/test-esm-detect-ambiguous.mjs b/test/es-module/test-esm-detect-ambiguous.mjs index 5d5a2744afcd14..8da2fea8022a63 100644 --- a/test/es-module/test-esm-detect-ambiguous.mjs +++ b/test/es-module/test-esm-detect-ambiguous.mjs @@ -402,15 +402,16 @@ describe('Module syntax detection', { concurrency: !process.env.TEST_PARALLEL }, }); }); -// Validate temporarily disabling `--abort-on-uncaught-exception` -// while running `containsModuleSyntax`. +// Checks the error caught during module detection does not trigger abort when +// `--abort-on-uncaught-exception` is passed in (as that's a caught internal error). // Ref: https://github.com/nodejs/node/issues/50878 describe('Wrapping a `require` of an ES module while using `--abort-on-uncaught-exception`', () => { it('should work', async () => { const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ '--abort-on-uncaught-exception', + '--no-warnings', '--eval', - 'assert.throws(() => require("./package-type-module/esm.js"), { code: "ERR_REQUIRE_ESM" })', + 'require("./package-type-module/esm.js")', ], { cwd: fixtures.path('es-modules'), }); diff --git a/test/es-module/test-esm-loader-hooks.mjs b/test/es-module/test-esm-loader-hooks.mjs index c697a5493c6d8b..d35f0964fe36c4 100644 --- a/test/es-module/test-esm-loader-hooks.mjs +++ b/test/es-module/test-esm-loader-hooks.mjs @@ -774,6 +774,7 @@ describe('Loader hooks', { concurrency: !process.env.TEST_PARALLEL }, () => { describe('should use hooks', async () => { const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--no-experimental-require-module', '--import', fixtures.fileURL('es-module-loaders/builtin-named-exports.mjs'), fixtures.path('es-modules/require-esm-throws-with-loaders.js'), diff --git a/test/es-module/test-esm-type-field-errors-2.js b/test/es-module/test-esm-type-field-errors-2.js new file mode 100644 index 00000000000000..3ea259446c7fb2 --- /dev/null +++ b/test/es-module/test-esm-type-field-errors-2.js @@ -0,0 +1,17 @@ +// Flags: --no-experimental-require-module +// Previously, this tested that require(esm) throws ERR_REQUIRE_ESM, which is no longer applicable +// since require(esm) is now supported. The test has been repurposed to ensure that the old behavior +// is preserved when the --no-experimental-require-module flag is used. + +'use strict'; +require('../common'); +const assert = require('assert'); +const { describe, it } = require('node:test'); + +describe('Errors related to ESM type field', () => { + it('Should throw an error when loading CJS from a `type: "module"` package.', () => { + assert.throws(() => require('../fixtures/es-modules/package-type-module/index.js'), { + code: 'ERR_REQUIRE_ESM' + }); + }); +}); diff --git a/test/es-module/test-esm-type-field-errors.js b/test/es-module/test-esm-type-field-errors.js index 9ec9aa64e18c07..4bf52f3ad6e7d3 100644 --- a/test/es-module/test-esm-type-field-errors.js +++ b/test/es-module/test-esm-type-field-errors.js @@ -50,12 +50,6 @@ describe('ESM type field errors', { concurrency: true }, () => { true, ); }); - - it('--input-type=module disallowed for directories', () => { - assert.throws(() => require('../fixtures/es-modules/package-type-module/index.js'), { - code: 'ERR_REQUIRE_ESM' - }); - }); }); function expect(opt = '', inputFile, want, wantsError = false) { diff --git a/test/es-module/test-require-module-feature-detect.js b/test/es-module/test-require-module-feature-detect.js index d6d4bb64f49f5f..8af5bc77d7b32e 100644 --- a/test/es-module/test-require-module-feature-detect.js +++ b/test/es-module/test-require-module-feature-detect.js @@ -7,7 +7,6 @@ require('../common'); const { spawnSyncAndAssert } = require('../common/child_process'); spawnSyncAndAssert(process.execPath, [ - '--no-warnings', '--experimental-require-module', '-p', 'process.features.require_module', @@ -17,13 +16,13 @@ spawnSyncAndAssert(process.execPath, [ stderr: '', // Should not emit warnings. }); -// It is not enabled by default. +// It is now enabled by default. spawnSyncAndAssert(process.execPath, [ '-p', 'process.features.require_module', ], { trim: true, - stdout: 'false', + stdout: 'true', stderr: '', // Should not emit warnings. }); diff --git a/test/es-module/test-require-module-preload.js b/test/es-module/test-require-module-preload.js index f2e572969a050c..65ec1a93bc9d4b 100644 --- a/test/es-module/test-require-module-preload.js +++ b/test/es-module/test-require-module-preload.js @@ -3,9 +3,12 @@ require('../common'); const { spawnSyncAndAssert } = require('../common/child_process'); const { fixturesDir } = require('../common/fixtures'); -const stderr = /ExperimentalWarning: Support for loading ES Module in require/; +const warningRE = /ExperimentalWarning: Support for loading ES Module in require/; function testPreload(preloadFlag) { + // The warning is only emitted when ESM is loaded by --require. + const stderr = preloadFlag !== '--import' ? warningRE : undefined; + // Test named exports. { spawnSyncAndAssert( @@ -112,7 +115,7 @@ testPreload('--import'); }, { stdout: /^package-type-module\s+A$/, - stderr, + stderr: warningRE, trim: true, } ); diff --git a/test/es-module/test-typescript-commonjs.mjs b/test/es-module/test-typescript-commonjs.mjs index 0ee687e0dcdbe6..c24576c4a0c392 100644 --- a/test/es-module/test-typescript-commonjs.mjs +++ b/test/es-module/test-typescript-commonjs.mjs @@ -100,6 +100,7 @@ test('execute a .cts file importing a .ts file export', async () => { test('execute a .cts file importing a .mts file export', async () => { const result = await spawnPromisified(process.execPath, [ '--experimental-strip-types', + '--no-experimental-require-module', fixtures.path('typescript/cts/test-require-mts-module.cts'), ]); @@ -158,6 +159,7 @@ test('expect failure of a .ts file in node_modules', async () => { test('expect failure of a .cts requiring esm without default type module', async () => { const result = await spawnPromisified(process.execPath, [ '--experimental-strip-types', + '--no-experimental-require-module', fixtures.path('typescript/cts/test-mts-node_modules.cts'), ]); diff --git a/test/es-module/test-typescript-module.mjs b/test/es-module/test-typescript-module.mjs index 7b75466e02ce9b..6b6b0e68f251f6 100644 --- a/test/es-module/test-typescript-module.mjs +++ b/test/es-module/test-typescript-module.mjs @@ -67,6 +67,7 @@ test('execute an .mts file importing a .cts file', async () => { test('execute an .mts file with wrong default module', async () => { const result = await spawnPromisified(process.execPath, [ '--experimental-strip-types', + '--no-experimental-require-module', '--experimental-default-type=commonjs', fixtures.path('typescript/mts/test-import-module.mts'), ]); @@ -76,6 +77,18 @@ test('execute an .mts file with wrong default module', async () => { strictEqual(result.code, 1); }); +test('execute an .mts file with wrong default module', async () => { + const result = await spawnPromisified(process.execPath, [ + '--experimental-strip-types', + '--experimental-require-module', + '--experimental-default-type=commonjs', + fixtures.path('typescript/mts/test-import-module.mts'), + ]); + + match(result.stdout, /Hello, TypeScript!/); + strictEqual(result.code, 0); +}); + test('execute an .mts file from node_modules', async () => { const result = await spawnPromisified(process.execPath, [ '--experimental-strip-types', diff --git a/test/es-module/test-typescript.mjs b/test/es-module/test-typescript.mjs index 00af572f2fc62f..a7ca6d70dd5e10 100644 --- a/test/es-module/test-typescript.mjs +++ b/test/es-module/test-typescript.mjs @@ -57,11 +57,11 @@ test('execute a TypeScript file with imports', async () => { const result = await spawnPromisified(process.execPath, [ '--no-warnings', '--eval', - `assert.throws(() => require(${JSON.stringify(fixtures.path('typescript/ts/test-import-fs.ts'))}), {code: 'ERR_REQUIRE_ESM'})`, + `require(${JSON.stringify(fixtures.path('typescript/ts/test-import-fs.ts'))})`, ]); strictEqual(result.stderr, ''); - strictEqual(result.stdout, ''); + match(result.stdout, /Hello, TypeScript!/); strictEqual(result.code, 0); }); @@ -308,9 +308,8 @@ test('execute a TypeScript file with CommonJS syntax requiring .mts', async () = fixtures.path('typescript/ts/test-require-mts.ts'), ]); - strictEqual(result.stdout, ''); - match(result.stderr, /Error \[ERR_REQUIRE_ESM\]: require\(\) of ES Module/); - strictEqual(result.code, 1); + match(result.stdout, /Hello, TypeScript!/); + strictEqual(result.code, 0); }); test('execute a TypeScript file with CommonJS syntax requiring .mts using require-module', async () => { diff --git a/test/fixtures/es-module-loaders/loader-with-custom-condition.mjs b/test/fixtures/es-module-loaders/loader-with-custom-condition.mjs index 3aefed51d57d3e..90c55fa576bb3c 100644 --- a/test/fixtures/es-module-loaders/loader-with-custom-condition.mjs +++ b/test/fixtures/es-module-loaders/loader-with-custom-condition.mjs @@ -5,6 +5,7 @@ export async function resolve(specifier, context, defaultResolve) { deepStrictEqual([...context.conditions].sort(), [ 'import', + 'module-sync', 'node', 'node-addons', ]); diff --git a/test/parallel/test-require-mjs.js b/test/parallel/test-require-mjs.js index 112f08879d4290..c169ec07ab6bd8 100644 --- a/test/parallel/test-require-mjs.js +++ b/test/parallel/test-require-mjs.js @@ -1,3 +1,8 @@ +// Flags: --no-experimental-require-module +// Previously, this tested that require(esm) throws ERR_REQUIRE_ESM, which is no longer applicable +// since require(esm) is now supported. The test has been repurposed to ensure that the old behavior +// is preserved when the --no-experimental-require-module flag is used. + 'use strict'; require('../common'); const assert = require('assert'); From 90d6ca69badeb89ed000dccbdbea3f4ae8459ab3 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Sun, 14 Jul 2024 13:00:49 -0700 Subject: [PATCH 02/10] module: support 'module.exports' interop export in require(esm) PR-URL: https://github.com/nodejs/node/pull/54563 Reviewed-By: Matteo Collina Reviewed-By: Antoine du Hamel Reviewed-By: James M Snell Reviewed-By: Joyee Cheung --- doc/api/modules.md | 66 ++++++++++++++++++- lib/internal/modules/cjs/loader.js | 11 ++-- .../es-module/test-require-as-esm-interop.mjs | 51 ++++++++++++++ .../node_modules/interop-cjs-esm/false-esm.js | 3 + .../node_modules/interop-cjs-esm/false.js | 3 + .../interop-cjs-esm/fauxesmdefault-esm.js | 3 + .../interop-cjs-esm/fauxesmdefault.js | 3 + .../interop-cjs-esm/fauxesmmixed-esm.js | 4 ++ .../interop-cjs-esm/fauxesmmixed.js | 3 + .../interop-cjs-esm/fauxesmnamed-esm.js | 3 + .../interop-cjs-esm/fauxesmnamed.js | 3 + .../interop-cjs-esm/object-esm.js | 3 + .../node_modules/interop-cjs-esm/object.js | 3 + .../node_modules/interop-cjs-esm/package.json | 17 +++++ .../interop-cjs-esm/string-esm.js | 3 + .../node_modules/interop-cjs-esm/string.js | 3 + .../node_modules/interop-cjs/false-esm.js | 1 + .../node_modules/interop-cjs/false.js | 1 + .../interop-cjs/fauxesmdefault-esm.js | 1 + .../interop-cjs/fauxesmdefault.js | 1 + .../interop-cjs/fauxesmmixed-esm.js | 1 + .../node_modules/interop-cjs/fauxesmmixed.js | 1 + .../interop-cjs/fauxesmnamed-esm.js | 1 + .../node_modules/interop-cjs/fauxesmnamed.js | 1 + .../node_modules/interop-cjs/object-esm.js | 1 + .../node_modules/interop-cjs/object.js | 1 + .../node_modules/interop-cjs/package.json | 17 +++++ .../node_modules/interop-cjs/string-esm.js | 1 + .../node_modules/interop-cjs/string.js | 1 + .../interop-cjsdep-false-esm/dep.js | 3 + .../interop-cjsdep-false-esm/package.json | 4 ++ .../node_modules/interop-cjsdep-false/dep.js | 1 + .../interop-cjsdep-false/package.json | 4 ++ .../interop-cjsdep-fauxesmdefault-esm/dep.js | 7 ++ .../package.json | 4 ++ .../interop-cjsdep-fauxesmdefault/dep.js | 2 + .../package.json | 4 ++ .../interop-cjsdep-fauxesmmixed-esm/dep.js | 9 +++ .../package.json | 4 ++ .../interop-cjsdep-fauxesmmixed/dep.js | 4 ++ .../interop-cjsdep-fauxesmmixed/package.json | 4 ++ .../interop-cjsdep-fauxesmnamed-esm/dep.js | 8 +++ .../package.json | 4 ++ .../interop-cjsdep-fauxesmnamed/dep.js | 3 + .../interop-cjsdep-fauxesmnamed/package.json | 4 ++ .../interop-cjsdep-object-esm/dep.js | 8 +++ .../interop-cjsdep-object-esm/package.json | 4 ++ .../node_modules/interop-cjsdep-object/dep.js | 2 + .../interop-cjsdep-object/package.json | 4 ++ .../interop-cjsdep-string-esm/dep.js | 3 + .../interop-cjsdep-string-esm/package.json | 4 ++ .../node_modules/interop-cjsdep-string/dep.js | 1 + .../interop-cjsdep-string/package.json | 4 ++ test/fixtures/pkgexports.mjs | 4 ++ 54 files changed, 308 insertions(+), 6 deletions(-) create mode 100644 test/es-module/test-require-as-esm-interop.mjs create mode 100644 test/fixtures/node_modules/interop-cjs-esm/false-esm.js create mode 100644 test/fixtures/node_modules/interop-cjs-esm/false.js create mode 100644 test/fixtures/node_modules/interop-cjs-esm/fauxesmdefault-esm.js create mode 100644 test/fixtures/node_modules/interop-cjs-esm/fauxesmdefault.js create mode 100644 test/fixtures/node_modules/interop-cjs-esm/fauxesmmixed-esm.js create mode 100644 test/fixtures/node_modules/interop-cjs-esm/fauxesmmixed.js create mode 100644 test/fixtures/node_modules/interop-cjs-esm/fauxesmnamed-esm.js create mode 100644 test/fixtures/node_modules/interop-cjs-esm/fauxesmnamed.js create mode 100644 test/fixtures/node_modules/interop-cjs-esm/object-esm.js create mode 100644 test/fixtures/node_modules/interop-cjs-esm/object.js create mode 100644 test/fixtures/node_modules/interop-cjs-esm/package.json create mode 100644 test/fixtures/node_modules/interop-cjs-esm/string-esm.js create mode 100644 test/fixtures/node_modules/interop-cjs-esm/string.js create mode 100644 test/fixtures/node_modules/interop-cjs/false-esm.js create mode 100644 test/fixtures/node_modules/interop-cjs/false.js create mode 100644 test/fixtures/node_modules/interop-cjs/fauxesmdefault-esm.js create mode 100644 test/fixtures/node_modules/interop-cjs/fauxesmdefault.js create mode 100644 test/fixtures/node_modules/interop-cjs/fauxesmmixed-esm.js create mode 100644 test/fixtures/node_modules/interop-cjs/fauxesmmixed.js create mode 100644 test/fixtures/node_modules/interop-cjs/fauxesmnamed-esm.js create mode 100644 test/fixtures/node_modules/interop-cjs/fauxesmnamed.js create mode 100644 test/fixtures/node_modules/interop-cjs/object-esm.js create mode 100644 test/fixtures/node_modules/interop-cjs/object.js create mode 100644 test/fixtures/node_modules/interop-cjs/package.json create mode 100644 test/fixtures/node_modules/interop-cjs/string-esm.js create mode 100644 test/fixtures/node_modules/interop-cjs/string.js create mode 100644 test/fixtures/node_modules/interop-cjsdep-false-esm/dep.js create mode 100644 test/fixtures/node_modules/interop-cjsdep-false-esm/package.json create mode 100644 test/fixtures/node_modules/interop-cjsdep-false/dep.js create mode 100644 test/fixtures/node_modules/interop-cjsdep-false/package.json create mode 100644 test/fixtures/node_modules/interop-cjsdep-fauxesmdefault-esm/dep.js create mode 100644 test/fixtures/node_modules/interop-cjsdep-fauxesmdefault-esm/package.json create mode 100644 test/fixtures/node_modules/interop-cjsdep-fauxesmdefault/dep.js create mode 100644 test/fixtures/node_modules/interop-cjsdep-fauxesmdefault/package.json create mode 100644 test/fixtures/node_modules/interop-cjsdep-fauxesmmixed-esm/dep.js create mode 100644 test/fixtures/node_modules/interop-cjsdep-fauxesmmixed-esm/package.json create mode 100644 test/fixtures/node_modules/interop-cjsdep-fauxesmmixed/dep.js create mode 100644 test/fixtures/node_modules/interop-cjsdep-fauxesmmixed/package.json create mode 100644 test/fixtures/node_modules/interop-cjsdep-fauxesmnamed-esm/dep.js create mode 100644 test/fixtures/node_modules/interop-cjsdep-fauxesmnamed-esm/package.json create mode 100644 test/fixtures/node_modules/interop-cjsdep-fauxesmnamed/dep.js create mode 100644 test/fixtures/node_modules/interop-cjsdep-fauxesmnamed/package.json create mode 100644 test/fixtures/node_modules/interop-cjsdep-object-esm/dep.js create mode 100644 test/fixtures/node_modules/interop-cjsdep-object-esm/package.json create mode 100644 test/fixtures/node_modules/interop-cjsdep-object/dep.js create mode 100644 test/fixtures/node_modules/interop-cjsdep-object/package.json create mode 100644 test/fixtures/node_modules/interop-cjsdep-string-esm/dep.js create mode 100644 test/fixtures/node_modules/interop-cjsdep-string-esm/package.json create mode 100644 test/fixtures/node_modules/interop-cjsdep-string/dep.js create mode 100644 test/fixtures/node_modules/interop-cjsdep-string/package.json diff --git a/doc/api/modules.md b/doc/api/modules.md index 7aa84fe15e8020..0e79d1b8d68f4d 100644 --- a/doc/api/modules.md +++ b/doc/api/modules.md @@ -176,6 +176,9 @@ changes: - version: REPLACEME pr-url: https://github.com/nodejs/node/pull/55085 description: require() now supports loading synchronous ES modules by default. + - version: REPLACEME + pr-url: https://github.com/nodejs/node/pull/54563 + description: Support `'module.exports'` interop export in `require(esm)`. --> > Stability: 1.1 - Active Development. Enable this API with the @@ -208,10 +211,9 @@ export function distance(a, b) { return (b.x - a.x) ** 2 + (b.y - a.y) ** 2; } ```mjs // point.mjs -class Point { +export default class Point { constructor(x, y) { this.x = x; this.y = y; } } -export default Point; ``` A CommonJS module can load them with `require()`: @@ -240,6 +242,66 @@ This property is experimental and can change in the future. It should only be us by tools converting ES modules into CommonJS modules, following existing ecosystem conventions. Code authored directly in CommonJS should avoid depending on it. +When a ES Module contains both named exports and a default export, the result returned by `require()` +is the module namespace object, which places the default export in the `.default` property, similar to +the results returned by `import()`. +To customize what should be returned by `require(esm)` directly, the ES Module can export the +desired value using the string name `"module.exports"`. + + + +```mjs +// point.mjs +export default class Point { + constructor(x, y) { this.x = x; this.y = y; } +} + +// `distance` is lost to CommonJS consumers of this module, unless it's +// added to `Point` as a static property. +export function distance(a, b) { return (b.x - a.x) ** 2 + (b.y - a.y) ** 2; } +export { Point as 'module.exports' } +``` + + + +```cjs +const Point = require('./point.mjs'); +console.log(Point); // [class Point] + +// Named exports are lost when 'module.exports' is used +const { distance } = require('./point.mjs'); +console.log(distance); // undefined +``` + +Notice in the example above, when the `module.exports` export name is used, named exports +will be lost to CommonJS consumers. To allow CommonJS consumers to continue accessing +named exports, the module can make sure that the default export is an object with the +named exports attached to it as properties. For example with the example above, +`distance` can be attached to the default export, the `Point` class, as a static method. + + + +```mjs +export function distance(a, b) { return (b.x - a.x) ** 2 + (b.y - a.y) ** 2; } + +export default class Point { + constructor(x, y) { this.x = x; this.y = y; } + static distance = distance; +} + +export { Point as 'module.exports' } +``` + + + +```cjs +const Point = require('./point.mjs'); +console.log(Point); // [class Point] + +const { distance } = require('./point.mjs'); +console.log(distance); // [Function: distance] +``` + If the module being `require()`'d contains top-level `await`, or the module graph it `import`s contains top-level `await`, [`ERR_REQUIRE_ASYNC_MODULE`][] will be thrown. In this case, users should diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 9d3818603f4e2b..315c7ca08a9f07 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -1416,10 +1416,13 @@ function loadESMFromCJS(mod, filename) { // createRequiredModuleFacade() to `wrap` which is a ModuleWrap wrapping // over the original module. - // We don't do this to modules that don't have default exports to avoid - // the unnecessary overhead. If __esModule is already defined, we will - // also skip the extension to allow users to override it. - if (!ObjectHasOwn(namespace, 'default') || ObjectHasOwn(namespace, '__esModule')) { + // We don't do this to modules that are marked as CJS ESM or that + // don't have default exports to avoid the unnecessary overhead. + // If __esModule is already defined, we will also skip the extension + // to allow users to override it. + if (ObjectHasOwn(namespace, 'module.exports')) { + mod.exports = namespace['module.exports']; + } else if (!ObjectHasOwn(namespace, 'default') || ObjectHasOwn(namespace, '__esModule')) { mod.exports = namespace; } else { mod.exports = createRequiredModuleFacade(wrap); diff --git a/test/es-module/test-require-as-esm-interop.mjs b/test/es-module/test-require-as-esm-interop.mjs new file mode 100644 index 00000000000000..8364fd6c2fceca --- /dev/null +++ b/test/es-module/test-require-as-esm-interop.mjs @@ -0,0 +1,51 @@ +// Flags: --experimental-require-module +import '../common/index.mjs'; +import assert from 'assert'; +import { directRequireFixture, importFixture } from '../fixtures/pkgexports.mjs'; + +const tests = { + 'false': false, + 'string': 'cjs', + 'object': { a: 'cjs a', b: 'cjs b' }, + 'fauxesmdefault': { default: 'faux esm default' }, + 'fauxesmmixed': { default: 'faux esm default', a: 'faux esm a', b: 'faux esm b' }, + 'fauxesmnamed': { a: 'faux esm a', b: 'faux esm b' } +}; + +// This test demonstrates interop between CJS and CJS represented as ESM +// under the new `export { ... as 'module.exports'}` pattern, for the above cases. +for (const [test, exactShape] of Object.entries(tests)) { + // Each case represents a CJS dependency, which has the expected shape in CJS: + assert.deepStrictEqual(directRequireFixture(`interop-cjsdep-${test}`), exactShape); + + // Each dependency is reexported through CJS as if it is a library being consumed, + // which in CJS is fully shape-preserving: + assert.deepStrictEqual(directRequireFixture(`interop-cjs/${test}`), exactShape); + + // Now we have ESM conversions of these dependencies, using `export ... as "module.exports"` + // staring with the conversion of those dependencies into ESM under require(esm): + assert.deepStrictEqual(directRequireFixture(`interop-cjsdep-${test}-esm`), exactShape); + + // When importing these ESM conversions, from require(esm), we should preserve the shape: + assert.deepStrictEqual(directRequireFixture(`interop-cjs/${test}-esm`), exactShape); + + // Now if the importer itself is converted into ESM, it should still be able to load the original + // CJS and reexport it, preserving the shape: + assert.deepStrictEqual(directRequireFixture(`interop-cjs-esm/${test}`), exactShape); + + // And then if we have the converted CJS to ESM importing from converted CJS to ESM, + // that should also work: + assert.deepStrictEqual(directRequireFixture(`interop-cjs-esm/${test}-esm`), exactShape); + + // Finally, the CJS ESM representation under `import()` should match all these cases equivalently, + // where the CJS module is exported as the default export: + const esmCjsImport = await importFixture(`interop-cjsdep-${test}`); + assert.deepStrictEqual(esmCjsImport.default, exactShape); + + assert.deepStrictEqual((await importFixture(`interop-cjsdep-${test}`)).default, exactShape); + assert.deepStrictEqual((await importFixture(`interop-cjs/${test}`)).default, exactShape); + assert.deepStrictEqual((await importFixture(`interop-cjsdep-${test}-esm`)).default, exactShape); + assert.deepStrictEqual((await importFixture(`interop-cjs/${test}-esm`)).default, exactShape); + assert.deepStrictEqual((await importFixture(`interop-cjs-esm/${test}`)).default, exactShape); + assert.deepStrictEqual((await importFixture(`interop-cjs-esm/${test}-esm`)).default, exactShape); +} diff --git a/test/fixtures/node_modules/interop-cjs-esm/false-esm.js b/test/fixtures/node_modules/interop-cjs-esm/false-esm.js new file mode 100644 index 00000000000000..44dd289dd080f4 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs-esm/false-esm.js @@ -0,0 +1,3 @@ +import dep from 'interop-cjsdep-false-esm'; +export default dep; +export { dep as 'module.exports' } diff --git a/test/fixtures/node_modules/interop-cjs-esm/false.js b/test/fixtures/node_modules/interop-cjs-esm/false.js new file mode 100644 index 00000000000000..63d896a4736bd6 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs-esm/false.js @@ -0,0 +1,3 @@ +import dep from 'interop-cjsdep-false'; +export default dep; +export { dep as 'module.exports' } diff --git a/test/fixtures/node_modules/interop-cjs-esm/fauxesmdefault-esm.js b/test/fixtures/node_modules/interop-cjs-esm/fauxesmdefault-esm.js new file mode 100644 index 00000000000000..3b87be4bd0c0bc --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs-esm/fauxesmdefault-esm.js @@ -0,0 +1,3 @@ +import dep from 'interop-cjsdep-fauxesmdefault-esm'; +export default dep; +export { dep as 'module.exports' } diff --git a/test/fixtures/node_modules/interop-cjs-esm/fauxesmdefault.js b/test/fixtures/node_modules/interop-cjs-esm/fauxesmdefault.js new file mode 100644 index 00000000000000..325fa352e16a81 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs-esm/fauxesmdefault.js @@ -0,0 +1,3 @@ +import dep from 'interop-cjsdep-fauxesmdefault'; +export default dep; +export { dep as 'module.exports' } diff --git a/test/fixtures/node_modules/interop-cjs-esm/fauxesmmixed-esm.js b/test/fixtures/node_modules/interop-cjs-esm/fauxesmmixed-esm.js new file mode 100644 index 00000000000000..9390b24f8d8bf4 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs-esm/fauxesmmixed-esm.js @@ -0,0 +1,4 @@ +import dep from 'interop-cjsdep-fauxesmmixed-esm'; +export default dep; +export { dep as 'module.exports' } + diff --git a/test/fixtures/node_modules/interop-cjs-esm/fauxesmmixed.js b/test/fixtures/node_modules/interop-cjs-esm/fauxesmmixed.js new file mode 100644 index 00000000000000..d649233736c31c --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs-esm/fauxesmmixed.js @@ -0,0 +1,3 @@ +import dep from 'interop-cjsdep-fauxesmmixed'; +export default dep; +export { dep as 'module.exports' } diff --git a/test/fixtures/node_modules/interop-cjs-esm/fauxesmnamed-esm.js b/test/fixtures/node_modules/interop-cjs-esm/fauxesmnamed-esm.js new file mode 100644 index 00000000000000..6ba7e266bc57bd --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs-esm/fauxesmnamed-esm.js @@ -0,0 +1,3 @@ +import dep from 'interop-cjsdep-fauxesmnamed-esm'; +export default dep; +export { dep as 'module.exports' } diff --git a/test/fixtures/node_modules/interop-cjs-esm/fauxesmnamed.js b/test/fixtures/node_modules/interop-cjs-esm/fauxesmnamed.js new file mode 100644 index 00000000000000..7c6c6bfc14adcd --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs-esm/fauxesmnamed.js @@ -0,0 +1,3 @@ +import dep from 'interop-cjsdep-fauxesmnamed'; +export default dep; +export { dep as 'module.exports' } diff --git a/test/fixtures/node_modules/interop-cjs-esm/object-esm.js b/test/fixtures/node_modules/interop-cjs-esm/object-esm.js new file mode 100644 index 00000000000000..0a4e4283625e5b --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs-esm/object-esm.js @@ -0,0 +1,3 @@ +import dep from 'interop-cjsdep-object-esm'; +export default dep; +export { dep as 'module.exports' } diff --git a/test/fixtures/node_modules/interop-cjs-esm/object.js b/test/fixtures/node_modules/interop-cjs-esm/object.js new file mode 100644 index 00000000000000..08ef3ba4f96f3d --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs-esm/object.js @@ -0,0 +1,3 @@ +import dep from 'interop-cjsdep-object'; +export default dep; +export { dep as 'module.exports' } diff --git a/test/fixtures/node_modules/interop-cjs-esm/package.json b/test/fixtures/node_modules/interop-cjs-esm/package.json new file mode 100644 index 00000000000000..95315e635e7c30 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs-esm/package.json @@ -0,0 +1,17 @@ +{ + "type": "module", + "exports": { + "./false-esm": "./false-esm.js", + "./false": "./false.js", + "./fauxesmdefault-esm": "./fauxesmdefault-esm.js", + "./fauxesmdefault": "./fauxesmdefault.js", + "./fauxesmmixed-esm": "./fauxesmmixed-esm.js", + "./fauxesmmixed": "./fauxesmmixed.js", + "./fauxesmnamed-esm": "./fauxesmnamed-esm.js", + "./fauxesmnamed": "./fauxesmnamed.js", + "./object-esm": "./object-esm.js", + "./object": "./object.js", + "./string-esm": "./string-esm.js", + "./string": "./string.js" + } +} diff --git a/test/fixtures/node_modules/interop-cjs-esm/string-esm.js b/test/fixtures/node_modules/interop-cjs-esm/string-esm.js new file mode 100644 index 00000000000000..6072f01bc95eed --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs-esm/string-esm.js @@ -0,0 +1,3 @@ +import dep from 'interop-cjsdep-string-esm'; +export default dep; +export { dep as 'module.exports' } diff --git a/test/fixtures/node_modules/interop-cjs-esm/string.js b/test/fixtures/node_modules/interop-cjs-esm/string.js new file mode 100644 index 00000000000000..74297e8eee8d39 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs-esm/string.js @@ -0,0 +1,3 @@ +import dep from 'interop-cjsdep-string'; +export default dep; +export { dep as 'module.exports' } diff --git a/test/fixtures/node_modules/interop-cjs/false-esm.js b/test/fixtures/node_modules/interop-cjs/false-esm.js new file mode 100644 index 00000000000000..eeb934952b51de --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs/false-esm.js @@ -0,0 +1 @@ +module.exports = require('interop-cjsdep-false-esm'); diff --git a/test/fixtures/node_modules/interop-cjs/false.js b/test/fixtures/node_modules/interop-cjs/false.js new file mode 100644 index 00000000000000..459ccbeee45944 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs/false.js @@ -0,0 +1 @@ +module.exports = require('interop-cjsdep-false'); diff --git a/test/fixtures/node_modules/interop-cjs/fauxesmdefault-esm.js b/test/fixtures/node_modules/interop-cjs/fauxesmdefault-esm.js new file mode 100644 index 00000000000000..45e91e3c3c80da --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs/fauxesmdefault-esm.js @@ -0,0 +1 @@ +module.exports = require('interop-cjsdep-fauxesmdefault-esm'); diff --git a/test/fixtures/node_modules/interop-cjs/fauxesmdefault.js b/test/fixtures/node_modules/interop-cjs/fauxesmdefault.js new file mode 100644 index 00000000000000..c6e5176f528416 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs/fauxesmdefault.js @@ -0,0 +1 @@ +module.exports = require('interop-cjsdep-fauxesmdefault'); diff --git a/test/fixtures/node_modules/interop-cjs/fauxesmmixed-esm.js b/test/fixtures/node_modules/interop-cjs/fauxesmmixed-esm.js new file mode 100644 index 00000000000000..6b16d657b0e42d --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs/fauxesmmixed-esm.js @@ -0,0 +1 @@ +module.exports = require('interop-cjsdep-fauxesmmixed-esm'); diff --git a/test/fixtures/node_modules/interop-cjs/fauxesmmixed.js b/test/fixtures/node_modules/interop-cjs/fauxesmmixed.js new file mode 100644 index 00000000000000..8897d00fb3e563 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs/fauxesmmixed.js @@ -0,0 +1 @@ +module.exports = require('interop-cjsdep-fauxesmmixed'); diff --git a/test/fixtures/node_modules/interop-cjs/fauxesmnamed-esm.js b/test/fixtures/node_modules/interop-cjs/fauxesmnamed-esm.js new file mode 100644 index 00000000000000..67ed05bfa7f6d4 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs/fauxesmnamed-esm.js @@ -0,0 +1 @@ +module.exports = require('interop-cjsdep-fauxesmnamed-esm'); diff --git a/test/fixtures/node_modules/interop-cjs/fauxesmnamed.js b/test/fixtures/node_modules/interop-cjs/fauxesmnamed.js new file mode 100644 index 00000000000000..092ec333260157 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs/fauxesmnamed.js @@ -0,0 +1 @@ +module.exports = require('interop-cjsdep-fauxesmnamed'); diff --git a/test/fixtures/node_modules/interop-cjs/object-esm.js b/test/fixtures/node_modules/interop-cjs/object-esm.js new file mode 100644 index 00000000000000..f0e0d3fa3dad07 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs/object-esm.js @@ -0,0 +1 @@ +module.exports = require('interop-cjsdep-object-esm'); diff --git a/test/fixtures/node_modules/interop-cjs/object.js b/test/fixtures/node_modules/interop-cjs/object.js new file mode 100644 index 00000000000000..ff2d57e11b824f --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs/object.js @@ -0,0 +1 @@ +module.exports = require('interop-cjsdep-object'); diff --git a/test/fixtures/node_modules/interop-cjs/package.json b/test/fixtures/node_modules/interop-cjs/package.json new file mode 100644 index 00000000000000..9e8c0b3f59712c --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs/package.json @@ -0,0 +1,17 @@ +{ + "type": "commonjs", + "exports": { + "./false-esm": "./false-esm.js", + "./false": "./false.js", + "./fauxesmdefault-esm": "./fauxesmdefault-esm.js", + "./fauxesmdefault": "./fauxesmdefault.js", + "./fauxesmmixed-esm": "./fauxesmmixed-esm.js", + "./fauxesmmixed": "./fauxesmmixed.js", + "./fauxesmnamed-esm": "./fauxesmnamed-esm.js", + "./fauxesmnamed": "./fauxesmnamed.js", + "./object-esm": "./object-esm.js", + "./object": "./object.js", + "./string-esm": "./string-esm.js", + "./string": "./string.js" + } +} diff --git a/test/fixtures/node_modules/interop-cjs/string-esm.js b/test/fixtures/node_modules/interop-cjs/string-esm.js new file mode 100644 index 00000000000000..bdd23a1b1db4da --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs/string-esm.js @@ -0,0 +1 @@ +module.exports = require('interop-cjsdep-string-esm'); diff --git a/test/fixtures/node_modules/interop-cjs/string.js b/test/fixtures/node_modules/interop-cjs/string.js new file mode 100644 index 00000000000000..65010183f2f76b --- /dev/null +++ b/test/fixtures/node_modules/interop-cjs/string.js @@ -0,0 +1 @@ +module.exports = require('interop-cjsdep-string'); diff --git a/test/fixtures/node_modules/interop-cjsdep-false-esm/dep.js b/test/fixtures/node_modules/interop-cjsdep-false-esm/dep.js new file mode 100644 index 00000000000000..7ef62281a771cb --- /dev/null +++ b/test/fixtures/node_modules/interop-cjsdep-false-esm/dep.js @@ -0,0 +1,3 @@ +const output = false; +export default output; +export { output as 'module.exports' } diff --git a/test/fixtures/node_modules/interop-cjsdep-false-esm/package.json b/test/fixtures/node_modules/interop-cjsdep-false-esm/package.json new file mode 100644 index 00000000000000..f5b94a85497063 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjsdep-false-esm/package.json @@ -0,0 +1,4 @@ +{ + "type": "module", + "exports": "./dep.js" +} diff --git a/test/fixtures/node_modules/interop-cjsdep-false/dep.js b/test/fixtures/node_modules/interop-cjsdep-false/dep.js new file mode 100644 index 00000000000000..a5d30209bd0bbc --- /dev/null +++ b/test/fixtures/node_modules/interop-cjsdep-false/dep.js @@ -0,0 +1 @@ +module.exports = false; diff --git a/test/fixtures/node_modules/interop-cjsdep-false/package.json b/test/fixtures/node_modules/interop-cjsdep-false/package.json new file mode 100644 index 00000000000000..e6fc9863573e3d --- /dev/null +++ b/test/fixtures/node_modules/interop-cjsdep-false/package.json @@ -0,0 +1,4 @@ +{ + "type": "commonjs", + "exports": "./dep.js" +} diff --git a/test/fixtures/node_modules/interop-cjsdep-fauxesmdefault-esm/dep.js b/test/fixtures/node_modules/interop-cjsdep-fauxesmdefault-esm/dep.js new file mode 100644 index 00000000000000..731bf61547a427 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjsdep-fauxesmdefault-esm/dep.js @@ -0,0 +1,7 @@ +const exports = {}; +Object.defineProperty(exports, '__esModule', { value: true }); +exports.default = 'faux esm default'; + +export default exports; + +export { exports as 'module.exports' } \ No newline at end of file diff --git a/test/fixtures/node_modules/interop-cjsdep-fauxesmdefault-esm/package.json b/test/fixtures/node_modules/interop-cjsdep-fauxesmdefault-esm/package.json new file mode 100644 index 00000000000000..f5b94a85497063 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjsdep-fauxesmdefault-esm/package.json @@ -0,0 +1,4 @@ +{ + "type": "module", + "exports": "./dep.js" +} diff --git a/test/fixtures/node_modules/interop-cjsdep-fauxesmdefault/dep.js b/test/fixtures/node_modules/interop-cjsdep-fauxesmdefault/dep.js new file mode 100644 index 00000000000000..5cd0b37b27d054 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjsdep-fauxesmdefault/dep.js @@ -0,0 +1,2 @@ +Object.defineProperty(exports, '__esModule', { value: true }); +exports.default = 'faux esm default'; diff --git a/test/fixtures/node_modules/interop-cjsdep-fauxesmdefault/package.json b/test/fixtures/node_modules/interop-cjsdep-fauxesmdefault/package.json new file mode 100644 index 00000000000000..e6fc9863573e3d --- /dev/null +++ b/test/fixtures/node_modules/interop-cjsdep-fauxesmdefault/package.json @@ -0,0 +1,4 @@ +{ + "type": "commonjs", + "exports": "./dep.js" +} diff --git a/test/fixtures/node_modules/interop-cjsdep-fauxesmmixed-esm/dep.js b/test/fixtures/node_modules/interop-cjsdep-fauxesmmixed-esm/dep.js new file mode 100644 index 00000000000000..0eef418de77ea7 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjsdep-fauxesmmixed-esm/dep.js @@ -0,0 +1,9 @@ +const exports = {}; +Object.defineProperty(exports, '__esModule', { value: true }); +exports.default = 'faux esm default'; +exports.a = 'faux esm a'; +exports.b = 'faux esm b'; + +export default exports; + +export { exports as 'module.exports' } diff --git a/test/fixtures/node_modules/interop-cjsdep-fauxesmmixed-esm/package.json b/test/fixtures/node_modules/interop-cjsdep-fauxesmmixed-esm/package.json new file mode 100644 index 00000000000000..f5b94a85497063 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjsdep-fauxesmmixed-esm/package.json @@ -0,0 +1,4 @@ +{ + "type": "module", + "exports": "./dep.js" +} diff --git a/test/fixtures/node_modules/interop-cjsdep-fauxesmmixed/dep.js b/test/fixtures/node_modules/interop-cjsdep-fauxesmmixed/dep.js new file mode 100644 index 00000000000000..926ce033f829a0 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjsdep-fauxesmmixed/dep.js @@ -0,0 +1,4 @@ +Object.defineProperty(exports, '__esModule', { value: true }); +exports.default = 'faux esm default'; +exports.a = 'faux esm a'; +exports.b = 'faux esm b'; diff --git a/test/fixtures/node_modules/interop-cjsdep-fauxesmmixed/package.json b/test/fixtures/node_modules/interop-cjsdep-fauxesmmixed/package.json new file mode 100644 index 00000000000000..e6fc9863573e3d --- /dev/null +++ b/test/fixtures/node_modules/interop-cjsdep-fauxesmmixed/package.json @@ -0,0 +1,4 @@ +{ + "type": "commonjs", + "exports": "./dep.js" +} diff --git a/test/fixtures/node_modules/interop-cjsdep-fauxesmnamed-esm/dep.js b/test/fixtures/node_modules/interop-cjsdep-fauxesmnamed-esm/dep.js new file mode 100644 index 00000000000000..ac048c7b1634a7 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjsdep-fauxesmnamed-esm/dep.js @@ -0,0 +1,8 @@ +const exports = {}; +Object.defineProperty(exports, '__esModule', { value: true }); +exports.a = 'faux esm a'; +exports.b = 'faux esm b'; + +export default exports; + +export { exports as 'module.exports' } diff --git a/test/fixtures/node_modules/interop-cjsdep-fauxesmnamed-esm/package.json b/test/fixtures/node_modules/interop-cjsdep-fauxesmnamed-esm/package.json new file mode 100644 index 00000000000000..f5b94a85497063 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjsdep-fauxesmnamed-esm/package.json @@ -0,0 +1,4 @@ +{ + "type": "module", + "exports": "./dep.js" +} diff --git a/test/fixtures/node_modules/interop-cjsdep-fauxesmnamed/dep.js b/test/fixtures/node_modules/interop-cjsdep-fauxesmnamed/dep.js new file mode 100644 index 00000000000000..e3218c09b35752 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjsdep-fauxesmnamed/dep.js @@ -0,0 +1,3 @@ +Object.defineProperty(exports, '__esModule', { value: true }); +exports.a = 'faux esm a'; +exports.b = 'faux esm b'; diff --git a/test/fixtures/node_modules/interop-cjsdep-fauxesmnamed/package.json b/test/fixtures/node_modules/interop-cjsdep-fauxesmnamed/package.json new file mode 100644 index 00000000000000..e6fc9863573e3d --- /dev/null +++ b/test/fixtures/node_modules/interop-cjsdep-fauxesmnamed/package.json @@ -0,0 +1,4 @@ +{ + "type": "commonjs", + "exports": "./dep.js" +} diff --git a/test/fixtures/node_modules/interop-cjsdep-object-esm/dep.js b/test/fixtures/node_modules/interop-cjsdep-object-esm/dep.js new file mode 100644 index 00000000000000..f53cd1c055bd0a --- /dev/null +++ b/test/fixtures/node_modules/interop-cjsdep-object-esm/dep.js @@ -0,0 +1,8 @@ +const output = { + a: 'cjs a', + b: 'cjs b' +}; + +export default output; + +export { output as 'module.exports' } diff --git a/test/fixtures/node_modules/interop-cjsdep-object-esm/package.json b/test/fixtures/node_modules/interop-cjsdep-object-esm/package.json new file mode 100644 index 00000000000000..f5b94a85497063 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjsdep-object-esm/package.json @@ -0,0 +1,4 @@ +{ + "type": "module", + "exports": "./dep.js" +} diff --git a/test/fixtures/node_modules/interop-cjsdep-object/dep.js b/test/fixtures/node_modules/interop-cjsdep-object/dep.js new file mode 100644 index 00000000000000..c37089bccc741e --- /dev/null +++ b/test/fixtures/node_modules/interop-cjsdep-object/dep.js @@ -0,0 +1,2 @@ +exports.a = 'cjs a'; +exports.b = 'cjs b'; diff --git a/test/fixtures/node_modules/interop-cjsdep-object/package.json b/test/fixtures/node_modules/interop-cjsdep-object/package.json new file mode 100644 index 00000000000000..e6fc9863573e3d --- /dev/null +++ b/test/fixtures/node_modules/interop-cjsdep-object/package.json @@ -0,0 +1,4 @@ +{ + "type": "commonjs", + "exports": "./dep.js" +} diff --git a/test/fixtures/node_modules/interop-cjsdep-string-esm/dep.js b/test/fixtures/node_modules/interop-cjsdep-string-esm/dep.js new file mode 100644 index 00000000000000..8b718c32a1fab5 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjsdep-string-esm/dep.js @@ -0,0 +1,3 @@ +const output = 'cjs'; +export default output; +export { output as 'module.exports' } diff --git a/test/fixtures/node_modules/interop-cjsdep-string-esm/package.json b/test/fixtures/node_modules/interop-cjsdep-string-esm/package.json new file mode 100644 index 00000000000000..f5b94a85497063 --- /dev/null +++ b/test/fixtures/node_modules/interop-cjsdep-string-esm/package.json @@ -0,0 +1,4 @@ +{ + "type": "module", + "exports": "./dep.js" +} diff --git a/test/fixtures/node_modules/interop-cjsdep-string/dep.js b/test/fixtures/node_modules/interop-cjsdep-string/dep.js new file mode 100644 index 00000000000000..b2825bd3c9949b --- /dev/null +++ b/test/fixtures/node_modules/interop-cjsdep-string/dep.js @@ -0,0 +1 @@ +module.exports = 'cjs'; diff --git a/test/fixtures/node_modules/interop-cjsdep-string/package.json b/test/fixtures/node_modules/interop-cjsdep-string/package.json new file mode 100644 index 00000000000000..e6fc9863573e3d --- /dev/null +++ b/test/fixtures/node_modules/interop-cjsdep-string/package.json @@ -0,0 +1,4 @@ +{ + "type": "commonjs", + "exports": "./dep.js" +} diff --git a/test/fixtures/pkgexports.mjs b/test/fixtures/pkgexports.mjs index 7d642c443e6b71..b71566b95c79ac 100644 --- a/test/fixtures/pkgexports.mjs +++ b/test/fixtures/pkgexports.mjs @@ -3,6 +3,10 @@ import { createRequire } from 'module'; const rawRequire = createRequire(fileURLToPath(import.meta.url)); +export function directRequireFixture(specifier) { + return rawRequire(specifier); +} + export async function requireFixture(specifier) { return { default: rawRequire(specifier ) }; } From 1578ca2d2a84597cb7cab89972ae6cf7785573fe Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 2 Oct 2024 23:02:31 +0200 Subject: [PATCH 03/10] doc: update `require(ESM)` history and stability status PR-URL: https://github.com/nodejs/node/pull/55199 Reviewed-By: Moshe Atlow Reviewed-By: Guy Bedford Reviewed-By: Joyee Cheung --- doc/api/modules.md | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/doc/api/modules.md b/doc/api/modules.md index 0e79d1b8d68f4d..1f55fefd793075 100644 --- a/doc/api/modules.md +++ b/doc/api/modules.md @@ -171,18 +171,18 @@ relative, and based on the real path of the files making the calls to ## Loading ECMAScript modules using `require()` -> Stability: 1.1 - Active Development. Enable this API with the -> [`--experimental-require-module`][] CLI flag. +> Stability: 1.2 - Release candidate The `.mjs` extension is reserved for [ECMAScript Modules][]. See [Determining module system][] section for more info @@ -1265,7 +1265,6 @@ This section was moved to [GLOBAL_FOLDERS]: #loading-from-the-global-folders [`"main"`]: packages.md#main [`"type"`]: packages.md#type -[`--experimental-require-module`]: cli.md#--experimental-require-module [`ERR_REQUIRE_ASYNC_MODULE`]: errors.md#err_require_async_module [`ERR_UNSUPPORTED_DIR_IMPORT`]: errors.md#err_unsupported_dir_import [`MODULE_NOT_FOUND`]: errors.md#module_not_found From dde6304d0ca0ae341e256650aec6af5ae3dc6ed4 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 4 Oct 2024 19:29:34 +0200 Subject: [PATCH 04/10] module: use kNodeModulesRE to detect node_modules This is faster and more consistent with other places using the regular expression to detect node_modules. PR-URL: https://github.com/nodejs/node/pull/55243 Backport-PR-URL: https://github.com/nodejs/node/pull/55217 Reviewed-By: Antoine du Hamel Reviewed-By: Jacob Smith Reviewed-By: Richard Lau Reviewed-By: Marco Ippolito Refs: https://github.com/nodejs/node/issues/52697 --- lib/internal/modules/cjs/loader.js | 2 +- lib/internal/modules/esm/load.js | 8 ++++---- lib/internal/modules/helpers.js | 9 --------- lib/internal/util.js | 7 ++++++- 4 files changed, 11 insertions(+), 15 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 315c7ca08a9f07..b10a8cceb1cf0f 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -124,6 +124,7 @@ const { pathToFileURL, fileURLToPath, isURL } = require('internal/url'); const { pendingDeprecate, emitExperimentalWarning, + isUnderNodeModules, kEmptyObject, setOwnProperty, getLazy, @@ -146,7 +147,6 @@ const { safeGetenv } = internalBinding('credentials'); const { getCjsConditions, initializeCjsConditions, - isUnderNodeModules, loadBuiltinModule, makeRequireFunction, setHasStartedUserCJSExecution, diff --git a/lib/internal/modules/esm/load.js b/lib/internal/modules/esm/load.js index 1932dd3c9ca369..d56dae3f001b1c 100644 --- a/lib/internal/modules/esm/load.js +++ b/lib/internal/modules/esm/load.js @@ -3,7 +3,10 @@ const { RegExpPrototypeExec, } = primordials; -const { kEmptyObject } = require('internal/util'); +const { + isUnderNodeModules, + kEmptyObject, +} = require('internal/util'); const { defaultGetFormat } = require('internal/modules/esm/get_format'); const { validateAttributes, emitImportAssertionWarning } = require('internal/modules/esm/assert'); @@ -14,9 +17,6 @@ const defaultType = getOptionValue('--experimental-default-type'); const { Buffer: { from: BufferFrom } } = require('buffer'); -const { - isUnderNodeModules, -} = require('internal/modules/helpers'); const { URL } = require('internal/url'); const { diff --git a/lib/internal/modules/helpers.js b/lib/internal/modules/helpers.js index 1add21bb541cf4..11adec360cc98c 100644 --- a/lib/internal/modules/helpers.js +++ b/lib/internal/modules/helpers.js @@ -2,7 +2,6 @@ const { ArrayPrototypeForEach, - ArrayPrototypeIncludes, ObjectDefineProperty, ObjectFreeze, ObjectPrototypeHasOwnProperty, @@ -11,7 +10,6 @@ const { StringPrototypeCharCodeAt, StringPrototypeIncludes, StringPrototypeSlice, - StringPrototypeSplit, StringPrototypeStartsWith, } = primordials; const { @@ -380,12 +378,6 @@ function stripTypeScriptTypes(source, filename) { return `${code}\n\n//# sourceURL=${filename}`; } -function isUnderNodeModules(filename) { - const resolvedPath = path.resolve(filename); - const normalizedPath = path.normalize(resolvedPath); - const splitPath = StringPrototypeSplit(normalizedPath, path.sep); - return ArrayPrototypeIncludes(splitPath, 'node_modules'); -} /** * Enable on-disk compiled cache for all user modules being complied in the current Node.js instance @@ -486,7 +478,6 @@ module.exports = { getCjsConditions, getCompileCacheDir, initializeCjsConditions, - isUnderNodeModules, loadBuiltinModule, makeRequireFunction, normalizeReferrerURL, diff --git a/lib/internal/util.js b/lib/internal/util.js index 0973d4efdf2257..8cba1c90ab8189 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -481,6 +481,10 @@ function spliceOne(list, index) { const kNodeModulesRE = /^(?:.*)[\\/]node_modules[\\/]/; +function isUnderNodeModules(filename) { + return filename && (RegExpPrototypeExec(kNodeModulesRE, filename) !== null); +} + let getStructuredStackImpl; function lazyGetStructuredStack() { @@ -528,7 +532,7 @@ function isInsideNodeModules() { ) { continue; } - return RegExpPrototypeExec(kNodeModulesRE, filename) !== null; + return isUnderNodeModules(filename); } } return false; @@ -908,6 +912,7 @@ module.exports = { guessHandleType, isError, isInsideNodeModules, + isUnderNodeModules, isMacOS, isWindows, join, From 6987bb25e1aba7eb6a2891e5f53be4df94912679 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 8 Oct 2024 22:26:13 +0200 Subject: [PATCH 05/10] module: check --experimental-require-module separately from detection Previously we assumed if `--experimental-detect-module` is true, then `--experimental-require-module` is true, which isn't the case, as the two can be enabled/disabled separately. This patch fixes the checks so `--no-experimental-require-module` is still effective when `--experimental-detect-module` is enabled. Drive-by: make the assertion messages more informative and remove obsolete TODO about allowing TLA in entrypoints handled by require(esm). PR-URL: https://github.com/nodejs/node/pull/55250 Reviewed-By: Antoine du Hamel Reviewed-By: Jacob Smith Reviewed-By: Rafael Gonzaga --- lib/internal/modules/cjs/loader.js | 16 ++++++++++++---- ...t-disable-require-module-with-detection.js | 19 +++++++++++++++++++ test/fixtures/es-modules/loose.js | 4 +++- 3 files changed, 34 insertions(+), 5 deletions(-) create mode 100644 test/es-module/test-disable-require-module-with-detection.js diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index b10a8cceb1cf0f..310e9a3a630efd 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -1438,7 +1438,7 @@ function loadESMFromCJS(mod, filename) { * @param {'commonjs'|undefined} format Intended format of the module. */ function wrapSafe(filename, content, cjsModuleInstance, format) { - assert(format !== 'module'); // ESM should be handled in loadESMFromCJS(). + assert(format !== 'module', 'ESM should be handled in loadESMFromCJS()'); const hostDefinedOptionId = vm_dynamic_import_default_internal; const importModuleDynamically = vm_dynamic_import_default_internal; if (patched) { @@ -1468,7 +1468,17 @@ function wrapSafe(filename, content, cjsModuleInstance, format) { }; } - const shouldDetectModule = (format !== 'commonjs' && getOptionValue('--experimental-detect-module')); + let shouldDetectModule = false; + if (format !== 'commonjs') { + if (cjsModuleInstance?.[kIsMainSymbol]) { + // For entry points, format detection is used unless explicitly disabled. + shouldDetectModule = getOptionValue('--experimental-detect-module'); + } else { + // For modules being loaded by `require()`, if require(esm) is disabled, + // don't try to reparse to detect format and just throw for ESM syntax. + shouldDetectModule = getOptionValue('--experimental-require-module'); + } + } const result = compileFunctionForCJSLoader(content, filename, false /* is_sea_main */, shouldDetectModule); // Cache the source map for the module if present. @@ -1498,8 +1508,6 @@ Module.prototype._compile = function(content, filename, format) { } } - // TODO(joyeecheung): when the module is the entry point, consider allowing TLA. - // Only modules being require()'d really need to avoid TLA. if (format === 'module') { // Pass the source into the .mjs extension handler indirectly through the cache. this[kModuleSource] = content; diff --git a/test/es-module/test-disable-require-module-with-detection.js b/test/es-module/test-disable-require-module-with-detection.js new file mode 100644 index 00000000000000..3ac5ec82171587 --- /dev/null +++ b/test/es-module/test-disable-require-module-with-detection.js @@ -0,0 +1,19 @@ +// Flags: --no-experimental-require-module +'use strict'; + +// Tests that --experimental-require-module is not implied by --experimental-detect-module +// and is checked independently. +require('../common'); +const assert = require('assert'); + +// Check that require() still throws SyntaxError for an ambiguous module that's detected to be ESM. +// TODO(joyeecheung): now that --experimental-detect-module is unflagged, it makes more sense +// to either throw ERR_REQUIRE_ESM for require() of detected ESM instead, or add a hint about the +// use of require(esm) to the SyntaxError. + +assert.throws( + () => require('../fixtures/es-modules/loose.js'), + { + name: 'SyntaxError', + message: /Unexpected token 'export'/ + }); diff --git a/test/fixtures/es-modules/loose.js b/test/fixtures/es-modules/loose.js index 69147a3b8ca027..c0d85f7eab9a2b 100644 --- a/test/fixtures/es-modules/loose.js +++ b/test/fixtures/es-modules/loose.js @@ -1,3 +1,5 @@ -// This file can be run or imported only if `--experimental-default-type=module` is set. +// This file can be run or imported only if `--experimental-default-type=module` is set +// or `--experimental-detect-module` is not disabled. If it's loaded by +// require(), then `--experimental-require-module` must not be disabled. export default 'module'; console.log('executed'); From 08323c270578314719e7b3c63c37e5426ae3cad0 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 8 Oct 2024 12:19:46 +0200 Subject: [PATCH 06/10] src: implement IsInsideNodeModules() in C++ This previously compiles a script and run it in a new context to avoid global pollution, which is more complex than necessary and can be too slow for it to be reused in other cases. The new implementation just checks the frames in C++ which is safe from global pollution, faster and simpler. The previous implementation also had a bug when the call site is in a ESM, because ESM have URLs as their script names, which don't start with '/' or '\' and will be skipped. The new implementation removes the skipping to fix it for ESM. PR-URL: https://github.com/nodejs/node/pull/55286 Reviewed-By: Yagiz Nizipli Reviewed-By: Chengzhong Wu --- lib/buffer.js | 6 ++- lib/internal/util.js | 29 ----------- src/node_util.cc | 44 +++++++++++++++++ .../warning_node_modules/new-buffer-cjs.js | 1 + .../warning_node_modules/new-buffer-esm.mjs | 1 + .../node_modules/new-buffer-cjs/index.js | 1 + .../node_modules/new-buffer-cjs/package.json | 3 ++ .../node_modules/new-buffer-esm/index.js | 2 + .../node_modules/new-buffer-esm/package.json | 4 ++ .../test-buffer-constructor-node-modules.js | 48 +++++++++++++++++++ 10 files changed, 108 insertions(+), 31 deletions(-) create mode 100644 test/fixtures/warning_node_modules/new-buffer-cjs.js create mode 100644 test/fixtures/warning_node_modules/new-buffer-esm.mjs create mode 100644 test/fixtures/warning_node_modules/node_modules/new-buffer-cjs/index.js create mode 100644 test/fixtures/warning_node_modules/node_modules/new-buffer-cjs/package.json create mode 100644 test/fixtures/warning_node_modules/node_modules/new-buffer-esm/index.js create mode 100644 test/fixtures/warning_node_modules/node_modules/new-buffer-esm/package.json create mode 100644 test/parallel/test-buffer-constructor-node-modules.js diff --git a/lib/buffer.js b/lib/buffer.js index 6294eae2fb5093..2c56b6c504db9b 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -79,10 +79,10 @@ const { ONLY_ENUMERABLE, }, getOwnNonIndexProperties, + isInsideNodeModules, } = internalBinding('util'); const { customInspectSymbol, - isInsideNodeModules, lazyDOMException, normalizeEncoding, kIsEncodingSymbol, @@ -178,13 +178,15 @@ function showFlaggedDeprecation() { if (bufferWarningAlreadyEmitted || ++nodeModulesCheckCounter > 10000 || (!require('internal/options').getOptionValue('--pending-deprecation') && - isInsideNodeModules())) { + isInsideNodeModules(100, true))) { // We don't emit a warning, because we either: // - Already did so, or // - Already checked too many times whether a call is coming // from node_modules and want to stop slowing down things, or // - We aren't running with `--pending-deprecation` enabled, // and the code is inside `node_modules`. + // - We found node_modules in up to the topmost 100 frames, or + // there are more than 100 frames and we don't want to search anymore. return; } diff --git a/lib/internal/util.js b/lib/internal/util.js index 8cba1c90ab8189..a3ffbbdc6b350f 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -2,7 +2,6 @@ const { ArrayFrom, - ArrayIsArray, ArrayPrototypePush, ArrayPrototypeSlice, ArrayPrototypeSort, @@ -34,9 +33,7 @@ const { SafeSet, SafeWeakMap, SafeWeakRef, - StringPrototypeIncludes, StringPrototypeReplace, - StringPrototypeStartsWith, StringPrototypeToLowerCase, StringPrototypeToUpperCase, Symbol, @@ -513,31 +510,6 @@ function getStructuredStack() { return getStructuredStackImpl(); } -function isInsideNodeModules() { - const stack = getStructuredStack(); - - // Iterate over all stack frames and look for the first one not coming - // from inside Node.js itself: - if (ArrayIsArray(stack)) { - for (const frame of stack) { - const filename = frame.getFileName(); - - if ( - filename == null || - StringPrototypeStartsWith(filename, 'node:') === true || - ( - filename[0] !== '/' && - StringPrototypeIncludes(filename, '\\') === false - ) - ) { - continue; - } - return isUnderNodeModules(filename); - } - } - return false; -} - function once(callback, { preserveReturnValue = false } = kEmptyObject) { let called = false; let returnValue; @@ -911,7 +883,6 @@ module.exports = { getSystemErrorName, guessHandleType, isError, - isInsideNodeModules, isUnderNodeModules, isMacOS, isWindows, diff --git a/src/node_util.cc b/src/node_util.cc index c99e26752c7d93..77da3235182efe 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -298,6 +298,48 @@ static void GetCallSite(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(callsites); } +static void IsInsideNodeModules(const FunctionCallbackInfo& args) { + Isolate* isolate = args.GetIsolate(); + CHECK_EQ(args.Length(), 2); + CHECK(args[0]->IsInt32()); // frame_limit + // The second argument is the default value. + + int frames_limit = args[0].As()->Value(); + Local stack = + StackTrace::CurrentStackTrace(isolate, frames_limit); + int frame_count = stack->GetFrameCount(); + + // If the search requires looking into more than |frames_limit| frames, give + // up and return the specified default value. + if (frame_count == frames_limit) { + return args.GetReturnValue().Set(args[1]); + } + + bool result = false; + for (int i = 0; i < frame_count; ++i) { + Local stack_frame = stack->GetFrame(isolate, i); + Local script_name = stack_frame->GetScriptName(); + + if (script_name.IsEmpty() || script_name->Length() == 0) { + continue; + } + Utf8Value script_name_utf8(isolate, script_name); + std::string_view script_name_str = script_name_utf8.ToStringView(); + if (script_name_str.starts_with("node:")) { + continue; + } + if (script_name_str.find("/node_modules/") != std::string::npos || + script_name_str.find("\\node_modules\\") != std::string::npos || + script_name_str.find("/node_modules\\") != std::string::npos || + script_name_str.find("\\node_modules/") != std::string::npos) { + result = true; + break; + } + } + + args.GetReturnValue().Set(result); +} + void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(GetPromiseDetails); registry->Register(GetProxyDetails); @@ -313,6 +355,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(FastGuessHandleType); registry->Register(fast_guess_handle_type_.GetTypeInfo()); registry->Register(ParseEnv); + registry->Register(IsInsideNodeModules); } void Initialize(Local target, @@ -396,6 +439,7 @@ void Initialize(Local target, target->Set(context, env->constants_string(), constants).Check(); } + SetMethod(context, target, "isInsideNodeModules", IsInsideNodeModules); SetMethodNoSideEffect( context, target, "getPromiseDetails", GetPromiseDetails); SetMethodNoSideEffect(context, target, "getProxyDetails", GetProxyDetails); diff --git a/test/fixtures/warning_node_modules/new-buffer-cjs.js b/test/fixtures/warning_node_modules/new-buffer-cjs.js new file mode 100644 index 00000000000000..be2877fa30c46d --- /dev/null +++ b/test/fixtures/warning_node_modules/new-buffer-cjs.js @@ -0,0 +1 @@ +require('new-buffer-cjs'); diff --git a/test/fixtures/warning_node_modules/new-buffer-esm.mjs b/test/fixtures/warning_node_modules/new-buffer-esm.mjs new file mode 100644 index 00000000000000..9aa56f759f8ae4 --- /dev/null +++ b/test/fixtures/warning_node_modules/new-buffer-esm.mjs @@ -0,0 +1 @@ +import 'new-buffer-esm' diff --git a/test/fixtures/warning_node_modules/node_modules/new-buffer-cjs/index.js b/test/fixtures/warning_node_modules/node_modules/new-buffer-cjs/index.js new file mode 100644 index 00000000000000..514db554ed6edf --- /dev/null +++ b/test/fixtures/warning_node_modules/node_modules/new-buffer-cjs/index.js @@ -0,0 +1 @@ +new Buffer(10); diff --git a/test/fixtures/warning_node_modules/node_modules/new-buffer-cjs/package.json b/test/fixtures/warning_node_modules/node_modules/new-buffer-cjs/package.json new file mode 100644 index 00000000000000..f2c75cf8e443b4 --- /dev/null +++ b/test/fixtures/warning_node_modules/node_modules/new-buffer-cjs/package.json @@ -0,0 +1,3 @@ +{ + "main": "index.js" +} \ No newline at end of file diff --git a/test/fixtures/warning_node_modules/node_modules/new-buffer-esm/index.js b/test/fixtures/warning_node_modules/node_modules/new-buffer-esm/index.js new file mode 100644 index 00000000000000..9dadaeb12413c2 --- /dev/null +++ b/test/fixtures/warning_node_modules/node_modules/new-buffer-esm/index.js @@ -0,0 +1,2 @@ +import { Buffer } from 'node:buffer'; +new Buffer(10); diff --git a/test/fixtures/warning_node_modules/node_modules/new-buffer-esm/package.json b/test/fixtures/warning_node_modules/node_modules/new-buffer-esm/package.json new file mode 100644 index 00000000000000..07fe0622822c0e --- /dev/null +++ b/test/fixtures/warning_node_modules/node_modules/new-buffer-esm/package.json @@ -0,0 +1,4 @@ +{ + "main": "index.js", + "type": "module" +} \ No newline at end of file diff --git a/test/parallel/test-buffer-constructor-node-modules.js b/test/parallel/test-buffer-constructor-node-modules.js new file mode 100644 index 00000000000000..e865b9e0ae2f0b --- /dev/null +++ b/test/parallel/test-buffer-constructor-node-modules.js @@ -0,0 +1,48 @@ +'use strict'; + +const common = require('../common'); +const fixtures = require('../common/fixtures'); +const { spawnSyncAndAssert } = require('../common/child_process'); + +if (process.env.NODE_PENDING_DEPRECATION) + common.skip('test does not work when NODE_PENDING_DEPRECATION is set'); + +spawnSyncAndAssert( + process.execPath, + [ fixtures.path('warning_node_modules', 'new-buffer-cjs.js') ], + { + trim: true, + stderr: '', + } +); + +spawnSyncAndAssert( + process.execPath, + [ fixtures.path('warning_node_modules', 'new-buffer-esm.mjs') ], + { + trim: true, + stderr: '', + } +); + +spawnSyncAndAssert( + process.execPath, + [ + '--pending-deprecation', + fixtures.path('warning_node_modules', 'new-buffer-cjs.js'), + ], + { + stderr: /DEP0005/ + } +); + +spawnSyncAndAssert( + process.execPath, + [ + '--pending-deprecation', + fixtures.path('warning_node_modules', 'new-buffer-esm.mjs'), + ], + { + stderr: /DEP0005/ + } +); From d4d12cd05f160a5445ba4e94f5997807ddb0fdae Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 22 Oct 2024 18:51:45 +0200 Subject: [PATCH 07/10] module: include module information in require(esm) warning When emitting the experimental warning for `require(esm)`, include information about the parent module and the module being require()-d to help users locate and update them. PR-URL: https://github.com/nodejs/node/pull/55397 Reviewed-By: Matteo Collina Reviewed-By: Stephen Belanger Reviewed-By: Chemi Atlow Reviewed-By: James M Snell Reviewed-By: Chengzhong Wu --- lib/internal/modules/cjs/loader.js | 24 +++++++++++++++++-- lib/internal/util.js | 7 ++++-- test/es-module/test-require-module-preload.js | 18 +++++++------- test/es-module/test-require-module.js | 4 ++++ 4 files changed, 41 insertions(+), 12 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 310e9a3a630efd..fef17d95b4b08f 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -1374,11 +1374,31 @@ function loadESMFromCJS(mod, filename) { // ESM won't be accessible via process.mainModule. setOwnProperty(process, 'mainModule', undefined); } else { - emitExperimentalWarning('Support for loading ES Module in require()'); + const parent = mod[kModuleParent]; + let messagePrefix; + if (parent) { + // In the case of the module calling `require()`, it's more useful to know its absolute path. + let from = parent.filename || parent.id; + // In the case of the module being require()d, it's more useful to know the id passed into require(). + const to = mod.id || mod.filename; + if (from === 'internal/preload') { + from = '--require'; + } else if (from === '') { + from = 'The REPL'; + } else if (from === '.') { + from = 'The entry point'; + } else { + from &&= `CommonJS module ${from}`; + } + if (from && to) { + messagePrefix = `${from} is loading ES Module ${to} using require().\n`; + } + } + emitExperimentalWarning('Support for loading ES Module in require()', messagePrefix); const { wrap, namespace, - } = cascadedLoader.importSyncForRequire(mod, filename, source, isMain, mod[kModuleParent]); + } = cascadedLoader.importSyncForRequire(mod, filename, source, isMain, parent); // 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: // diff --git a/lib/internal/util.js b/lib/internal/util.js index a3ffbbdc6b350f..de05bb76c5c7a5 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -256,10 +256,13 @@ function slowCases(enc) { } } -function emitExperimentalWarning(feature) { +function emitExperimentalWarning(feature, messagePrefix) { if (experimentalWarnings.has(feature)) return; - const msg = `${feature} is an experimental feature and might change at any time`; experimentalWarnings.add(feature); + let msg = `${feature} is an experimental feature and might change at any time`; + if (messagePrefix) { + msg = messagePrefix + msg; + } process.emitWarning(msg, 'ExperimentalWarning'); } diff --git a/test/es-module/test-require-module-preload.js b/test/es-module/test-require-module-preload.js index 65ec1a93bc9d4b..7a8a09486b43d7 100644 --- a/test/es-module/test-require-module-preload.js +++ b/test/es-module/test-require-module-preload.js @@ -4,11 +4,9 @@ require('../common'); const { spawnSyncAndAssert } = require('../common/child_process'); const { fixturesDir } = require('../common/fixtures'); -const warningRE = /ExperimentalWarning: Support for loading ES Module in require/; function testPreload(preloadFlag) { // The warning is only emitted when ESM is loaded by --require. - const stderr = preloadFlag !== '--import' ? warningRE : undefined; - + const isRequire = preloadFlag === '--require'; // Test named exports. { spawnSyncAndAssert( @@ -24,7 +22,8 @@ function testPreload(preloadFlag) { }, { stdout: 'A', - stderr, + stderr: isRequire ? + /ExperimentalWarning: --require is loading ES Module .*module-named-exports\.mjs using require/ : undefined, trim: true, } ); @@ -44,7 +43,8 @@ function testPreload(preloadFlag) { cwd: fixturesDir }, { - stderr, + stderr: isRequire ? + /ExperimentalWarning: --require is loading ES Module .*import-esm\.mjs using require/ : undefined, stdout: /^world\s+A$/, trim: true, } @@ -66,7 +66,8 @@ function testPreload(preloadFlag) { }, { stdout: /^ok\s+A$/, - stderr, + stderr: isRequire ? + /ExperimentalWarning: --require is loading ES Module .*cjs-exports\.mjs using require/ : undefined, trim: true, } ); @@ -89,7 +90,8 @@ function testPreload(preloadFlag) { }, { stdout: /^world\s+A$/, - stderr, + stderr: isRequire ? + /ExperimentalWarning: --require is loading ES Module .*require-cjs\.mjs using require/ : undefined, trim: true, } ); @@ -115,7 +117,7 @@ testPreload('--import'); }, { stdout: /^package-type-module\s+A$/, - stderr: warningRE, + stderr: /ExperimentalWarning: --require is loading ES Module .*package-type-module[\\/]index\.js using require/, trim: true, } ); diff --git a/test/es-module/test-require-module.js b/test/es-module/test-require-module.js index 2cc7d0198837df..4987fbf7b07d43 100644 --- a/test/es-module/test-require-module.js +++ b/test/es-module/test-require-module.js @@ -3,9 +3,13 @@ const common = require('../common'); const assert = require('assert'); +const path = require('path'); +// Only the first load will trigger the warning. common.expectWarning( 'ExperimentalWarning', + `CommonJS module ${__filename} is loading ES Module ` + + `${path.resolve(__dirname, '../fixtures/es-module-loaders/module-named-exports.mjs')} using require().\n` + 'Support for loading ES Module in require() is an experimental feature ' + 'and might change at any time' ); From a5ef7c10e1404502ab5e87bfaedfea350592ff15 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 28 Oct 2024 13:21:22 +0100 Subject: [PATCH 08/10] module: allow ESM that failed to be required to be re-imported When a ESM module cannot be loaded by require due to the presence of TLA, its module status would be stopped at kInstantiated. In this case, when it's imported again, we should allow it to be evaluated asynchronously, as it's also a common pattern for users to retry with dynamic import when require fails. PR-URL: https://github.com/nodejs/node/pull/55502 Fixes: https://github.com/nodejs/node/issues/55500 Refs: https://github.com/nodejs/node/issues/52697 Reviewed-By: Matteo Collina Reviewed-By: Chemi Atlow --- lib/internal/modules/esm/module_job.js | 24 ++++++++++--- ...est-require-module-retry-import-errored.js | 35 +++++++++++++++++++ ...-require-module-retry-import-evaluating.js | 32 +++++++++++++++++ .../test-require-module-tla-retry-import-2.js | 26 ++++++++++++++ .../test-require-module-tla-retry-import.js | 25 +++++++++++++ .../es-modules/tla/await-export-promise.mjs | 4 +++ test/fixtures/es-modules/tla/export-async.mjs | 2 ++ .../es-modules/tla/export-promise.mjs | 8 +++++ 8 files changed, 152 insertions(+), 4 deletions(-) create mode 100644 test/es-module/test-require-module-retry-import-errored.js create mode 100644 test/es-module/test-require-module-retry-import-evaluating.js create mode 100644 test/es-module/test-require-module-tla-retry-import-2.js create mode 100644 test/es-module/test-require-module-tla-retry-import.js create mode 100644 test/fixtures/es-modules/tla/await-export-promise.mjs create mode 100644 test/fixtures/es-modules/tla/export-async.mjs create mode 100644 test/fixtures/es-modules/tla/export-promise.mjs diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index 413fcd27703e3e..ece30c3864d6a7 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -22,7 +22,7 @@ let debug = require('internal/util/debuglog').debuglog('esm', (fn) => { debug = fn; }); -const { ModuleWrap, kEvaluated } = internalBinding('module_wrap'); +const { ModuleWrap, kInstantiated } = internalBinding('module_wrap'); const { privateSymbols: { entry_point_module_private_symbol, @@ -354,10 +354,26 @@ class ModuleJobSync extends ModuleJobBase { } async run() { + // This path is hit by a require'd module that is imported again. const status = this.module.getStatus(); - assert(status === kEvaluated, - `A require()-d module that is imported again must be evaluated. Status = ${status}`); - return { __proto__: null, module: this.module }; + if (status > kInstantiated) { + if (this.evaluationPromise) { + await this.evaluationPromise; + } + return { __proto__: null, module: this.module }; + } else if (status === kInstantiated) { + // The evaluation may have been canceled because instantiateSync() detected TLA first. + // But when it is imported again, it's fine to re-evaluate it asynchronously. + const timeout = -1; + const breakOnSigint = false; + this.evaluationPromise = this.module.evaluate(timeout, breakOnSigint); + await this.evaluationPromise; + this.evaluationPromise = undefined; + return { __proto__: null, module: this.module }; + } + + assert.fail('Unexpected status of a module that is imported again after being required. ' + + `Status = ${status}`); } runSync() { diff --git a/test/es-module/test-require-module-retry-import-errored.js b/test/es-module/test-require-module-retry-import-errored.js new file mode 100644 index 00000000000000..4736087d2624a5 --- /dev/null +++ b/test/es-module/test-require-module-retry-import-errored.js @@ -0,0 +1,35 @@ +// This tests that after failing to require an ESM that contains TLA, +// retrying with import() still works, and produces consistent results. +'use strict'; +const common = require('../common'); +const assert = require('assert'); + +const { exportedReject } = require('../fixtures/es-modules/tla/export-promise.mjs'); + +assert.throws(() => { + require('../fixtures/es-modules/tla/await-export-promise.mjs'); +}, { + code: 'ERR_REQUIRE_ASYNC_MODULE' +}); + +const interval = setInterval(() => {}, 1000); // Keep the test running, because await alone doesn't. +const err = new Error('rejected'); + +const p1 = import('../fixtures/es-modules/tla/await-export-promise.mjs') + .then(common.mustNotCall(), common.mustCall((e) => { + assert.strictEqual(e, err); + })); + +const p2 = import('../fixtures/es-modules/tla/await-export-promise.mjs') + .then(common.mustNotCall(), common.mustCall((e) => { + assert.strictEqual(e, err); + })); + +const p3 = import('../fixtures/es-modules/tla/await-export-promise.mjs') + .then(common.mustNotCall(), common.mustCall((e) => { + assert.strictEqual(e, err); + })); + +exportedReject(err); + +Promise.all([p1, p2, p3]).then(common.mustCall(() => { clearInterval(interval); })); diff --git a/test/es-module/test-require-module-retry-import-evaluating.js b/test/es-module/test-require-module-retry-import-evaluating.js new file mode 100644 index 00000000000000..df70432f1136e4 --- /dev/null +++ b/test/es-module/test-require-module-retry-import-evaluating.js @@ -0,0 +1,32 @@ +// This tests that after failing to require an ESM that contains TLA, +// retrying with import() still works, and produces consistent results. +'use strict'; +const common = require('../common'); +const assert = require('assert'); + +const { exportedResolve } = require('../fixtures/es-modules/tla/export-promise.mjs'); + +assert.throws(() => { + require('../fixtures/es-modules/tla/await-export-promise.mjs'); +}, { + code: 'ERR_REQUIRE_ASYNC_MODULE' +}); + +const interval = setInterval(() => {}, 1000); // Keep the test running, because await alone doesn't. +const value = { hello: 'world' }; + +const p1 = import('../fixtures/es-modules/tla/await-export-promise.mjs').then(common.mustCall((ns) => { + assert.strictEqual(ns.default, value); +}), common.mustNotCall()); + +const p2 = import('../fixtures/es-modules/tla/await-export-promise.mjs').then(common.mustCall((ns) => { + assert.strictEqual(ns.default, value); +}), common.mustNotCall()); + +const p3 = import('../fixtures/es-modules/tla/await-export-promise.mjs').then(common.mustCall((ns) => { + assert.strictEqual(ns.default, value); +}), common.mustNotCall()); + +exportedResolve(value); + +Promise.all([p1, p2, p3]).then(common.mustCall(() => { clearInterval(interval); })); diff --git a/test/es-module/test-require-module-tla-retry-import-2.js b/test/es-module/test-require-module-tla-retry-import-2.js new file mode 100644 index 00000000000000..aa9c344dd398d7 --- /dev/null +++ b/test/es-module/test-require-module-tla-retry-import-2.js @@ -0,0 +1,26 @@ +// This tests that after loading a ESM with import() and then retrying +// with require(), it errors as expected, and produces consistent results. +'use strict'; +const common = require('../common'); +const assert = require('assert'); + +let ns; +async function test() { + const newNs = await import('../fixtures/es-modules/tla/export-async.mjs'); + if (ns === undefined) { + ns = newNs; + } else { + // Check that re-evalaution is returning the same namespace. + assert.strictEqual(ns, newNs); + } + assert.strictEqual(ns.hello, 'world'); + + assert.throws(() => { + require('../fixtures/es-modules/tla/export-async.mjs'); + }, { + code: 'ERR_REQUIRE_ASYNC_MODULE' + }); +} + +// Run the test twice to check consistency after caching. +test().then(common.mustCall(test)).catch(common.mustNotCall()); diff --git a/test/es-module/test-require-module-tla-retry-import.js b/test/es-module/test-require-module-tla-retry-import.js new file mode 100644 index 00000000000000..70f918fa4f463f --- /dev/null +++ b/test/es-module/test-require-module-tla-retry-import.js @@ -0,0 +1,25 @@ +// This tests that after failing to require an ESM that contains TLA, +// retrying with import() still works, and produces consistent results. +'use strict'; +const common = require('../common'); +const assert = require('assert'); + +let ns; +async function test() { + assert.throws(() => { + require('../fixtures/es-modules/tla/export-async.mjs'); + }, { + code: 'ERR_REQUIRE_ASYNC_MODULE' + }); + const newNs = await import('../fixtures/es-modules/tla/export-async.mjs'); + if (ns === undefined) { + ns = newNs; + } else { + // Check that re-evalaution is returning the same namespace. + assert.strictEqual(ns, newNs); + } + assert.strictEqual(ns.hello, 'world'); +} + +// Run the test twice to check consistency after caching. +test().then(common.mustCall(test)).catch(common.mustNotCall()); diff --git a/test/fixtures/es-modules/tla/await-export-promise.mjs b/test/fixtures/es-modules/tla/await-export-promise.mjs new file mode 100644 index 00000000000000..0129793e42b54a --- /dev/null +++ b/test/fixtures/es-modules/tla/await-export-promise.mjs @@ -0,0 +1,4 @@ +import promise from './export-promise.mjs'; +let result; +result = await promise; +export default result; diff --git a/test/fixtures/es-modules/tla/export-async.mjs b/test/fixtures/es-modules/tla/export-async.mjs new file mode 100644 index 00000000000000..b6de9a5a5f68e9 --- /dev/null +++ b/test/fixtures/es-modules/tla/export-async.mjs @@ -0,0 +1,2 @@ +let hello = await Promise.resolve('world'); +export { hello }; diff --git a/test/fixtures/es-modules/tla/export-promise.mjs b/test/fixtures/es-modules/tla/export-promise.mjs new file mode 100644 index 00000000000000..74864d232e3c35 --- /dev/null +++ b/test/fixtures/es-modules/tla/export-promise.mjs @@ -0,0 +1,8 @@ +let exportedResolve; +let exportedReject; +const promise = new Promise((resolve, reject) => { + exportedResolve = resolve; + exportedReject = reject; +}); +export default promise; +export { exportedResolve, exportedReject }; From 2c468590d926eeeaa3b9017d6db45ddffbaa41fe Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 18 Oct 2024 16:48:19 +0200 Subject: [PATCH 09/10] module: trim off internal stack frames for require(esm) warnings Trim off irrelevant internal stack frames for require(esm) warnings so it's easier to locate where the call comes from when --trace-warnings is used. PR-URL: https://github.com/nodejs/node/pull/55496 Reviewed-By: Marco Ippolito Reviewed-By: Paolo Insogna --- lib/internal/modules/cjs/loader.js | 5 ++- lib/internal/util.js | 10 ++++-- test/es-module/test-require-module-warning.js | 35 +++++++++++++++++++ test/fixtures/es-modules/require-module.js | 1 + 4 files changed, 48 insertions(+), 3 deletions(-) create mode 100644 test/es-module/test-require-module-warning.js create mode 100644 test/fixtures/es-modules/require-module.js diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index fef17d95b4b08f..5af29ef56acaac 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -1394,7 +1394,10 @@ function loadESMFromCJS(mod, filename) { messagePrefix = `${from} is loading ES Module ${to} using require().\n`; } } - emitExperimentalWarning('Support for loading ES Module in require()', messagePrefix); + emitExperimentalWarning('Support for loading ES Module in require()', + messagePrefix, + undefined, + parent?.require); const { wrap, namespace, diff --git a/lib/internal/util.js b/lib/internal/util.js index de05bb76c5c7a5..3fc0dc9c207fb5 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -256,14 +256,20 @@ function slowCases(enc) { } } -function emitExperimentalWarning(feature, messagePrefix) { +/** + * @param {string} feature Feature name used in the warning message + * @param {string} messagePrefix Prefix of the warning message + * @param {string} code See documentation of process.emitWarning + * @param {string} ctor See documentation of process.emitWarning + */ +function emitExperimentalWarning(feature, messagePrefix, code, ctor) { if (experimentalWarnings.has(feature)) return; experimentalWarnings.add(feature); let msg = `${feature} is an experimental feature and might change at any time`; if (messagePrefix) { msg = messagePrefix + msg; } - process.emitWarning(msg, 'ExperimentalWarning'); + process.emitWarning(msg, 'ExperimentalWarning', code, ctor); } function filterDuplicateStrings(items, low) { diff --git a/test/es-module/test-require-module-warning.js b/test/es-module/test-require-module-warning.js new file mode 100644 index 00000000000000..d5be2fc6da8755 --- /dev/null +++ b/test/es-module/test-require-module-warning.js @@ -0,0 +1,35 @@ +'use strict'; + +// This checks the warning and the stack trace emitted by the require(esm) +// experimental warning. It can get removed when `require(esm)` becomes stable. + +require('../common'); +const { spawnSyncAndAssert } = require('../common/child_process'); +const fixtures = require('../common/fixtures'); +const assert = require('assert'); + +spawnSyncAndAssert(process.execPath, [ + '--trace-warnings', + fixtures.path('es-modules', 'require-module.js'), +], { + trim: true, + stderr(output) { + const lines = output.split('\n'); + assert.match( + lines[0], + /ExperimentalWarning: CommonJS module .*require-module\.js is loading ES Module .*message\.mjs/ + ); + assert.strictEqual( + lines[1], + 'Support for loading ES Module in require() is an experimental feature and might change at any time' + ); + assert.match( + lines[2], + /at require \(.*modules\/helpers:\d+:\d+\)/ + ); + assert.match( + lines[3], + /at Object\. \(.*require-module\.js:1:1\)/ + ); + } +}); diff --git a/test/fixtures/es-modules/require-module.js b/test/fixtures/es-modules/require-module.js new file mode 100644 index 00000000000000..5a36590fc6171f --- /dev/null +++ b/test/fixtures/es-modules/require-module.js @@ -0,0 +1 @@ +require('./message.mjs'); From 4af41f06f58c49803cb00437dda60f60d2810be4 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 23 Nov 2024 12:28:47 +0100 Subject: [PATCH 10/10] module: do not warn when require(esm) comes from node_modules As part of the standard experimental feature graduation policy, when we unflagged require(esm) we moved the experimental warning to be emitted when require() is actually used to load ESM, which previously was an error. However, some packages in the ecosystem have already being using try-catch to load require(esm) to e.g. resolve optional dependency, and emitting warning from there instead of throwing directly could break the CLI output. To reduce the disruption for releases, as a compromise, this patch skips the warning if require(esm) comes from node_modules, where users typically don't have much control over the code. This warning will be eventually removed when require(esm) becomes stable. This patch was originally intended for the LTS releases, though it seems there's appetite for it on v23.x as well so it's re-targeted to the main branch. PR-URL: https://github.com/nodejs/node/pull/55960 Refs: https://github.com/nodejs/node/pull/55217 Refs: https://github.com/nodejs/node/issues/52697 Reviewed-By: Matteo Collina Reviewed-By: Antoine du Hamel Reviewed-By: Jacob Smith --- lib/internal/modules/cjs/loader.js | 59 ++++++++++++------- .../test-require-node-modules-warning.js | 59 +++++++++++++++++++ .../import-import-require-esm.mjs | 2 + .../test_node_modules/import-require-esm.mjs | 2 + .../node_modules/esm/index.js | 1 + .../node_modules/esm/package.json | 4 ++ .../node_modules/import-require-esm/index.js | 2 + .../import-require-esm/package.json | 4 ++ .../node_modules/require-esm/index.js | 2 + .../test_node_modules/require-esm.js | 2 + .../test_node_modules/require-require-esm.js | 2 + 11 files changed, 119 insertions(+), 20 deletions(-) create mode 100644 test/es-module/test-require-node-modules-warning.js create mode 100644 test/fixtures/es-modules/test_node_modules/import-import-require-esm.mjs create mode 100644 test/fixtures/es-modules/test_node_modules/import-require-esm.mjs create mode 100644 test/fixtures/es-modules/test_node_modules/node_modules/esm/index.js create mode 100644 test/fixtures/es-modules/test_node_modules/node_modules/esm/package.json create mode 100644 test/fixtures/es-modules/test_node_modules/node_modules/import-require-esm/index.js create mode 100644 test/fixtures/es-modules/test_node_modules/node_modules/import-require-esm/package.json create mode 100644 test/fixtures/es-modules/test_node_modules/node_modules/require-esm/index.js create mode 100644 test/fixtures/es-modules/test_node_modules/require-esm.js create mode 100644 test/fixtures/es-modules/test_node_modules/require-require-esm.js diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 5af29ef56acaac..d28a295c3af9ec 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -70,6 +70,7 @@ const { module_export_private_symbol, module_parent_private_symbol, }, + isInsideNodeModules, } = internalBinding('util'); const { kEvaluated, createRequiredModuleFacade } = internalBinding('module_wrap'); @@ -1342,6 +1343,7 @@ Module.prototype.require = function(id) { } }; +let emittedRequireModuleWarning = false; /** * Resolved path to `process.argv[1]` will be lazily placed here * (needed for setting breakpoint when called with `--inspect-brk`). @@ -1375,29 +1377,46 @@ function loadESMFromCJS(mod, filename) { setOwnProperty(process, 'mainModule', undefined); } else { const parent = mod[kModuleParent]; - let messagePrefix; - if (parent) { - // In the case of the module calling `require()`, it's more useful to know its absolute path. - let from = parent.filename || parent.id; - // In the case of the module being require()d, it's more useful to know the id passed into require(). - const to = mod.id || mod.filename; - if (from === 'internal/preload') { - from = '--require'; - } else if (from === '') { - from = 'The REPL'; - } else if (from === '.') { - from = 'The entry point'; - } else { - from &&= `CommonJS module ${from}`; + + if (!emittedRequireModuleWarning) { + let shouldEmitWarning = false; + // Check if the require() comes from node_modules. + if (parent) { + shouldEmitWarning = !isUnderNodeModules(parent.filename); + } else if (mod[kIsCachedByESMLoader]) { + // It comes from the require() built for `import cjs` and doesn't have a parent recorded + // in the CJS module instance. Inspect the stack trace to see if the require() + // comes from node_modules and reduce the noise. If there are more than 100 frames, + // just give up and assume it is under node_modules. + shouldEmitWarning = !isInsideNodeModules(100, true); } - if (from && to) { - messagePrefix = `${from} is loading ES Module ${to} using require().\n`; + if (shouldEmitWarning) { + let messagePrefix; + if (parent) { + // In the case of the module calling `require()`, it's more useful to know its absolute path. + let from = parent.filename || parent.id; + // In the case of the module being require()d, it's more useful to know the id passed into require(). + const to = mod.id || mod.filename; + if (from === 'internal/preload') { + from = '--require'; + } else if (from === '') { + from = 'The REPL'; + } else if (from === '.') { + from = 'The entry point'; + } else { + from &&= `CommonJS module ${from}`; + } + if (from && to) { + messagePrefix = `${from} is loading ES Module ${to} using require().\n`; + } + } + emitExperimentalWarning('Support for loading ES Module in require()', + messagePrefix, + undefined, + parent?.require); + emittedRequireModuleWarning = true; } } - emitExperimentalWarning('Support for loading ES Module in require()', - messagePrefix, - undefined, - parent?.require); const { wrap, namespace, diff --git a/test/es-module/test-require-node-modules-warning.js b/test/es-module/test-require-node-modules-warning.js new file mode 100644 index 00000000000000..837f174fd28950 --- /dev/null +++ b/test/es-module/test-require-node-modules-warning.js @@ -0,0 +1,59 @@ +'use strict'; + +// This checks the experimental warning for require(esm) is disabled when the +// require() comes from node_modules. +require('../common'); +const { spawnSyncAndAssert } = require('../common/child_process'); +const fixtures = require('../common/fixtures'); + +const warningRE = /Support for loading ES Module in require\(\)/; + +// The fixtures are placed in a directory that includes "node_modules" in its name +// to check false negatives. + +// require() in non-node_modules -> esm in node_modules should warn. +spawnSyncAndAssert( + process.execPath, + [fixtures.path('es-modules', 'test_node_modules', 'require-esm.js')], + { + trim: true, + stderr: warningRE, + stdout: 'world', + } +); + +// require() in non-node_modules -> require() in node_modules -> esm in node_modules +// should not warn. +spawnSyncAndAssert( + process.execPath, + [fixtures.path('es-modules', 'test_node_modules', 'require-require-esm.js')], + { + trim: true, + stderr: '', + stdout: 'world', + } +); + +// Import in non-node_modules -> require() in node_modules -> esm in node_modules +// should not warn. +spawnSyncAndAssert( + process.execPath, + [fixtures.path('es-modules', 'test_node_modules', 'import-require-esm.mjs')], + { + trim: true, + stderr: '', + stdout: 'world', + } +); + +// Import in non-node_modules -> import in node_modules -> +// require() in node_modules -> esm in node_modules should not warn. +spawnSyncAndAssert( + process.execPath, + [fixtures.path('es-modules', 'test_node_modules', 'import-import-require-esm.mjs')], + { + trim: true, + stderr: '', + stdout: 'world', + } +); diff --git a/test/fixtures/es-modules/test_node_modules/import-import-require-esm.mjs b/test/fixtures/es-modules/test_node_modules/import-import-require-esm.mjs new file mode 100644 index 00000000000000..d470088c38caee --- /dev/null +++ b/test/fixtures/es-modules/test_node_modules/import-import-require-esm.mjs @@ -0,0 +1,2 @@ +import mod from 'import-require-esm'; +console.log(mod.hello); diff --git a/test/fixtures/es-modules/test_node_modules/import-require-esm.mjs b/test/fixtures/es-modules/test_node_modules/import-require-esm.mjs new file mode 100644 index 00000000000000..2ad346de9e841c --- /dev/null +++ b/test/fixtures/es-modules/test_node_modules/import-require-esm.mjs @@ -0,0 +1,2 @@ +import mod from 'require-esm'; +console.log(mod.hello); diff --git a/test/fixtures/es-modules/test_node_modules/node_modules/esm/index.js b/test/fixtures/es-modules/test_node_modules/node_modules/esm/index.js new file mode 100644 index 00000000000000..35f468bf4856d8 --- /dev/null +++ b/test/fixtures/es-modules/test_node_modules/node_modules/esm/index.js @@ -0,0 +1 @@ +export const hello = 'world'; diff --git a/test/fixtures/es-modules/test_node_modules/node_modules/esm/package.json b/test/fixtures/es-modules/test_node_modules/node_modules/esm/package.json new file mode 100644 index 00000000000000..07aec65d5a4f31 --- /dev/null +++ b/test/fixtures/es-modules/test_node_modules/node_modules/esm/package.json @@ -0,0 +1,4 @@ +{ + "type": "module", + "main": "index.js" +} diff --git a/test/fixtures/es-modules/test_node_modules/node_modules/import-require-esm/index.js b/test/fixtures/es-modules/test_node_modules/node_modules/import-require-esm/index.js new file mode 100644 index 00000000000000..918bb5d5597e34 --- /dev/null +++ b/test/fixtures/es-modules/test_node_modules/node_modules/import-require-esm/index.js @@ -0,0 +1,2 @@ +import mod from 'require-esm'; +export default mod; diff --git a/test/fixtures/es-modules/test_node_modules/node_modules/import-require-esm/package.json b/test/fixtures/es-modules/test_node_modules/node_modules/import-require-esm/package.json new file mode 100644 index 00000000000000..07aec65d5a4f31 --- /dev/null +++ b/test/fixtures/es-modules/test_node_modules/node_modules/import-require-esm/package.json @@ -0,0 +1,4 @@ +{ + "type": "module", + "main": "index.js" +} diff --git a/test/fixtures/es-modules/test_node_modules/node_modules/require-esm/index.js b/test/fixtures/es-modules/test_node_modules/node_modules/require-esm/index.js new file mode 100644 index 00000000000000..ca6f0c264ad1fc --- /dev/null +++ b/test/fixtures/es-modules/test_node_modules/node_modules/require-esm/index.js @@ -0,0 +1,2 @@ +module.exports = require('esm'); + diff --git a/test/fixtures/es-modules/test_node_modules/require-esm.js b/test/fixtures/es-modules/test_node_modules/require-esm.js new file mode 100644 index 00000000000000..60ad3f7fff60c6 --- /dev/null +++ b/test/fixtures/es-modules/test_node_modules/require-esm.js @@ -0,0 +1,2 @@ +const { hello } = require('esm'); +console.log(hello); diff --git a/test/fixtures/es-modules/test_node_modules/require-require-esm.js b/test/fixtures/es-modules/test_node_modules/require-require-esm.js new file mode 100644 index 00000000000000..9fe255dce258a6 --- /dev/null +++ b/test/fixtures/es-modules/test_node_modules/require-require-esm.js @@ -0,0 +1,2 @@ +const { hello } = require('require-esm'); +console.log(hello);