folder main reversion, error unification

Author: guybedford

doc/api/errors.md

@@ -1369,26 +1369,19 @@ a `dynamicInstantiate` hook.
 A `MessagePort` was found in the object passed to a `postMessage()` call,
 but not provided in the `transferList` for that call.
 
-<a id="ERR_MISSING_MODULE"></a>
-### ERR_MISSING_MODULE
-
-> Stability: 1 - Experimental
-
-An [ES6 module][] could not be resolved.
-
 <a id="ERR_MISSING_PLATFORM_FOR_WORKER"></a>
 ### ERR_MISSING_PLATFORM_FOR_WORKER
 
 The V8 platform used by this instance of Node.js does not support creating
 Workers. This is caused by lack of embedder support for Workers. In particular,
 this error will not occur with standard builds of Node.js.
 
-<a id="ERR_MODULE_RESOLUTION_LEGACY"></a>
-### ERR_MODULE_RESOLUTION_LEGACY
+<a id="ERR_MODULE_NOT_FOUND"></a>
+### ERR_MODULE_NOT_FOUND
 
 > Stability: 1 - Experimental
 
-A failure occurred resolving imports in an [ES6 module][].
+An [ESM module][] could not be resolved.
 
 <a id="ERR_MULTIPLE_CALLBACK"></a>
 ### ERR_MULTIPLE_CALLBACK

doc/api/esm.md

@@ -152,6 +152,7 @@ The resolver has the following properties:
 * Support for builtin module loading
 * Relative and absolute URL resolution
 * No default extensions
+* No folder mains
 * Bare specifier package resolution lookup through node_modules
 * Avoids the package fallback which breaks package isolation. This is the
   problem that an import to `x/y.js` will keep checking subsequent node_modules
@@ -174,59 +175,43 @@ Relative and absolute resolution without directory or extension resolution
 (URL resolution):
 
 * ESM_RESOLVE("./a.asdf", "file:///parent/path") -> "file:///parent/a.asdf"
-* ESMRESOLVE("/a", "file:///parent/path") -> "file:///a"
-* ESMRESOLVE("file:///a", "file:///parent/path") -> "file:///a"
+* ESM_RESOLVE("/a", "file:///parent/path") -> "file:///a"
+* ESM_RESOLVE("file:///a", "file:///parent/path") -> "file:///a"
 
 Package resolution:
 
-1. ESMRESOLVE("pkg/x", "file:///path/to/project") ->
+1. ESM_RESOLVE("pkg/x", "file:///path/to/project") ->
    "file:///path/to/nodemodules/pkg/x" (if it exists)
-1. ESMRESOLVE("pkg/x", "file:///path/to/project") ->
+1. ESM_RESOLVE("pkg/x", "file:///path/to/project") ->
    Not Found otherwise, even if "file:///path/nodemodules/pkg/x" exists.
 
-Main resolution:
-
-1. ESMRESOLVE("pkg", "file:///path/to/project") -> 
-   "file:///path/to/project/nodemodules/pkg/index.mjs" (if it exists)
-1. ESMRESOLVE("pkg", "file:///path/to/project") -> Not Found otherwise,
-   even if "file:///path/to/nodemodules/pkg/index.mjs" exists.
-1. ESMRESOLVE("file:///path/to/project") ->
-   "file:///path/to/project/index.mjs" (if it exists)
-1. ESMRESOLVE("file:///path/to/project") ->
-   Not Found, if "file:///path/to/project/index.mjs" does not exist.
-
 ### Resolver Algorithm
 
 The algorithm to resolve an ES module specifier is provided through
 _ESM_RESOLVE_:
 
 **ESM_RESOLVE**(_specifier_, _parentURL_)
+> 1. Let _resolvedURL_ be _undefined_.
 > 1. If _specifier_ is a valid URL then,
->    1. Let _resolvedURL_ be the result of parsing and reserializing
+>    1. Set _resolvedURL_ to the result of parsing and reserializing
 >       _specifier_ as a URL.
