esm: minimal resolver spec and implementation refinements

PR URL: #12

Author: guybedford

doc/api/errors.md

@@ -1414,26 +1414,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

@@ -63,10 +63,6 @@ ESM must have the `.mjs` extension.
 
 You must provide a file extension when using the `import` keyword.
 
-### No importing directories
-
-There is no support for importing directories.
-
 ### No NODE_PATH
 
 `NODE_PATH` is not part of resolving `import` specifiers. Please use symlinks
@@ -146,6 +142,52 @@ fs.readFileSync = () => Buffer.from('Hello, ESM');
 fs.readFileSync === readFileSync;
 ```
 
+## Resolution Algorithm
+
+### Features
+
+The resolver has the following properties:
+
+* FileURL-based resolution as is used by ES modules
+* Support for builtin module loading
+* Relative and absolute URL resolution
+* No default extensions
+* No folder mains
+* Bare specifier package resolution lookup through node_modules
+
+### 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. Set _resolvedURL_ to the result of parsing and reserializing
+>       _specifier_ as a URL.
+> 1. Otherwise, if _specifier_ starts with _"/"_, _"./"_ or _"../"_ then,
+>    1. Set _resolvedURL_ to the URL resolution of _specifier_ relative 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 a Node.js builtin module then,
+>    1. Return the string _"node:"_ concatenated with _packageSpecifier_.
+> 1. While _parentURL_ contains a non-empty _pathname_,
+>    1. Set _parentURL_ to the parent folder URL of _parentURL_.
+>    1. Let _packageURL_ be the URL resolution of the string concatenation of
+>       _parentURL_, _"/node_modules/"_ and _"packageSpecifier"_.
+>    1. If the file at _packageURL_ exists then,
+>       1. Return _packageURL_.
+> 1. Throw a _Module Not Found_ error.
+
 [Node.js EP for ES Modules]: https://github.com/nodejs/node-eps/blob/master/002-es-modules.md
 [`module.createRequireFromPath()`]: modules.html#modules_module_createrequirefrompath_filename
 [ESM Minimal Kernel]: https://github.com/nodejs/modules/blob/master/doc/plan-for-new-modules-implementation.md

lib/internal/errors.js

@@ -842,11 +842,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',
-  '%s not found by import in %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',
@@ -940,11 +942,13 @@ E('ERR_UNHANDLED_ERROR',
     if (err === undefined) return msg;
     return `${msg} (${err})`;
   }, Error);
+// This should probably be a `TypeError`.
 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);
 

lib/internal/modules/esm/default_resolve.js

@@ -10,21 +10,15 @@ const { getOptionValue } = require('internal/options');
 const preserveSymlinks = getOptionValue('--preserve-symlinks');
 const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main');
 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');
-const StringStartsWith = Function.call.bind(String.prototype.startsWith);
 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) {
@@ -36,7 +30,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, base, found);
+      error = new ERR_MODULE_NOT_FOUND(target, fileURLToPath(base), found);
     } catch {
       // ignore
     }
@@ -57,16 +51,8 @@ function resolve(specifier, parentURL) {
     };
   }
 
-  let url;
-  try {
-    url = search(specifier,
-                 parentURL || pathToFileURL(`${process.cwd()}/`).href);
-  } catch (e) {
-    if (typeof e.message === 'string' &&
-        StringStartsWith(e.message, 'Cannot find module'))
-      e.code = 'MODULE_NOT_FOUND';
-    throw e;
-  }
+  let url = search(specifier,
+                   parentURL || pathToFileURL(`${process.cwd()}/`).href);
 
   const isMain = parentURL === undefined;
 
@@ -87,12 +73,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

@@ -467,104 +467,37 @@ std::string ReadFile(uv_file file) {
   return contents;
 }
 
-enum CheckFileOptions {
-  LEAVE_OPEN_AFTER_CHECK,
-  CLOSE_AFTER_CHECK
+enum DescriptorType {
+  NONE,
+  FILE,
+  DIRECTORY
 };
 
-Maybe<uv_file> CheckFile(const std::string& path,
-                         CheckFileOptions opt = CLOSE_AFTER_CHECK) {
+DescriptorType CheckDescriptor(const std::string& path) {
   uv_fs_t fs_req;
-  if (path.empty()) {
-    return Nothing<uv_file>();
-  }
-
-  uv_file fd = uv_fs_open(nullptr, &fs_req, path.c_str(), O_RDONLY, 0, nullptr);
-  uv_fs_req_cleanup(&fs_req);
-
-  if (fd < 0) {
-    return Nothing<uv_file>();
-  }
-
-  uv_fs_fstat(nullptr, &fs_req, fd, nullptr);
-  uint64_t is_directory = fs_req.statbuf.st_mode & S_IFDIR;
-  uv_fs_req_cleanup(&fs_req);
-
-  if (is_directory) {
-    CHECK_EQ(0, uv_fs_close(nullptr, &fs_req, fd, nullptr));
-    uv_fs_req_cleanup(&fs_req);
-    return Nothing<uv_file>();
-  }
-
-  if (opt == CLOSE_AFTER_CHECK) {
-    CHECK_EQ(0, uv_fs_close(nullptr, &fs_req, fd, nullptr));
+  int rc = uv_fs_stat(nullptr, &fs_req, path.c_str(), nullptr);
+  if (rc == 0) {
+    uint64_t is_directory = fs_req.statbuf.st_mode & S_IFDIR;
     uv_fs_req_cleanup(&fs_req);
+    return is_directory ? DIRECTORY : FILE;
   }
-
-  return Just(fd);
-}
-
-enum ResolveExtensionsOptions {
-  TRY_EXACT_NAME,
-  ONLY_VIA_EXTENSIONS
-};
-
-inline bool ResolvesToFile(const URL& search) {
-  std::string filePath = search.ToFilePath();
-  Maybe<uv_file> check = CheckFile(filePath);
-  return !check.IsNothing();
-}
-
-template <ResolveExtensionsOptions options>
-Maybe<URL> ResolveExtensions(const URL& search) {
-  if (options == TRY_EXACT_NAME) {
-    std::string filePath = search.ToFilePath();
-    Maybe<uv_file> check = CheckFile(filePath);
-    if (!check.IsNothing()) {
-      return Just(search);
-    }
-  }
-
-  for (const char* extension : EXTENSIONS) {
-    URL guess(search.path() + extension, &search);
-    Maybe<uv_file> check = CheckFile(guess.ToFilePath());
-    if (!check.IsNothing()) {
-      return Just(guess);
-    }
-  }
-
-  return Nothing<URL>();
-}
-
-inline Maybe<URL> ResolveIndex(const URL& search) {
-  return ResolveExtensions<ONLY_VIA_EXTENSIONS>(URL("index", search));
+  uv_fs_req_cleanup(&fs_req);
+  return NONE;
 }
 
-Maybe<URL> ResolveModule(Environment* env,
-                         const std::string& specifier,
-                         const URL& base) {
+Maybe<URL> PackageResolve(Environment* env,
+                          const std::string& specifier,
+                          const URL& base) {
   URL parent(".", base);
-  URL dir("");
+  std::string last_path;
   do {
-    dir = parent;
-    Maybe<URL> check =
-        Resolve(env, "./node_modules/" + specifier, dir);
-    if (!check.IsNothing()) {
-      const size_t limit = specifier.find('/');
-      const size_t spec_len =
-          limit == std::string::npos ? specifier.length() :
-                                       limit + 1;
-      std::string chroot =
-          dir.path() + "node_modules/" + specifier.substr(0, spec_len);
-      if (check.FromJust().path().substr(0, chroot.length()) != chroot) {
-        return Nothing<URL>();
-      }
-      return check;
-    } else {
-      // TODO(bmeck) PREVENT FALLTHROUGH
-    }
-    parent = URL("..", &dir);
-  } while (parent.path() != dir.path());
+    URL pkg_url("./node_modules/" + specifier, &parent);
+    DescriptorType check = CheckDescriptor(pkg_url.ToFilePath());
+    if (check == FILE) return Just(pkg_url);
+    last_path = parent.path();
+    parent = URL("..", &parent);
+    // cross-platform root check
+  } while (parent.path() != last_path);
   return Nothing<URL>();
 }
 
@@ -573,26 +506,27 @@ Maybe<URL> ResolveModule(Environment* env,
 Maybe<URL> Resolve(Environment* env,
                    const std::string& specifier,
                    const URL& base) {
-  URL pure_url(specifier);
-  if (!(pure_url.flags() & URL_FLAGS_FAILED)) {
-    // just check existence, without altering
-    Maybe<uv_file> check = CheckFile(pure_url.ToFilePath());
-    if (check.IsNothing()) {
-      return Nothing<URL>();
+  // Order swapped from spec for minor perf gain.
+  // Ok since relative URLs cannot parse as URLs.
+  URL resolved;
+  if (ShouldBeTreatedAsRelativeOrAbsolutePath(specifier)) {
+    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);
     }
-    return Just(pure_url);
   }
-  if (specifier.length() == 0) {
+  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>();
   }
-  if (ShouldBeTreatedAsRelativeOrAbsolutePath(specifier)) {
-    URL resolved(specifier, base);
-    if (ResolvesToFile(resolved))
-      return Just(resolved);
-    return Nothing<URL>();
-  } else {
-    return ResolveModule(env, specifier, base);
-  }
+  return Just(resolved);
 }
 
 void ModuleWrap::Resolve(const FunctionCallbackInfo<Value>& args) {
@@ -614,10 +548,18 @@ void ModuleWrap::Resolve(const FunctionCallbackInfo<Value>& args) {
         env, "second argument is not a URL string");
   }
 
+  TryCatchScope try_catch(env);
   Maybe<URL> result = node::loader::Resolve(env, specifier_std, url);
-  if (result.IsNothing() || (result.FromJust().flags() & URL_FLAGS_FAILED)) {
-    std::string msg = "Cannot find module " + specifier_std;
-    return node::THROW_ERR_MISSING_MODULE(env, msg.c_str());
+  if (try_catch.HasCaught()) {
+    try_catch.ReThrow();
+    return;
+  } else if (result.IsNothing() ||
+             (result.FromJust().flags() & URL_FLAGS_FAILED)) {
+    std::string msg = "Cannot find module '" + specifier_std +
+        "' imported from " + url.ToFilePath();
+    node::THROW_ERR_MODULE_NOT_FOUND(env, msg.c_str());
+    try_catch.ReThrow();
+    return;
   }
 
   MaybeLocal<Value> obj = result.FromJust().ToObject(env);

src/node_errors.h

@@ -63,8 +63,8 @@ void FatalException(const v8::FunctionCallbackInfo<v8::Value>& args);
   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)                                     \