Skip to content

Commit

Permalink
module: self resolve bug fix and esm ordering
Browse files Browse the repository at this point in the history
PR-URL: #31009
Reviewed-By: Jan Krems <[email protected]>
  • Loading branch information
guybedford authored and MylesBorins committed Jan 12, 2020
1 parent 5b4db8e commit 801ea12
Show file tree
Hide file tree
Showing 12 changed files with 106 additions and 101 deletions.
36 changes: 18 additions & 18 deletions doc/api/esm.md
Original file line number Diff line number Diff line change
Expand Up @@ -1209,6 +1209,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_.
Expand All @@ -1230,30 +1233,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 @@ -420,13 +420,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 @@ -438,12 +438,15 @@ function trySelf(paths, exts, isMain, trailingSlash, request) {
return false;
}

if (exts === undefined)
exts = Object.keys(Module._extensions);

if (expansion) {
// Use exports
const fromExports = applyExports(basePath, expansion);
const exts = Object.keys(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 @@ -691,13 +694,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 @@ -941,26 +937,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 @@ -783,6 +783,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 @@ -23,8 +23,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 @@ -33,6 +31,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 @@ -140,7 +143,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.

0 comments on commit 801ea12

Please sign in to comment.