->    1. Return the result of **PATH_RESOLVE**(_resolvedURL_).
-> 1. If _specifier_ starts with _"/"_, _"./"_ or _"../"_ then,
->    1. Let _resolvedURL_ be the URL resolution of _specifier_ to _parentURL_.
->    1. Return the result of **PATH_RESOLVE**(_resolvedURL_).
-> 1. Note: _specifier_ is now a bare specifier.
-> 1. Return the result of **PACKAGE_RESOLVE**(_specifier_, _parentURL_).
-
-**PATH_RESOLVE**(_resolvedURL_)
-> 1. If the file at _resolvedURL_ exists then,
->    1. Return _resolvedURL_.
-> 1. Let _packageMainURL_ be the result of
->    **PACKAGE_MAIN_RESOLVE**(_resolvedURL_).
-> 1. If _packageMainURL_ is not _undefined_ then,
->    1. Return _packageMainURL_.
-> 1. Throw a _Module Not Found_ error.
+> 1. Otherwise, if _specifier_ starts with _"/"_, _"./"_ or _"../"_ then,
+>    1. Set _resolvedURL_ to the URL resolution of _specifier_ to _parentURL_.
+> 1. Otherwise,
+>    1. Note: _specifier_ is now a bare specifier.
+>    1. Set _resolvedURL_ the result of
+>       **PACKAGE_RESOLVE**(_specifier_, _parentURL_).
+> 1. If the file at _resolvedURL_ does not exist then,
+>    1. Throw a _Module Not Found_ error.
+> 1. Return _resolvedURL_.
 
 **PACKAGE_RESOLVE**(_packageSpecifier_, _parentURL_)
 > 1. Assert: _packageSpecifier_ is a bare specifier.
-> 1. If _packageSpecifier_ is an empty string then,
->    1. Throw a _Module Not Found_ error.
 > 1. Let _packageName_ be _undefined_.
 > 1. Let _packagePath_ be _undefined_.
 > 1. If _packageSpecifier_ does not start with _"@"_ then,
+>    1. If _packageSpecifier_ is an empty string then,
+>       1. Throw a _Invalid Package Name_ error.
 >    1. Set _packageName_ to the substring of _packageSpecifier_ until the
 >       first _"/"_ separator or the end of the string.
 > 1. If _packageSpecifier_ starts with _"@"_ then,
@@ -239,7 +224,7 @@ _ESM_RESOLVE_:
 > 1. Assert: _packageName_ is a valid package name or scoped package name.
 > 1. Assert: _packagePath_ is either empty, or a path without a leading
 >    separator.
-> 1. Note: Further package name encoding validations can be added here.
+> 1. Note: Further package name validations can be added here.
 > 1. If _packagePath_ is empty and _packageName_ is a Node.js builtin
 >    module then,
 >    1. Return _"node:${packageName}"_.
@@ -254,23 +239,15 @@ _ESM_RESOLVE_:
 >       1. Continue the next loop iteration.
 >    1. If _packagePath_ is empty then,
 >       1. Let _url_ be the result of **PACKAGE_MAIN_RESOLVE**(_packageURL_).
->       1. If _url_ is not _undefined_ then,
->          1. Return _url_.
+>       1. If _url_ is _undefined_ then,
+>          1. Throw a _Module Not Found_ error.
+>       1. Return _url_.
 >    1. Otherwise,
->       1. Let _url_ be equal to the URL resolution of _packagePath_ in
->          _packageURL_.
->       1. If the file at _url_ exists then,
->          1. Return _url_.
->       1. Otherwise, if _url_ is a directory then,
->          1. Let _url_ be the result of **PACKAGE_MAIN_RESOLVE**(_url_).
->          1. If _url_ is not _undefined_ then,
->             1. Return _url_.
->    1. Throw a _Module Not Found_ error.
+>       1. Return the URL resolution of _packagePath_ in _packageURL_.
 > 1. Throw a _Module Not Found_ error.
 
 **PACKAGE_MAIN_RESOLVE**(_packageURL_)
-> 1. If the file at _"${packageURL}/index.mjs"_ exists then,
->    1. Return _"${packageURL}/index.mjs"_.
+> 1. Note: Main resolution to be implemented here.
 > 1. Return _undefined_.
 
 

lib/internal/errors.js

