Skip to content

Commit

Permalink
esm: make specifier flag clearly experimental
Browse files Browse the repository at this point in the history
`--es-module-specifier-resolution` is the only flagged portion of the
ESM implementation that does not have the word experimental in the flag
name. This commit changes the flag to:

`--experimental-specifier-resolution`

`--es-module-specifier-resolution` remains as an alias for backwards
compatibility but it is no longer documented.

PR-URL: #30678
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
  • Loading branch information
MylesBorins authored and targos committed Dec 9, 2019
1 parent 7b9400c commit eb4f443
Show file tree
Hide file tree
Showing 11 changed files with 89 additions and 31 deletions.
30 changes: 15 additions & 15 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -156,20 +156,6 @@ Enable experimental Source Map V3 support for stack traces.
Currently, overriding `Error.prepareStackTrace` is ignored when the
`--enable-source-maps` flag is set.

### `--es-module-specifier-resolution=mode`
<!-- YAML
added: v12.0.0
-->

Sets the resolution algorithm for resolving ES module specifiers. Valid options
are `explicit` and `node`.

The default is `explicit`, which requires providing the full path to a
module. The `node` mode will enable support for optional file extensions and
the ability to import a directory that has an index file.

Please see [customizing ESM specifier resolution][] for example usage.

### `--experimental-conditional-exports`
<!-- YAML
added: v13.2.0
Expand Down Expand Up @@ -223,6 +209,20 @@ added: v13.1.0
Enable experimental support for a package using `require` or `import` to load
itself.

### `--experimental-specifier-resolution=mode`
<!-- YAML
added: REPLACEME
-->

Sets the resolution algorithm for resolving ES module specifiers. Valid options
are `explicit` and `node`.

The default is `explicit`, which requires providing the full path to a
module. The `node` mode will enable support for optional file extensions and
the ability to import a directory that has an index file.

Please see [customizing ESM specifier resolution][] for example usage.

