Skip to content

Commit 8a96d05

Browse files
guybedfordguybedford
committed
module: self resolve bug fix and esm ordering
PR-URL: nodejs#31009 Reviewed-By: Jan Krems <jan.krems@gmail.com>
1 parent 3cd7780 commit 8a96d05

12 files changed

Lines changed: 106 additions & 101 deletions

File tree

‎doc/api/esm.md‎

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1207,6 +1207,9 @@ _defaultEnv_ is the conditional environment name priority array,
12071207
> 1. If _packageSubpath_ contains any _"."_ or _".."_ segments or percent
12081208
> encoded strings for _"/"_ or _"\\"_, then
12091209
> 1. Throw an _Invalid Specifier_ error.
1210+
> 1. Set _selfUrl_ to the result of
1211+
> **SELF_REFERENCE_RESOLVE**(_packageName_, _packageSubpath_, _parentURL_).
1212+
> 1. If _selfUrl_ isn't empty, return _selfUrl_.
12101213
> 1. If _packageSubpath_ is _undefined_ and _packageName_ is a Node.js builtin
12111214
> module, then
12121215
> 1. Return the string _"node:"_ concatenated with _packageSpecifier_.
@@ -1228,30 +1231,27 @@ _defaultEnv_ is the conditional environment name priority array,
12281231
> 1. Return **PACKAGE_EXPORTS_RESOLVE**(_packageURL_,
12291232
> _packageSubpath_, _pjson.exports_).
12301233
> 1. Return the URL resolution of _packageSubpath_ in _packageURL_.
1231-
> 1. Set _selfUrl_ to the result of
1232-
> **SELF_REFERENCE_RESOLE**(_packageSpecifier_, _parentURL_).
1233-
> 1. If _selfUrl_ isn't empty, return _selfUrl_.
12341234
> 1. Throw a _Module Not Found_ error.
12351235
1236-
**SELF_REFERENCE_RESOLVE**(_specifier_, _parentURL_)
1236+
**SELF_REFERENCE_RESOLVE**(_packageName_, _packageSubpath_, _parentURL_)
12371237
12381238
> 1. Let _packageURL_ be the result of **READ_PACKAGE_SCOPE**(_parentURL_).
12391239
> 1. If _packageURL_ is **null**, then
1240-
> 1. Return an empty result.
1240+
> 1. Return **undefined**.
12411241
> 1. Let _pjson_ be the result of **READ_PACKAGE_JSON**(_packageURL_).
1242-
> 1. Set _name_ to _pjson.name_.
1243-
> 1. If _name_ is empty, then return an empty result.
1244-
> 1. If _name_ is equal to _specifier_, then
1245-
> 1. Return the result of **PACKAGE_MAIN_RESOLVE**(_packageURL_, _pjson_).
1246-
> 1. If _specifier_ starts with _name_ followed by "/", then
1247-
> 1. Set _subpath_ to everything after the "/".
1248-
> 1. If _pjson_ is not **null** and _pjson_ has an _"exports"_ key, then
1249-
> 1. Let _exports_ be _pjson.exports_.
1250-
> 1. If _exports_ is not **null** or **undefined**, then
1251-
> 1. Return **PACKAGE_EXPORTS_RESOLVE**(_packageURL_, _subpath_,
1252-
> _pjson.exports_).
1253-
> 1. Return the URL resolution of _subpath_ in _packageURL_.
1254-
> 1. Otherwise return an empty result.
1242+
> 1. If _pjson_ does not include an _"exports"_ property, then
1243+
> 1. Return **undefined**.
1244+
> 1. If _pjson.name_ is equal to _packageName_, then
1245+
> 1. If _packageSubpath_ is _undefined_, then
1246+
> 1. Return the result of **PACKAGE_MAIN_RESOLVE**(_packageURL_, _pjson_).
1247+
> 1. Otherwise,
1248+
> 1. If _pjson_ is not **null** and _pjson_ has an _"exports"_ key, then
1249+
> 1. Let _exports_ be _pjson.exports_.
1250+
> 1. If _exports_ is not **null** or **undefined**, then
1251+
> 1. Return **PACKAGE_EXPORTS_RESOLVE**(_packageURL_, _subpath_,
1252+
> _pjson.exports_).
1253+
> 1. Return the URL resolution of _subpath_ in _packageURL_.
1254+
> 1. Otherwise, return **undefined**.
12551255
12561256
**PACKAGE_MAIN_RESOLVE**(_packageURL_, _pjson_)
12571257

