Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

module: self resolve bug fixes and esm ordering #31009

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 18 additions & 18 deletions doc/api/esm.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
guybedford marked this conversation as resolved.
Show resolved Hide resolved
> **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_.
Expand All @@ -1227,30 +1230,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_)

Expand Down
13 changes: 7 additions & 6 deletions doc/api/modules.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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`.
```

Expand Down
73 changes: 39 additions & 34 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -431,13 +431,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;
Expand All @@ -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
Expand Down Expand Up @@ -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;
};

Expand Down Expand Up @@ -1006,26 +1002,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;
};


Expand Down
49 changes: 14 additions & 35 deletions src/module_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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());
Expand Down
1 change: 1 addition & 0 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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\""))) {
Expand Down
9 changes: 6 additions & 3 deletions test/es-module/test-esm-exports.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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' }],
Expand All @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion test/fixtures/node_modules/pkgexports-main/package.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions test/fixtures/node_modules/pkgexports-sugar/main.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions test/fixtures/node_modules/pkgexports-sugar/package.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 6 additions & 3 deletions test/fixtures/node_modules/pkgexports/package.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion test/fixtures/node_modules/pkgexports/resolve-self.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions test/fixtures/node_modules/pkgexports/resolve-self.mjs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.