### `--experimental-vm-modules`
<!-- YAML
added: v9.6.0
Expand Down Expand Up @@ -1045,7 +1045,6 @@ Node.js options that are allowed are:
<!-- node-options-node start -->
* `--enable-fips`
* `--enable-source-maps`
* `--es-module-specifier-resolution`
* `--experimental-conditional-exports`
* `--experimental-json-modules`
* `--experimental-loader`
Expand All @@ -1054,6 +1053,7 @@ Node.js options that are allowed are:
* `--experimental-repl-await`
* `--experimental-report`
* `--experimental-resolve-self`
* `--experimental-specifier-resolution`
* `--experimental-vm-modules`
* `--experimental-wasi-unstable-preview0`
* `--experimental-wasm-modules`
Expand Down
4 changes: 2 additions & 2 deletions doc/api/esm.md
Original file line number Diff line number Diff line change
Expand Up @@ -1372,7 +1372,7 @@ the CommonJS loader. One of the behavior differences is automatic resolution
of file extensions and the ability to import directories that have an index
file.
The `--es-module-specifier-resolution=[mode]` flag can be used to customize
The `--experimental-specifier-resolution=[mode]` flag can be used to customize
the extension resolution algorithm. The default mode is `explicit`, which
requires the full path to a module be provided to the loader. To enable the
automatic extension resolution and importing from directories that include an
Expand All @@ -1383,7 +1383,7 @@ $ node index.mjs
success!
$ node index # Failure!
Error: Cannot find module
$ node --es-module-specifier-resolution=node index
$ node --experimental-specifier-resolution=node index
success!
```
Expand Down
6 changes: 3 additions & 3 deletions doc/node.1
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,6 @@ Enable FIPS-compliant crypto at startup.
Requires Node.js to be built with
.Sy ./configure --openssl-fips .
.
.It Fl -es-module-specifier-resolution
Select extension resolution algorithm for ES Modules; either 'explicit' (default) or 'node'
.
.It Fl -experimental-conditional-exports
Enable experimental support for "require" and "node" conditional export targets.
.
Expand All @@ -130,6 +127,9 @@ Enable experimental top-level
.Sy await
keyword support in REPL.
.
.It Fl -experimental-specifier-resolution
Select extension resolution algorithm for ES Modules; either 'explicit' (default) or 'node'
.
.It Fl -experimental-report
Enable experimental
.Sy diagnostic report
Expand Down
12 changes: 8 additions & 4 deletions lib/internal/modules/esm/default_resolve.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ const { getOptionValue } = require('internal/options');
const preserveSymlinks = getOptionValue('--preserve-symlinks');
const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main');
const experimentalJsonModules = getOptionValue('--experimental-json-modules');
const esModuleSpecifierResolution =
getOptionValue('--es-module-specifier-resolution');
const experimentalSpeciferResolution =
getOptionValue('--experimental-specifier-resolution');
const typeFlag = getOptionValue('--input-type');
const experimentalWasmModules = getOptionValue('--experimental-wasm-modules');
const { resolve: moduleWrapResolve,
Expand Down Expand Up @@ -110,10 +110,14 @@ function resolve(specifier, parentURL) {
if (ext === '.js' || (!format && isMain))
format = getPackageType(url.href) === TYPE_MODULE ? 'module' : 'commonjs';
if (!format) {
if (esModuleSpecifierResolution === 'node')
if (experimentalSpeciferResolution === 'node') {
process.emitWarning(
'The Node.js specifier resolution in ESM is experimental.',
'ExperimentalWarning');
format = legacyExtensionFormatMap[ext];
else
} else {
throw new ERR_UNKNOWN_FILE_EXTENSION(fileURLToPath(url));
}
}
return { url: `${url}`, format };
}
Expand Down
4 changes: 2 additions & 2 deletions src/module_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -789,7 +789,7 @@ inline Maybe<URL> ResolveIndex(const URL& search) {
Maybe<URL> FinalizeResolution(Environment* env,
const URL& resolved,
const URL& base) {
if (env->options()->es_module_specifier_resolution == "node") {
if (env->options()->experimental_specifier_resolution == "node") {
Maybe<URL> file = ResolveExtensions<TRY_EXACT_NAME>(resolved);
if (!file.IsNothing()) {
return file;
Expand Down Expand Up @@ -1053,7 +1053,7 @@ Maybe<URL> PackageMainResolve(Environment* env,
return Just(resolved);
}
}
if (env->options()->es_module_specifier_resolution == "node") {
if (env->options()->experimental_specifier_resolution == "node") {
if (pcfg.has_main == HasMain::Yes) {
return FinalizeResolution(env, URL(pcfg.main, pjson_url), base);
} else {
Expand Down
26 changes: 22 additions & 4 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,23 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors) {
}

if (!es_module_specifier_resolution.empty()) {
if (es_module_specifier_resolution != "node" &&
es_module_specifier_resolution != "explicit") {
errors->push_back("invalid value for --es-module-specifier-resolution");
if (!experimental_specifier_resolution.empty()) {
errors->push_back(
"bad option: cannot use --es-module-specifier-resolution"
" and --experimental-specifier-resolution at the same time");
} else {
experimental_specifier_resolution = es_module_specifier_resolution;
if (experimental_specifier_resolution != "node" &&
experimental_specifier_resolution != "explicit") {
errors->push_back(
"invalid value for --es-module-specifier-resolution");
}
}
} else if (!experimental_specifier_resolution.empty()) {
if (experimental_specifier_resolution != "node" &&
experimental_specifier_resolution != "explicit") {
errors->push_back(
"invalid value for --experimental-specifier-resolution");
}
}

Expand Down Expand Up @@ -365,9 +379,13 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
"set module type for string input",
&EnvironmentOptions::module_type,
kAllowedInEnvironment);
AddOption("--es-module-specifier-resolution",
AddOption("--experimental-specifier-resolution",
"Select extension resolution algorithm for es modules; "
"either 'explicit' (default) or 'node'",
&EnvironmentOptions::experimental_specifier_resolution,
kAllowedInEnvironment);
AddOption("--es-module-specifier-resolution",
"",
&EnvironmentOptions::es_module_specifier_resolution,
kAllowedInEnvironment);
AddOption("--no-deprecation",
Expand Down
1 change: 1 addition & 0 deletions src/node_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ class EnvironmentOptions : public Options {
bool experimental_conditional_exports = false;
bool experimental_json_modules = false;
bool experimental_resolve_self = false;
std::string experimental_specifier_resolution;
std::string es_module_specifier_resolution;
bool experimental_wasm_modules = false;
std::string module_type;
Expand Down
16 changes: 16 additions & 0 deletions test/es-module/test-esm-specifiers-both-flags.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { mustCall } from '../common/index.mjs';
import { exec } from 'child_process';
import assert from 'assert';

const expectedError =
'cannot use --es-module-specifier-resolution ' +
'and --experimental-specifier-resolution at the same time';

const flags = '--es-module-specifier-resolution=node ' +
'--experimental-specifier-resolution=node';

exec(`${process.execPath} ${flags}`, {
timeout: 300
}, mustCall((error) => {
assert(error.message.includes(expectedError));
}));
18 changes: 18 additions & 0 deletions test/es-module/test-esm-specifiers-legacy-flag.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Flags: --es-module-specifier-resolution=node
import '../common/index.mjs';
import assert from 'assert';

// commonJS index.js
import commonjs from '../fixtures/es-module-specifiers/package-type-commonjs';
// esm index.js
import module from '../fixtures/es-module-specifiers/package-type-module';
// Notice the trailing slash
import success, { explicit, implicit, implicitModule }
from '../fixtures/es-module-specifiers/';

assert.strictEqual(commonjs, 'commonjs');
assert.strictEqual(module, 'module');
assert.strictEqual(success, 'success');
assert.strictEqual(explicit, 'esm');
assert.strictEqual(implicit, 'cjs');
assert.strictEqual(implicitModule, 'cjs');
2 changes: 1 addition & 1 deletion test/es-module/test-esm-specifiers.mjs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Flags: --es-module-specifier-resolution=node
// Flags: --experimental-specifier-resolution=node
import { mustNotCall } from '../common/index.mjs';
import assert from 'assert';
import path from 'path';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ const undocumented = difference(process.allowedNodeEnvironmentFlags,
documented);
// Remove intentionally undocumented options.
assert(undocumented.delete('--debug-arraybuffer-allocations'));
assert(undocumented.delete('--es-module-specifier-resolution'));
assert(undocumented.delete('--experimental-worker'));
assert(undocumented.delete('--no-node-snapshot'));
assert(undocumented.delete('--loader'));
Expand Down

0 comments on commit eb4f443

Please sign in to comment.