module: self resolve bug fix and esm ordering

PR-URL: #31009
Reviewed-By: Jan Krems <jan.krems@gmail.com>

Author: guybedford

Diff for: doc/api/esm.md

@@ -1207,6 +1207,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_RESOLVE**(_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_.
@@ -1228,30 +1231,27 @@ _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. Return **undefined**.
 > 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. Otherwise return an empty result.
+> 1. If _pjson_ does not include an _"exports"_ property, then
+>    1. Return **undefined**.
+> 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 **undefined**.
 
 **PACKAGE_MAIN_RESOLVE**(_packageURL_, _pjson_)
 

Diff for: doc/api/modules.md

@@ -160,9 +160,9 @@ require(X) from module at path Y
    a. LOAD_AS_FILE(Y + X)
    b. LOAD_AS_DIRECTORY(Y + X)
    c. THROW "not found"
-4. LOAD_NODE_MODULES(X, dirname(Y))
-5. LOAD_SELF_REFERENCE(X, dirname(Y))
-6. THROW "not found"
+4. LOAD_SELF_REFERENCE(X, dirname(Y))
+5. LOAD_NODE_MODULES(X, dirname(Y))
+7. THROW "not found"
 
 LOAD_AS_FILE(X)
 1. If X is a file, load X as JavaScript text.  STOP
@@ -205,9 +205,10 @@ NODE_MODULES_PATHS(START)
 
 LOAD_SELF_REFERENCE(X, START)
 1. Find the closest package scope to START.
-2. If no scope was found, throw "not found".
-3. If the name in `package.json` isn't a prefix of X, throw "not found".
-4. Otherwise, resolve the remainder of X relative to this package as if it
+2. If no scope was found, return.
+3. If the `package.json` has no "exports", return.
+4. If the name in `package.json` isn't a prefix of X, throw "not found".
+5. Otherwise, resolve the remainder of X relative to this package as if it
    was loaded via `LOAD_NODE_MODULES` with a name in `package.json`.
 ```
 

Diff for: lib/internal/modules/cjs/loader.js

@@ -428,13 +428,13 @@ 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]);
-  if (!pkg) return false;
+  const { data: pkg, path: basePath } = readPackageScope(parentPath) || {};
+  if (!pkg || 'exports' in pkg === false) return false;
   if (typeof pkg.name !== 'string') return false;
 
   let expansion;
@@ -446,12 +446,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
@@ -699,13 +702,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;
 };
 
@@ -949,26 +945,35 @@ Module._resolveFilename = function(request, parent, isMain, options) {
     paths = Module._resolveLookupPaths(request, parent);
   }
 
-  // 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 (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;
+
+  // Look up the filename first, since that's the cache key.
+  const filename = Module._findPath(request, paths, isMain, false);
+  if (filename) 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;
 };
 
 

Diff for: 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,34 +1171,12 @@ 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 (pcfg->exports.IsEmpty()) return Nothing<URL>();
+    if (!pkg_subpath.length()) {
       return PackageMainResolve(env, pjson_url, *pcfg, base);
-    } else if (found_pjson) {
-      if (!pcfg->exports.IsEmpty()) {
-        return PackageExportsResolve(env, pjson_url, subpath, *pcfg, base);
-      } else {
-        return FinalizeResolution(env, URL(subpath, pjson_url), base);
-      }
+    } else {
+      return PackageExportsResolve(env, pjson_url, pkg_subpath, *pcfg, base);
     }
   }
 
@@ -1245,6 +1223,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 +1263,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());

Diff for: 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\""))) {

Diff for: 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;