From fb2d67019f2ede54ca850eb21da77ecbfb1ed4d2 Mon Sep 17 00:00:00 2001 From: Myles Borins Date: Wed, 27 Nov 2019 01:54:23 -0500 Subject: [PATCH 1/6] esm: make specifier flag clearly experimental `--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. --- doc/api/cli.md | 30 +++++++++---------- doc/api/esm.md | 4 +-- lib/internal/modules/esm/default_resolve.js | 6 ++-- src/module_wrap.cc | 4 +-- src/node_options.cc | 13 ++++---- src/node_options.h | 2 +- .../test-esm-specifiers-both-flags.mjs | 24 +++++++++++++++ .../test-esm-specifiers-legacy-flag.mjs | 18 +++++++++++ test/es-module/test-esm-specifiers.mjs | 2 +- 9 files changed, 73 insertions(+), 30 deletions(-) create mode 100644 test/es-module/test-esm-specifiers-both-flags.mjs create mode 100644 test/es-module/test-esm-specifiers-legacy-flag.mjs diff --git a/doc/api/cli.md b/doc/api/cli.md index c2bf666c9288d4..676001005b1297 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -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` - - -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` + +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` * `--enable-fips` * `--enable-source-maps` -* `--es-module-specifier-resolution` * `--experimental-conditional-exports` * `--experimental-json-modules` * `--experimental-loader` @@ -1047,6 +1046,7 @@ Node.js options that are allowed are: * `--experimental-repl-await` * `--experimental-report` * `--experimental-resolve-self` +* `--experimental-specifier-resolution` * `--experimental-vm-modules` * `--experimental-wasm-modules` * `--force-context-aware` diff --git a/doc/api/esm.md b/doc/api/esm.md index caaecaa0b41498..e4b878462d16e4 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -1369,7 +1369,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 @@ -1380,7 +1380,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! ``` diff --git a/lib/internal/modules/esm/default_resolve.js b/lib/internal/modules/esm/default_resolve.js index 169c6f35694f24..977a62c363a898 100644 --- a/lib/internal/modules/esm/default_resolve.js +++ b/lib/internal/modules/esm/default_resolve.js @@ -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, @@ -110,7 +110,7 @@ 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') format = legacyExtensionFormatMap[ext]; else throw new ERR_UNKNOWN_FILE_EXTENSION(fileURLToPath(url)); diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 5745cce9e099ab..3c3d568329221d 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -789,7 +789,7 @@ inline Maybe ResolveIndex(const URL& search) { Maybe 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 file = ResolveExtensions(resolved); if (!file.IsNothing()) { return file; @@ -1053,7 +1053,7 @@ Maybe 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 { diff --git a/src/node_options.cc b/src/node_options.cc index 0bc6730156ce12..90abfcf17efa9a 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -127,10 +127,11 @@ void EnvironmentOptions::CheckOptions(std::vector* 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()) { + if (experimental_specifier_resolution != "node" && + experimental_specifier_resolution != "explicit") { + errors->push_back( + "invalid value for --experimental-specifier-resolution"); } } @@ -361,10 +362,10 @@ 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::es_module_specifier_resolution, + &EnvironmentOptions::experimental_specifier_resolution, kAllowedInEnvironment); AddOption("--no-deprecation", "silence deprecation warnings", diff --git a/src/node_options.h b/src/node_options.h index ce0cee5fe56784..44f494810c96ee 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -104,7 +104,7 @@ class EnvironmentOptions : public Options { bool experimental_conditional_exports = false; bool experimental_json_modules = false; bool experimental_resolve_self = false; - std::string es_module_specifier_resolution; + std::string experimental_specifier_resolution; bool experimental_wasm_modules = false; std::string module_type; std::string experimental_policy; diff --git a/test/es-module/test-esm-specifiers-both-flags.mjs b/test/es-module/test-esm-specifiers-both-flags.mjs new file mode 100644 index 00000000000000..0b5816676475b1 --- /dev/null +++ b/test/es-module/test-esm-specifiers-both-flags.mjs @@ -0,0 +1,24 @@ +import '../common/index.mjs'; +import { exec } from 'child_process'; +import { ok, strictEqual } from 'assert'; + +const expectedMessage = [ + 'Command failed: node --es-module-specifier-resolution=node', + ' --experimental-specifier-resolution=node\n', + 'node: bad option: cannot use --es-module-specifier-resolution', + 'and --experimental-specifier-resolution at the same time' +].join(' '); + +function handleError(error, stdout, stderr) { + ok(error); + strictEqual(error.message, expectedMessage); +} + +const flags = [ + '--es-module-specifier-resolution=node', + '--experimental-specifier-resolution=node' +].join(' '); + +exec(`${process.execPath} ${flags}`, { + timeout: 300 +}, handleError); diff --git a/test/es-module/test-esm-specifiers-legacy-flag.mjs b/test/es-module/test-esm-specifiers-legacy-flag.mjs new file mode 100644 index 00000000000000..fcf0c915b649f0 --- /dev/null +++ b/test/es-module/test-esm-specifiers-legacy-flag.mjs @@ -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'); diff --git a/test/es-module/test-esm-specifiers.mjs b/test/es-module/test-esm-specifiers.mjs index 3e7bc181962f4f..383aff849a21aa 100644 --- a/test/es-module/test-esm-specifiers.mjs +++ b/test/es-module/test-esm-specifiers.mjs @@ -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'; From 60a9dcbd6e6bf1e74b20bc2d154cade1846ee687 Mon Sep 17 00:00:00 2001 From: Myles Borins Date: Wed, 27 Nov 2019 03:22:21 -0500 Subject: [PATCH 2/6] fixup: add experimental warning --- lib/internal/modules/esm/default_resolve.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/internal/modules/esm/default_resolve.js b/lib/internal/modules/esm/default_resolve.js index 977a62c363a898..c9ef3883c40412 100644 --- a/lib/internal/modules/esm/default_resolve.js +++ b/lib/internal/modules/esm/default_resolve.js @@ -110,10 +110,14 @@ function resolve(specifier, parentURL) { if (ext === '.js' || (!format && isMain)) format = getPackageType(url.href) === TYPE_MODULE ? 'module' : 'commonjs'; if (!format) { - if (experimentalSpeciferResolution === '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 }; } From db9f3ec09cde4e4c06a654a585da6d8d17b3e76b Mon Sep 17 00:00:00 2001 From: Myles Borins Date: Wed, 27 Nov 2019 04:05:32 -0500 Subject: [PATCH 3/6] fixup: support both flags and give proper warnings --- src/node_options.cc | 20 ++++++++++++- src/node_options.h | 1 + .../test-esm-specifiers-both-flags.mjs | 28 +++++++------------ 3 files changed, 30 insertions(+), 19 deletions(-) diff --git a/src/node_options.cc b/src/node_options.cc index 90abfcf17efa9a..60d773cc906fcd 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -127,7 +127,20 @@ void EnvironmentOptions::CheckOptions(std::vector* errors) { } } - if (!experimental_specifier_resolution.empty()) { + if (!es_module_specifier_resolution.empty()) { + 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( @@ -367,6 +380,11 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() { "either 'explicit' (default) or 'node'", &EnvironmentOptions::experimental_specifier_resolution, kAllowedInEnvironment); + AddOption("--es-module-specifier-resolution", + "Select extension resolution algorithm for es modules; " + "either 'explicit' (default) or 'node'", + &EnvironmentOptions::es_module_specifier_resolution, + kAllowedInEnvironment); AddOption("--no-deprecation", "silence deprecation warnings", &EnvironmentOptions::no_deprecation, diff --git a/src/node_options.h b/src/node_options.h index 44f494810c96ee..fcf949df46db61 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -105,6 +105,7 @@ class EnvironmentOptions : public Options { 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; std::string experimental_policy; diff --git a/test/es-module/test-esm-specifiers-both-flags.mjs b/test/es-module/test-esm-specifiers-both-flags.mjs index 0b5816676475b1..95287301a73c7f 100644 --- a/test/es-module/test-esm-specifiers-both-flags.mjs +++ b/test/es-module/test-esm-specifiers-both-flags.mjs @@ -1,24 +1,16 @@ -import '../common/index.mjs'; +import { mustCall } from '../common/index.mjs'; import { exec } from 'child_process'; -import { ok, strictEqual } from 'assert'; +import assert from 'assert'; -const expectedMessage = [ - 'Command failed: node --es-module-specifier-resolution=node', - ' --experimental-specifier-resolution=node\n', - 'node: bad option: cannot use --es-module-specifier-resolution', - 'and --experimental-specifier-resolution at the same time' -].join(' '); +const expectedError = + 'node: bad option: cannot use --es-module-specifier-resolution ' + + 'and --experimental-specifier-resolution at the same time'; -function handleError(error, stdout, stderr) { - ok(error); - strictEqual(error.message, expectedMessage); -} - -const flags = [ - '--es-module-specifier-resolution=node', - '--experimental-specifier-resolution=node' -].join(' '); +const flags = '--es-module-specifier-resolution=node ' + + '--experimental-specifier-resolution=node'; exec(`${process.execPath} ${flags}`, { timeout: 300 -}, handleError); +}, mustCall((error) => { + assert(error.message.includes(expectedError)); +})); From f4eb371483ce9fb5eb942ad28bb9457fe8306db7 Mon Sep 17 00:00:00 2001 From: Myles Borins Date: Wed, 27 Nov 2019 04:32:12 -0500 Subject: [PATCH 4/6] fixup: cleanup cli help / man data --- doc/node.1 | 6 +++--- src/node_options.cc | 3 +-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/doc/node.1 b/doc/node.1 index e3628034e832e5..ebf4b526ea7e06 100644 --- a/doc/node.1 +++ b/doc/node.1 @@ -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. . @@ -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 diff --git a/src/node_options.cc b/src/node_options.cc index 60d773cc906fcd..83175bfb7a8998 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -381,8 +381,7 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() { &EnvironmentOptions::experimental_specifier_resolution, kAllowedInEnvironment); AddOption("--es-module-specifier-resolution", - "Select extension resolution algorithm for es modules; " - "either 'explicit' (default) or 'node'", + "", &EnvironmentOptions::es_module_specifier_resolution, kAllowedInEnvironment); AddOption("--no-deprecation", From 118171cc2ab1163502cb9db4951099557f3dd741 Mon Sep 17 00:00:00 2001 From: Myles Borins Date: Thu, 28 Nov 2019 03:15:35 -0500 Subject: [PATCH 5/6] fixup: fix broken tests --- test/parallel/test-process-env-allowed-flags-are-documented.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/parallel/test-process-env-allowed-flags-are-documented.js b/test/parallel/test-process-env-allowed-flags-are-documented.js index 2e0d67eefcb242..f356f88fe9c740 100644 --- a/test/parallel/test-process-env-allowed-flags-are-documented.js +++ b/test/parallel/test-process-env-allowed-flags-are-documented.js @@ -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')); From 7bf301ebd8dbd955bbbaa2a00e8323b4c82a0188 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Tue, 3 Dec 2019 16:41:15 -0500 Subject: [PATCH 6/6] windows possible fix --- test/es-module/test-esm-specifiers-both-flags.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/es-module/test-esm-specifiers-both-flags.mjs b/test/es-module/test-esm-specifiers-both-flags.mjs index 95287301a73c7f..fc5c7fcd0e98a9 100644 --- a/test/es-module/test-esm-specifiers-both-flags.mjs +++ b/test/es-module/test-esm-specifiers-both-flags.mjs @@ -3,7 +3,7 @@ import { exec } from 'child_process'; import assert from 'assert'; const expectedError = - 'node: bad option: cannot use --es-module-specifier-resolution ' + + 'cannot use --es-module-specifier-resolution ' + 'and --experimental-specifier-resolution at the same time'; const flags = '--es-module-specifier-resolution=node ' +