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. PR-URL: #30678 Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Guy Bedford <[email protected]>
nodejs · Jan 12, 2020 · 0a4cfef · 0a4cfef
1 parent d7972b7
commit 0a4cfef
Show file tree

Hide file tree

Showing 11 changed files with 95 additions and 31 deletions.
diff --git 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`
-<!-- YAML
-added: v12.0.0
--->
-
-To be used in conjunction with `--experimental-modules`. Sets the resolution
-algorithm for resolving 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: REPLACEME
@@ -222,6 +208,20 @@ added: REPLACEME
 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
@@ -1072,7 +1072,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`
@@ -1081,6 +1080,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
@@ -1375,7 +1375,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
@@ -1386,7 +1386,7 @@ $ node --experimental-modules index.mjs
 success!
 $ node --experimental-modules index #Failure!
 Error: Cannot find module
-$ node --experimental-modules --es-module-specifier-resolution=node index
+$ node --experimental-modules --experimental-specifier-resolution=node index
 success!
 ```
 

diff --git 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/lib/internal/modules/esm/default_resolve.js b/lib/internal/modules/esm/default_resolve.js
@@ -9,8 +9,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,
@@ -108,10 +108,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 };
 }

diff --git a/src/module_wrap.cc b/src/module_wrap.cc
@@ -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;
@@ -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 {

diff --git a/src/node_options.cc b/src/node_options.cc
@@ -155,9 +155,27 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors) {
       errors->push_back("--es-module-specifier-resolution requires "
                         "--experimental-modules be enabled");
     }
-    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_modules) {
+      errors->push_back("--experimental-specifier-resolution requires "
+                        "--experimental-modules be enabled");
+    }
+    if (experimental_specifier_resolution != "node" &&
+        experimental_specifier_resolution != "explicit") {
+      errors->push_back(
+        "invalid value for --experimental-specifier-resolution");
     }
   }
 
@@ -404,9 +422,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",

diff --git 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_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;

diff --git a/test/es-module/test-esm-specifiers-both-flags.mjs b/test/es-module/test-esm-specifiers-both-flags.mjs
@@ -0,0 +1,18 @@
+// Flags: --experimental-modules
+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 = '--experimental-modules ' +
+              '--es-module-specifier-resolution=node ' +
+              '--experimental-specifier-resolution=node';
+
+exec(`${process.execPath} ${flags}`, {
+  timeout: 300
+}, mustCall((error) => {
+  assert(error.message.includes(expectedError));
+}));
diff --git a/test/es-module/test-esm-specifiers-legacy-flag.mjs b/test/es-module/test-esm-specifiers-legacy-flag.mjs
@@ -0,0 +1,18 @@
+// Flags: --experimental-modules --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
@@ -1,4 +1,4 @@
-// Flags: --experimental-modules --es-module-specifier-resolution=node
+// Flags: --experimental-modules --experimental-specifier-resolution=node
 import { mustNotCall } from '../common/index.mjs';
 import assert from 'assert';
 

diff --git 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'));