module: self resolve bug fixes and esm ordering

doc/api/esm.md

@@ -1206,6 +1206,9 @@ _defaultEnv_ is the conditional environment name priority array,
 > 1. If _packageSubpath_ contains any _"."_ or _".."_ segments or percent
 >    encoded strings for _"/"_ or _"\\"_, then
 >    1. Throw an _Invalid Specifier_ error.
+> 1. Set _selfUrl_ to the result of
+>    **SELF_REFERENCE_RESOLE**(_packageName_, _packageSubpath_, _parentURL_).
+> 1. If _selfUrl_ isn't empty, return _selfUrl_.
 > 1. If _packageSubpath_ is _undefined_ and _packageName_ is a Node.js builtin
 >    module, then
 >    1. Return the string _"node:"_ concatenated with _packageSpecifier_.
@@ -1227,29 +1230,24 @@ _defaultEnv_ is the conditional environment name priority array,
 >             1. Return **PACKAGE_EXPORTS_RESOLVE**(_packageURL_,
 >                _packageSubpath_, _pjson.exports_).
 >       1. Return the URL resolution of _packageSubpath_ in _packageURL_.
-> 1. Set _selfUrl_ to the result of
->    **SELF_REFERENCE_RESOLE**(_packageSpecifier_, _parentURL_).
-> 1. If _selfUrl_ isn't empty, return _selfUrl_.
 > 1. Throw a _Module Not Found_ error.
 
-**SELF_REFERENCE_RESOLVE**(_specifier_, _parentURL_)
+**SELF_REFERENCE_RESOLVE**(_packageName_, _packageSubpath_, _parentURL_)
 
 > 1. Let _packageURL_ be the result of **READ_PACKAGE_SCOPE**(_parentURL_).
 > 1. If _packageURL_ is **null**, then
 >    1. Return an empty result.
 > 1. Let _pjson_ be the result of **READ_PACKAGE_JSON**(_packageURL_).
-> 1. Set _name_ to _pjson.name_.
-> 1. If _name_ is empty, then return an empty result.
-> 1. If _name_ is equal to _specifier_, then
->    1. Return the result of **PACKAGE_MAIN_RESOLVE**(_packageURL_, _pjson_).
-> 1. If _specifier_ starts with _name_ followed by "/", then
->    1. Set _subpath_ to everything after the "/".
->    1. If _pjson_ is not **null** and _pjson_ has an _"exports"_ key, then
->       1. Let _exports_ be _pjson.exports_.
->       1. If _exports_ is not **null** or **undefined**, then
->          1. Return **PACKAGE_EXPORTS_RESOLVE**(_packageURL_, _subpath_,
->             _pjson.exports_).
->    1. Return the URL resolution of _subpath_ in _packageURL_.
+> 1. If _pjson.name_ is equal to _packageName_, then
+>    1. If _packageSubpath_ is _undefined_, then
+>       1. Return the result of **PACKAGE_MAIN_RESOLVE**(_packageURL_, _pjson_).
+>    1. Otherwise,
+>       1. If _pjson_ is not **null** and _pjson_ has an _"exports"_ key, then
+>          1. Let _exports_ be _pjson.exports_.
+>          1. If _exports_ is not **null** or **undefined**, then
+>             1. Return **PACKAGE_EXPORTS_RESOLVE**(_packageURL_, _subpath_,
+>                _pjson.exports_).
+>       1. Return the URL resolution of _subpath_ in _packageURL_.
 > 1. Otherwise return an empty result.
 
 **PACKAGE_MAIN_RESOLVE**(_packageURL_, _pjson_)

lib/internal/modules/cjs/loader.js

@@ -431,12 +431,12 @@ function resolveBasePath(basePath, exts, isMain, trailingSlash, request) {
   return filename;
 }
 
