Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

module: unflag --experimental-require-module #55085

Merged
merged 3 commits into from
Sep 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -1034,6 +1034,10 @@ following permissions are restricted:
added:
- v22.0.0
- v20.17.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 @@ -1659,6 +1663,24 @@ added: v16.6.0

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

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

<!-- YAML
added:
- v22.0.0
- v20.17.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 @@ -1875,9 +1897,7 @@ added:
- v20.17.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 @@ -2426,8 +2426,8 @@ object.

> 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 @@ -2441,8 +2441,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 @@ -2452,12 +2452,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 @@ -4061,6 +4069,7 @@ Type stripping is not supported for files descendent of a `node_modules` directo
[`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
33 changes: 20 additions & 13 deletions doc/api/modules.md
Original file line number Diff line number Diff line change
Expand Up @@ -170,15 +170,18 @@ relative, and based on the real path of the files making the calls to

## Loading ECMAScript modules using `require()`

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

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 @@ -187,8 +190,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 @@ -207,7 +210,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 @@ -236,13 +239,19 @@ 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.

## All together

<!-- type=misc -->
Expand Down Expand Up @@ -272,8 +281,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 @@ -1190,7 +1198,6 @@ This section was moved to
[`"main"`]: packages.md#main
[`"type"`]: packages.md#type
[`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 @@ -1386,6 +1385,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';
}

joyeecheung marked this conversation as resolved.
Show resolved Hide resolved
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;
joyeecheung marked this conversation as resolved.
Show resolved Hide resolved
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
Loading