@@ -762,11 +762,13 @@ E('ERR_MISSING_ARGS',
 E('ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK',
   'The ES Module loader may not return a format of \'dynamic\' when no ' +
   'dynamicInstantiate function was provided', Error);
-E('ERR_MISSING_MODULE', 'Cannot find module %s', Error);
-E('ERR_MODULE_RESOLUTION_LEGACY',
-  'Cannot find module \'%s\' imported from %s.' +
-    ' Legacy behavior in require() would have found it at %s',
-  Error);
+E('ERR_MODULE_NOT_FOUND', (module, base, legacyResolution) => {
+  let msg = `Cannot find module '${module}' imported from ${base}.`;
+  if (legacyResolution)
+    msg += ' Legacy behavior in require() would have found it at ' +
+           legacyResolution;
+  return msg;
+}, Error);
 E('ERR_MULTIPLE_CALLBACK', 'Callback called multiple times', Error);
 E('ERR_NAPI_CONS_FUNCTION', 'Constructor must be a function', TypeError);
 E('ERR_NAPI_INVALID_DATAVIEW_ARGS',
@@ -859,7 +861,8 @@ E('ERR_UNKNOWN_CREDENTIAL', '%s identifier does not exist: %s', Error);
 E('ERR_UNKNOWN_ENCODING', 'Unknown encoding: %s', TypeError);
 
 // This should probably be a `TypeError`.
-E('ERR_UNKNOWN_FILE_EXTENSION', 'Unknown file extension: %s', Error);
+E('ERR_UNKNOWN_FILE_EXTENSION', 'Unknown file extension: \'%s\' imported ' +
+  'from %s', Error);
 E('ERR_UNKNOWN_MODULE_FORMAT', 'Unknown module format: %s', RangeError);
 E('ERR_UNKNOWN_SIGNAL', 'Unknown signal: %s', TypeError);
 E('ERR_UNKNOWN_STDIN_TYPE', 'Unknown stdin file type', Error);

lib/internal/modules/esm/default_resolve.js

@@ -9,8 +9,7 @@ const { realpathSync } = require('fs');
 const preserveSymlinks = !!process.binding('config').preserveSymlinks;
 const preserveSymlinksMain = !!process.binding('config').preserveSymlinksMain;
 const {
-  ERR_MISSING_MODULE,
-  ERR_MODULE_RESOLUTION_LEGACY,
+  ERR_MODULE_NOT_FOUND,
   ERR_UNKNOWN_FILE_EXTENSION
 } = require('internal/errors').codes;
 const { resolve: moduleWrapResolve } = internalBinding('module_wrap');
@@ -19,10 +18,6 @@ const { pathToFileURL, fileURLToPath } = require('internal/url');
 const realpathCache = new Map();
 
 function search(target, base) {
-  if (base === undefined) {
-    // We cannot search without a base.
-    throw new ERR_MISSING_MODULE(target);
-  }
   try {
     return moduleWrapResolve(target, base);
   } catch (e) {
@@ -34,8 +29,7 @@ function search(target, base) {
       tmpMod.paths = CJSmodule._nodeModulePaths(
         new URL('./', questionedBase).pathname);
       const found = CJSmodule._resolveFilename(target, tmpMod);
-      error = new ERR_MODULE_RESOLUTION_LEGACY(target,
-                                               fileURLToPath(base), found);
+      error = new ERR_MODULE_NOT_FOUND(target, fileURLToPath(base), found);
     } catch {
       // ignore
     }
@@ -78,12 +72,11 @@ function resolve(specifier, parentURL) {
     if (isMain)
       format = 'cjs';
     else
-      throw new ERR_UNKNOWN_FILE_EXTENSION(url.pathname);
+      throw new ERR_UNKNOWN_FILE_EXTENSION(url.pathname,
+                                           fileURLToPath(parentURL));
   }
 
   return { url: `${url}`, format };
 }
 
 module.exports = resolve;
-// exported for tests
-module.exports.search = search;

src/module_wrap.cc

@@ -486,26 +486,21 @@ DescriptorType CheckDescriptor(const std::string& path) {
 }
 
 inline Maybe<URL> PackageMainResolve(const URL& search) {
-  std::string path = search.path();
-  std::string last_segment = path.substr(path.find_last_of('/') + 1);
-  URL main_url = URL("./" + last_segment + "/index.mjs", search);
-  if (CheckDescriptor(main_url.ToFilePath()) == FILE)
-    return Just(main_url);
   return Nothing<URL>();
 }
 
 Maybe<URL> PackageResolve(Environment* env,
                           const std::string& specifier,
                           const URL& base) {
-  if (specifier.length() == 0) return Nothing<URL>();
   size_t sep_index = specifier.find('/');
+  if (specifier[0] == '@' && sep_index == std::string::npos ||
+      specifier.length() == 0) {
+    std::string msg = "Invalid package name '" + specifier +
+      "' imported from " + base.ToFilePath();
+    node::THROW_ERR_INVALID_PACKAGE_NAME(env, msg.c_str());
+    return Nothing<URL>();
+  }
   if (specifier[0] == '@') {
-    if (sep_index == std::string::npos) {
-      std::string msg = "Invalid package name '" + specifier +
-        "' imported from " + base.ToFilePath();
-      node::THROW_ERR_INVALID_PACKAGE_NAME(env, msg.c_str());
-      return Nothing<URL>();
-    }
     sep_index = specifier.find('/', sep_index + 1);
   }
   std::string pkg_name = specifier.substr(0,
@@ -527,24 +522,21 @@ Maybe<URL> PackageResolve(Environment* env,
     } else {
       url = pkg_url;
     }
+    DescriptorType check;
     if (pkg_path.length()) {
       DescriptorType check = CheckDescriptor(url.ToFilePath());
-      if (check == FILE) {
-        return Just(url);
-      } else if (check == DIRECTORY) {
-        Maybe<URL> main_url = PackageMainResolve(url);
-        if (!main_url.IsNothing()) return main_url;
-      }
+      if (check == FILE) return Just(url);
+      if (check == NONE) check = CheckDescriptor(pkg_url.ToFilePath());
     } else {
       Maybe<URL> main_url = PackageMainResolve(url);
       if (!main_url.IsNothing()) return main_url;
+      check = CheckDescriptor(pkg_url.ToFilePath());
     }
     // throw not found if we did match a package directory
-    DescriptorType check = CheckDescriptor(pkg_url.ToFilePath());
     if (check == DIRECTORY) {
       std::string msg = "Cannot find module '" + url.ToFilePath() +
           "' imported from " + base.ToFilePath();
-      node::THROW_ERR_MISSING_MODULE(env, msg.c_str());
+      node::THROW_ERR_MODULE_NOT_FOUND(env, msg.c_str());
       return Nothing<URL>();
     }
     last_path = parent.path();
@@ -554,38 +546,32 @@ Maybe<URL> PackageResolve(Environment* env,
   return Nothing<URL>();
 }
 
-Maybe<URL> PathResolve(Environment* env,
-                       const URL& url,
-                       const URL& base) {
-  std::string path = url.ToFilePath();
-  DescriptorType check = CheckDescriptor(path);
-  if (check == FILE) return Just(url);
-
-  if (check == DIRECTORY) {
-    Maybe<URL> url_main = PackageMainResolve(url);
-    if (!url_main.IsNothing()) return url_main;
-  }
-
-  std::string msg = "Cannot find module '" + path +
-      "' imported from " + base.ToFilePath();
-  node::THROW_ERR_MISSING_MODULE(env, msg.c_str());
-  return Nothing<URL>();
-}
-
 }  // anonymous namespace
 
 Maybe<URL> Resolve(Environment* env,
                    const std::string& specifier,
                    const URL& base) {
-  URL pure_url(specifier);
-  if (!(pure_url.flags() & URL_FLAGS_FAILED)) {
-    return PathResolve(env, pure_url, base);
-  }
+  // Order swapped from spec for minor perf gain.
+  // Ok since relative URLs cannot parse as URLs.
+  URL resolved;
   if (ShouldBeTreatedAsRelativeOrAbsolutePath(specifier)) {
-    URL resolved(specifier, base);
-    return PathResolve(env, resolved, base);
+    resolved = URL(specifier, base);
+  } else {
+    URL pure_url(specifier);
+    if (!(pure_url.flags() & URL_FLAGS_FAILED)) {
+      resolved = pure_url;
+    } else {
+      return PackageResolve(env, specifier, base);
+    }
+  }
+  DescriptorType check = CheckDescriptor(resolved.ToFilePath());
+  if (check != FILE) {
+    std::string msg = "Cannot find module '" + resolved.ToFilePath() +
+          "' imported from " + base.ToFilePath();
+    node::THROW_ERR_MODULE_NOT_FOUND(env, msg.c_str());
+    return Nothing<URL>();
   }
-  return PackageResolve(env, specifier, base);
+  return Just(resolved);
 }
 
 void ModuleWrap::Resolve(const FunctionCallbackInfo<Value>& args) {
@@ -616,7 +602,7 @@ void ModuleWrap::Resolve(const FunctionCallbackInfo<Value>& args) {
              (result.FromJust().flags() & URL_FLAGS_FAILED)) {
     std::string msg = "Cannot find module '" + specifier_std +
         "' imported from " + url.ToFilePath();
-    node::THROW_ERR_MISSING_MODULE(env, msg.c_str());
+    node::THROW_ERR_MODULE_NOT_FOUND(env, msg.c_str());
     try_catch.ReThrow();
     return;
   }

src/node_errors.h

@@ -33,8 +33,8 @@ namespace node {
   V(ERR_MEMORY_ALLOCATION_FAILED, Error)                                     \
   V(ERR_MISSING_ARGS, TypeError)                                             \
   V(ERR_MISSING_MESSAGE_PORT_IN_TRANSFER_LIST, TypeError)                    \
-  V(ERR_MISSING_MODULE, Error)                                               \
   V(ERR_MISSING_PLATFORM_FOR_WORKER, Error)                                  \
+  V(ERR_MODULE_NOT_FOUND, Error)                                             \
   V(ERR_OUT_OF_RANGE, RangeError)                                            \
   V(ERR_SCRIPT_EXECUTION_INTERRUPTED, Error)                                 \
   V(ERR_SCRIPT_EXECUTION_TIMEOUT, Error)                                     \

test/es-module/test-esm-dynamic-import.js

@@ -19,7 +19,7 @@ function expectErrorProperty(result, propertyKey, value) {
 }
 
 function expectMissingModuleError(result) {
-  expectErrorProperty(result, 'code', 'MODULE_NOT_FOUND');
+  expectErrorProperty(result, 'code', 'ERR_MODULE_NOT_FOUND');
 }
 
 function expectInvalidUrlError(result) {