-function trySelf(paths, exts, isMain, trailingSlash, request) {
+function trySelf(parentPath, isMain, request) {
   if (!experimentalSelf) {
     return false;
   }
 
-  const { data: pkg, path: basePath } = readPackageScope(paths[0]);
+  const { data: pkg, path: basePath } = readPackageScope(parentPath) || {};
   if (!pkg) return false;
   if (typeof pkg.name !== 'string') return false;
 
@@ -449,12 +449,15 @@ function trySelf(paths, exts, isMain, trailingSlash, request) {
     return false;
   }
 
-  if (exts === undefined)
-    exts = ObjectKeys(Module._extensions);
-
-  if (expansion) {
-    // Use exports
-    const fromExports = applyExports(basePath, expansion);
+  const exts = ObjectKeys(Module._extensions);
+  const fromExports = applyExports(basePath, expansion);
+  // Use exports
+  if (fromExports) {
+    let trailingSlash = request.length > 0 &&
+        request.charCodeAt(request.length - 1) === CHAR_FORWARD_SLASH;
+    if (!trailingSlash) {
+      trailingSlash = /(?:^|\/)\.?\.$/.test(request);
+    }
     return resolveBasePath(fromExports, exts, isMain, trailingSlash, request);
   } else {
     // Use main field
@@ -702,13 +705,6 @@ Module._findPath = function(request, paths, isMain) {
     }
   }
 
-  const selfFilename = trySelf(paths, exts, isMain, trailingSlash, request);
-  if (selfFilename) {
-    emitExperimentalWarning('Package name self resolution');
-    Module._pathCache[cacheKey] = selfFilename;
-    return selfFilename;
-  }
-
   return false;
 };
 
@@ -1008,24 +1004,32 @@ Module._resolveFilename = function(request, parent, isMain, options) {
 
   // Look up the filename first, since that's the cache key.
   const filename = Module._findPath(request, paths, isMain);
-  if (!filename) {
-    const requireStack = [];
-    for (let cursor = parent;
-      cursor;
-      cursor = cursor.parent) {
-      requireStack.push(cursor.filename || cursor.id);
-    }
-    let message = `Cannot find module '${request}'`;
-    if (requireStack.length > 0) {
-      message = message + '\nRequire stack:\n- ' + requireStack.join('\n- ');
+  if (filename) return filename;
+  if (parent && parent.filename) {
+    const filename = trySelf(parent.filename, isMain, request);
+    if (filename) {
+      emitExperimentalWarning('Package name self resolution');
+      const cacheKey = request + '\x00' +
+          (paths.length === 1 ? paths[0] : paths.join('\x00'));
+      Module._pathCache[cacheKey] = filename;
+      return filename;
     }
-    // eslint-disable-next-line no-restricted-syntax
-    const err = new Error(message);
-    err.code = 'MODULE_NOT_FOUND';
-    err.requireStack = requireStack;
-    throw err;
   }
-  return filename;
+  const requireStack = [];
+  for (let cursor = parent;
+    cursor;
+    cursor = cursor.parent) {
+    requireStack.push(cursor.filename || cursor.id);
+  }
+  let message = `Cannot find module '${request}'`;
+  if (requireStack.length > 0) {
+    message = message + '\nRequire stack:\n- ' + requireStack.join('\n- ');
+  }
+  // eslint-disable-next-line no-restricted-syntax
+  const err = new Error(message);
+  err.code = 'MODULE_NOT_FOUND';
+  err.requireStack = requireStack;
+  throw err;
 };
 
 

src/module_wrap.cc

@@ -1150,12 +1150,12 @@ Maybe<URL> PackageExportsResolve(Environment* env,
 }
 
 Maybe<URL> ResolveSelf(Environment* env,
-                       const std::string& specifier,
+                       const std::string& pkg_name,
+                       const std::string& pkg_subpath,
                        const URL& base) {
   if (!env->options()->experimental_resolve_self) {
     return Nothing<URL>();
   }
-
   const PackageConfig* pcfg;
   if (GetPackageScopeConfig(env, base, base).To(&pcfg) &&
       pcfg->exists == Exists::Yes) {
@@ -1171,33 +1171,14 @@ Maybe<URL> ResolveSelf(Environment* env,
         found_pjson = true;
       }
     }
-
-    if (!found_pjson) {
-      return Nothing<URL>();
-    }
-
-    // "If specifier starts with pcfg name"
-    std::string subpath;
-    if (specifier.rfind(pcfg->name, 0)) {
-      // We know now: specifier is either equal to name or longer.
-      if (specifier == subpath) {
-        subpath = "";
-      } else if (specifier[pcfg->name.length()] == '/') {
-        // Return everything after the slash
-        subpath = "." + specifier.substr(pcfg->name.length() + 1);
-      } else {
-        // The specifier is neither the name of the package nor a subpath of it
-        return Nothing<URL>();
-      }
-    }
-
-    if (found_pjson && !subpath.length()) {
+    if (!found_pjson || pcfg->name != pkg_name) return Nothing<URL>();
+    if (!pkg_subpath.length()) {
       return PackageMainResolve(env, pjson_url, *pcfg, base);
-    } else if (found_pjson) {
+    } else {
       if (!pcfg->exports.IsEmpty()) {
-        return PackageExportsResolve(env, pjson_url, subpath, *pcfg, base);
+        return PackageExportsResolve(env, pjson_url, pkg_subpath, *pcfg, base);
       } else {
-        return FinalizeResolution(env, URL(subpath, pjson_url), base);
+        return FinalizeResolution(env, URL(pkg_subpath, pjson_url), base);
       }
     }
   }
@@ -1245,6 +1226,13 @@ Maybe<URL> PackageResolve(Environment* env,
   } else {
     pkg_subpath = "." + specifier.substr(sep_index);
   }
+
+  Maybe<URL> self_url = ResolveSelf(env, pkg_name, pkg_subpath, base);
+  if (self_url.IsJust()) {
+    ProcessEmitExperimentalWarning(env, "Package name self resolution");
+    return self_url;
+  }
+
   URL pjson_url("./node_modules/" + pkg_name + "/package.json", &base);
   std::string pjson_path = pjson_url.ToFilePath();
   std::string last_path;
@@ -1278,12 +1266,6 @@ Maybe<URL> PackageResolve(Environment* env,
     // Cross-platform root check.
   } while (pjson_path.length() != last_path.length());
 
-  Maybe<URL> self_url = ResolveSelf(env, specifier, base);
-  if (self_url.IsJust()) {
-    ProcessEmitExperimentalWarning(env, "Package name self resolution");
-    return self_url;
-  }
-
   std::string msg = "Cannot find package '" + pkg_name +
       "' imported from " + base.ToFilePath();
   node::THROW_ERR_MODULE_NOT_FOUND(env, msg.c_str());

src/node_file.cc

@@ -829,6 +829,7 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo<Value>& args) {
 
   const size_t size = offset - start;
   if (size == 0 || (
+    size == SearchString(&chars[start], size, "\"name\"") &&
     size == SearchString(&chars[start], size, "\"main\"") &&
     size == SearchString(&chars[start], size, "\"exports\"") &&
     size == SearchString(&chars[start], size, "\"type\""))) {

test/es-module/test-esm-exports.mjs

@@ -22,8 +22,6 @@ import fromInside from '../fixtures/node_modules/pkgexports/lib/hole.js';
     // Fallbacks
     ['pkgexports/fallbackdir/asdf.js', { default: 'asdf' }],
     ['pkgexports/fallbackfile', { default: 'asdf' }],
-    // Dot main
-    ['pkgexports', { default: 'asdf' }],
     // Conditional split for require
     ['pkgexports/condition', isRequire ? { default: 'encoded path' } :
       { default: 'asdf' }],
@@ -32,6 +30,11 @@ import fromInside from '../fixtures/node_modules/pkgexports/lib/hole.js';
     // Conditional object exports sugar
     ['pkgexports-sugar2', isRequire ? { default: 'not-exported' } :
       { default: 'main' }],
+    // Resolve self
+    ['pkgexports/resolve-self', isRequire ?
+      { default: 'self-cjs' } : { default: 'self-mjs' }],
+    // Resolve self sugar
+    ['pkgexports-sugar', { default: 'main' }]
   ]);
 
   for (const [validSpecifier, expected] of validSpecifiers) {
@@ -139,7 +142,7 @@ const { requireFromInside, importFromInside } = fromInside;
     // A file not visible from outside of the package
     ['../not-exported.js', { default: 'not-exported' }],
     // Part of the public interface
-    ['@pkgexports/name/valid-cjs', { default: 'asdf' }],
+    ['pkgexports/valid-cjs', { default: 'asdf' }],
   ]);
   for (const [validSpecifier, expected] of validSpecifiers) {
     if (validSpecifier === null) continue;
@@ -173,7 +176,7 @@ function assertIncludes(actual, expected) {
     '/node_modules/pkgexports/resolve-self.js',
     'Package name self resolution',
   ],
-].forEach(([flag, file, message]) => {
+].forEach(([flag, file, message], index) => {
   const child = spawn(process.execPath, [flag, path(file)]);
 
   let stderr = '';
@@ -182,6 +185,7 @@ function assertIncludes(actual, expected) {
     stderr += data;
   });
   child.on('close', (code, signal) => {
+    console.log(stderr.toString());
     strictEqual(code, 0);
     strictEqual(signal, null);
     ok(stderr.toString().includes(