Skip to content

Commit

Permalink
module: unflag --experimental-require-module
Browse files Browse the repository at this point in the history
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: #55085
Backport-PR-URL: #55217
Refs: #52697
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
  • Loading branch information
joyeecheung authored and ruyadorno committed Nov 27, 2024
1 parent 9ffd2dd commit 58d6871
Show file tree
Hide file tree
Showing 24 changed files with 131 additions and 71 deletions.
24 changes: 21 additions & 3 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -1054,6 +1054,10 @@ following permissions are restricted:

<!-- YAML
added: v22.0.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/55085
description: This is now true by default.
-->

> Stability: 1.1 - Active Development
Expand Down Expand Up @@ -1695,6 +1699,22 @@ added: v16.6.0

Use this flag to disable top-level await in REPL.

### `--no-experimental-require-module`

<!-- YAML
added: v22.0.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/55085
description: This is now false by default.
-->

> 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`

<!-- YAML
Expand Down Expand Up @@ -1900,9 +1920,7 @@ Identical to `-e` but prints the result.
added: v22.0.0
-->

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.

Expand Down
23 changes: 16 additions & 7 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand All @@ -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
Expand All @@ -2504,12 +2504,20 @@ before looking for the top-level awaits).

### `ERR_REQUIRE_ESM`

> Stability: 1 - Experimental
<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/55085
description: require() now supports loading synchronous ES modules by default.
-->

> 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.

<a id="ERR_SCRIPT_EXECUTION_INTERRUPTED"></a>

Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion doc/api/esm.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
29 changes: 16 additions & 13 deletions doc/api/modules.md
Original file line number Diff line number Diff line change
Expand Up @@ -172,20 +172,20 @@ relative, and based on the real path of the files making the calls to

<!-- YAML
added: v22.0.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/55085
description: require() now supports loading synchronous ES modules by default.
-->

> 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:
Expand All @@ -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.

Expand All @@ -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');
Expand Down Expand Up @@ -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`.

Expand Down Expand Up @@ -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
<a href="esm.md#resolver-algorithm-specification">DETECT_MODULE_SYNTAX defined in
the ESM resolver</a>,
a. Load X as an ECMAScript module. STOP.
Expand Down Expand Up @@ -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
Expand Down
6 changes: 2 additions & 4 deletions doc/api/packages.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down
10 changes: 0 additions & 10 deletions lib/internal/modules/esm/load.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down Expand Up @@ -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,
Expand Down
8 changes: 4 additions & 4 deletions lib/internal/modules/esm/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -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`);
Expand Down
1 change: 1 addition & 0 deletions lib/internal/modules/esm/module_job.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
6 changes: 2 additions & 4 deletions lib/internal/modules/esm/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}
}

/**
Expand Down
5 changes: 3 additions & 2 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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)",
Expand Down
2 changes: 1 addition & 1 deletion src/node_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ class EnvironmentOptions : public Options {
std::vector<std::string> 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;
Expand Down
11 changes: 9 additions & 2 deletions test/es-module/test-cjs-esm-warn.js
Original file line number Diff line number Diff line change
@@ -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');
Expand All @@ -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(
Expand All @@ -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(
Expand Down
7 changes: 4 additions & 3 deletions test/es-module/test-esm-detect-ambiguous.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
});
Expand Down
1 change: 1 addition & 0 deletions test/es-module/test-esm-loader-hooks.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Expand Down
17 changes: 17 additions & 0 deletions test/es-module/test-esm-type-field-errors-2.js
Original file line number Diff line number Diff line change
@@ -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'
});
});
});
Loading

0 comments on commit 58d6871

Please sign in to comment.