‎doc/api/modules.md‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -160,9 +160,9 @@ require(X) from module at path Y
160160
a. LOAD_AS_FILE(Y + X)
161161
b. LOAD_AS_DIRECTORY(Y + X)
162162
c. THROW "not found"
163-
4. LOAD_NODE_MODULES(X, dirname(Y))
164-
5. LOAD_SELF_REFERENCE(X, dirname(Y))
165-
6. THROW "not found"
163+
4. LOAD_SELF_REFERENCE(X, dirname(Y))
164+
5. LOAD_NODE_MODULES(X, dirname(Y))
165+
7. THROW "not found"
166166
167167
LOAD_AS_FILE(X)
168168
1. If X is a file, load X as JavaScript text. STOP
@@ -205,9 +205,10 @@ NODE_MODULES_PATHS(START)
205205
206206
LOAD_SELF_REFERENCE(X, START)
207207
1. Find the closest package scope to START.
208-
2. If no scope was found, throw "not found".
209-
3. If the name in `package.json` isn't a prefix of X, throw "not found".
210-
4. Otherwise, resolve the remainder of X relative to this package as if it
208+
2. If no scope was found, return.
209+
3. If the `package.json` has no "exports", return.
210+
4. If the name in `package.json` isn't a prefix of X, throw "not found".
211+
5. Otherwise, resolve the remainder of X relative to this package as if it
211212
was loaded via `LOAD_NODE_MODULES` with a name in `package.json`.
212213
```
213214

‎lib/internal/modules/cjs/loader.js‎

Lines changed: 39 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -431,13 +431,13 @@ function resolveBasePath(basePath, exts, isMain, trailingSlash, request) {
431431
return filename;
432432
}
433433

434-
function trySelf(paths, exts, isMain, trailingSlash, request) {
434+
function trySelf(parentPath, isMain, request) {
435435
if (!experimentalSelf) {
436436
return false;
437437
}
438438

439-
const { data: pkg, path: basePath } = readPackageScope(paths[0]);
440-
if (!pkg) return false;
439+
const { data: pkg, path: basePath } = readPackageScope(parentPath) || {};
440+
if (!pkg || 'exports' in pkg === false) return false;
441441
if (typeof pkg.name !== 'string') return false;
442442

443443
let expansion;
@@ -449,12 +449,15 @@ function trySelf(paths, exts, isMain, trailingSlash, request) {
449449
return false;
450450
}
451451

452-
if (exts === undefined)
453-
exts = ObjectKeys(Module._extensions);
454-
455-
if (expansion) {
456-
// Use exports
457-
const fromExports = applyExports(basePath, expansion);
452+
const exts = ObjectKeys(Module._extensions);
453+
const fromExports = applyExports(basePath, expansion);
454+
// Use exports
455+
if (fromExports) {
456+
let trailingSlash = request.length > 0 &&
457+
request.charCodeAt(request.length - 1) === CHAR_FORWARD_SLASH;
458+
if (!trailingSlash) {
459+
trailingSlash = /(?:^|\/)\.?\.$/.test(request);
460+
}
458461
return resolveBasePath(fromExports, exts, isMain, trailingSlash, request);
459462
} else {
460463
// Use main field
@@ -702,13 +705,6 @@ Module._findPath = function(request, paths, isMain) {
702705
}
703706
}
704707

705-
const selfFilename = trySelf(paths, exts, isMain, trailingSlash, request);
706-
if (selfFilename) {
707-
emitExperimentalWarning('Package name self resolution');
708-
Module._pathCache[cacheKey] = selfFilename;
709-
return selfFilename;
710-
}
711-
712708
return false;
713709
};
714710

@@ -1006,26 +1002,35 @@ Module._resolveFilename = function(request, parent, isMain, options) {
10061002
paths = Module._resolveLookupPaths(request, parent);
10071003
}
10081004

1009-
// Look up the filename first, since that's the cache key.
1010-
const filename = Module._findPath(request, paths, isMain);
1011-
if (!filename) {
1012-
const requireStack = [];
1013-
for (let cursor = parent;
1014-
cursor;
1015-
cursor = cursor.parent) {
1016-
requireStack.push(cursor.filename || cursor.id);
1017-
}
1018-
let message = `Cannot find module '${request}'`;
1019-
if (requireStack.length > 0) {
1020-
message = message + '\nRequire stack:\n- ' + requireStack.join('\n- ');
1005+
if (parent && parent.filename) {
1006+
const filename = trySelf(parent.filename, isMain, request);
1007+
if (filename) {
1008+
emitExperimentalWarning('Package name self resolution');
1009+
const cacheKey = request + '\x00' +
1010+
(paths.length === 1 ? paths[0] : paths.join('\x00'));
1011+
Module._pathCache[cacheKey] = filename;
1012+
return filename;
10211013
}
1022-
// eslint-disable-next-line no-restricted-syntax
1023-
const err = new Error(message);
1024-
err.code = 'MODULE_NOT_FOUND';
1025-
err.requireStack = requireStack;
1026-
throw err;
10271014
}
1028-
return filename;
1015+
1016+
// Look up the filename first, since that's the cache key.
1017+
const filename = Module._findPath(request, paths, isMain, false);
1018+
if (filename) return filename;
1019+
const requireStack = [];
1020+
for (let cursor = parent;
1021+
cursor;
1022+
cursor = cursor.parent) {
1023+
requireStack.push(cursor.filename || cursor.id);
1024+
}
1025+
let message = `Cannot find module '${request}'`;
1026+
if (requireStack.length > 0) {
1027+
message = message + '\nRequire stack:\n- ' + requireStack.join('\n- ');
1028+
}
1029+
// eslint-disable-next-line no-restricted-syntax
1030+
const err = new Error(message);
1031+
err.code = 'MODULE_NOT_FOUND';
1032+
err.requireStack = requireStack;
1033+
throw err;
10291034
};
10301035

10311036

‎src/module_wrap.cc‎

Lines changed: 14 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1150,12 +1150,12 @@ Maybe<URL> PackageExportsResolve(Environment* env,
11501150
}
11511151

11521152
Maybe<URL> ResolveSelf(Environment* env,
1153-
const std::string& specifier,
1153+
const std::string& pkg_name,
1154+
const std::string& pkg_subpath,
11541155
const URL& base) {
11551156
if (!env->options()->experimental_resolve_self) {
11561157
return Nothing<URL>();
11571158
}
1158-
11591159
const PackageConfig* pcfg;
11601160
if (GetPackageScopeConfig(env, base, base).To(&pcfg) &&
11611161
pcfg->exists == Exists::Yes) {
@@ -1171,34 +1171,12 @@ Maybe<URL> ResolveSelf(Environment* env,
11711171
found_pjson = true;
11721172
}
11731173
}
1174-
1175-
if (!found_pjson) {
1176-
return Nothing<URL>();
1177-
}
1178-
1179-
// "If specifier starts with pcfg name"
1180-
std::string subpath;
1181-
if (specifier.rfind(pcfg->name, 0)) {
1182-
// We know now: specifier is either equal to name or longer.
1183-
if (specifier == subpath) {
1184-
subpath = "";
1185-
} else if (specifier[pcfg->name.length()] == '/') {
1186-
// Return everything after the slash
1187-
subpath = "." + specifier.substr(pcfg->name.length() + 1);
1188-
} else {
1189-
// The specifier is neither the name of the package nor a subpath of it
1190-
return Nothing<URL>();
1191-
}
1192-
}
1193-
1194-
if (found_pjson && !subpath.length()) {
1174+
if (!found_pjson || pcfg->name != pkg_name) return Nothing<URL>();
1175+
if (pcfg->exports.IsEmpty()) return Nothing<URL>();
1176+
if (!pkg_subpath.length()) {
11951177
return PackageMainResolve(env, pjson_url, *pcfg, base);
1196-
} else if (found_pjson) {
1197-
if (!pcfg->exports.IsEmpty()) {
1198-
return PackageExportsResolve(env, pjson_url, subpath, *pcfg, base);
1199-
} else {
1200-
return FinalizeResolution(env, URL(subpath, pjson_url), base);
1201-
}
1178+
} else {
1179+
return PackageExportsResolve(env, pjson_url, pkg_subpath, *pcfg, base);
12021180
}
12031181
}
12041182

@@ -1245,6 +1223,13 @@ Maybe<URL> PackageResolve(Environment* env,
12451223
} else {
12461224
pkg_subpath = "." + specifier.substr(sep_index);
12471225
}
1226+
1227+
Maybe<URL> self_url = ResolveSelf(env, pkg_name, pkg_subpath, base);
1228+
if (self_url.IsJust()) {
1229+
ProcessEmitExperimentalWarning(env, "Package name self resolution");
1230+
return self_url;
1231+
}
1232+
12481233
URL pjson_url("./node_modules/" + pkg_name + "/package.json", &base);
12491234
std::string pjson_path = pjson_url.ToFilePath();
12501235
std::string last_path;
@@ -1278,12 +1263,6 @@ Maybe<URL> PackageResolve(Environment* env,
12781263
// Cross-platform root check.
12791264
} while (pjson_path.length() != last_path.length());
12801265

1281-
Maybe<URL> self_url = ResolveSelf(env, specifier, base);
1282-
if (self_url.IsJust()) {
1283-
ProcessEmitExperimentalWarning(env, "Package name self resolution");
1284-
return self_url;
1285-
}
1286-
12871266
std::string msg = "Cannot find package '" + pkg_name +
12881267
"' imported from " + base.ToFilePath();
12891268
node::THROW_ERR_MODULE_NOT_FOUND(env, msg.c_str());

‎src/node_file.cc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -829,6 +829,7 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo<Value>& args) {
829829

830830
const size_t size = offset - start;
831831
if (size == 0 || (
832+
size == SearchString(&chars[start], size, "\"name\"") &&
832833
size == SearchString(&chars[start], size, "\"main\"") &&
833834
size == SearchString(&chars[start], size, "\"exports\"") &&
834835
size == SearchString(&chars[start], size, "\"type\""))) {

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,6 @@ import fromInside from '../fixtures/node_modules/pkgexports/lib/hole.js';
2222
// Fallbacks
2323
['pkgexports/fallbackdir/asdf.js', { default: 'asdf' }],
2424
['pkgexports/fallbackfile', { default: 'asdf' }],
25-
// Dot main
26-
['pkgexports', { default: 'asdf' }],
2725
// Conditional split for require
2826
['pkgexports/condition', isRequire ? { default: 'encoded path' } :
2927
{ default: 'asdf' }],
@@ -32,6 +30,11 @@ import fromInside from '../fixtures/node_modules/pkgexports/lib/hole.js';
3230
// Conditional object exports sugar
3331
['pkgexports-sugar2', isRequire ? { default: 'not-exported' } :
3432
{ default: 'main' }],
33+
// Resolve self
34+
['pkgexports/resolve-self', isRequire ?
35+
{ default: 'self-cjs' } : { default: 'self-mjs' }],
36+
// Resolve self sugar
37+
['pkgexports-sugar', { default: 'main' }]
3538
]);
3639

3740
for (const [validSpecifier, expected] of validSpecifiers) {
@@ -139,7 +142,7 @@ const { requireFromInside, importFromInside } = fromInside;
139142
// A file not visible from outside of the package
140143
['../not-exported.js', { default: 'not-exported' }],
141144
// Part of the public interface
142-
['@pkgexports/name/valid-cjs', { default: 'asdf' }],
145+
['pkgexports/valid-cjs', { default: 'asdf' }],
143146
]);
144147
for (const [validSpecifier, expected] of validSpecifiers) {
145148
if (validSpecifier === null) continue;

‎test/fixtures/node_modules/pkgexports-main/package.json‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎test/fixtures/node_modules/pkgexports-sugar/main.js‎

Lines changed: 9 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎test/fixtures/node_modules/pkgexports-sugar/package.json‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎test/fixtures/node_modules/pkgexports/package.json‎

Lines changed: 6 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)