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.

Sign up for GitHub

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: fix extension searching for exports #32351

Closed
wants to merge 2 commits into from
Closed

module: fix extension searching for exports #32351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f399cfd
module: fix no extension searching for exports
guybedford Mar 18, 2020
ce967de
module: CJS extension searching for folder exports
guybedford Mar 25, 2020
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
2 changes: 1 addition & 1 deletion doc/api/esm.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1635,7 +1635,7 @@ The resolver can throw the following errors:
> 1. If _exports_ contains any index property keys, as defined in ECMA-262
> [6.1.7 Array Index][], throw an _Invalid Package Configuration_ error.
> 1. For each property _p_ of _target_, in object insertion order as,
> 1. If _env_ contains an entry for _p_, then
> 1. If _p_ equals _"default"_ or _env_ contains an entry for _p_, then
> 1. Let _targetValue_ be the value of the _p_ property in _target_.
> 1. Return the result of **PACKAGE_EXPORTS_TARGET_RESOLVE**(
> _packageURL_, _targetValue_, _subpath_, _env_), continuing the
Expand Down
70 changes: 28 additions & 42 deletions doc/api/modules.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ require(X) from module at path Y
6. THROW "not found"
LOAD_AS_FILE(X)
1. If X is a file, load X as JavaScript text. STOP
1. If X is a file, load X as its file extension format. STOP
guybedford marked this conversation as resolved.
Show resolved Hide resolved
2. If X.js is a file, load X.js as JavaScript text. STOP
3. If X.json is a file, parse X.json to a JavaScript Object. STOP
4. If X.node is a file, load X.node as binary addon. STOP
Expand All @@ -189,8 +189,9 @@ LOAD_AS_DIRECTORY(X)
LOAD_NODE_MODULES(X, START)
1. let DIRS = NODE_MODULES_PATHS(START)
2. for each DIR in DIRS:
a. LOAD_AS_FILE(DIR/X)
b. LOAD_AS_DIRECTORY(DIR/X)
a. LOAD_PACKAGE_EXPORTS(DIR, X)
b. LOAD_AS_FILE(DIR/X)
c. LOAD_AS_DIRECTORY(DIR/X)
NODE_MODULES_PATHS(START)
1. let PARTS = path split(START)
Expand All @@ -208,50 +209,35 @@ LOAD_SELF_REFERENCE(X, START)
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`.
```

Node.js allows packages loaded via
`LOAD_NODE_MODULES` to explicitly declare which file paths to expose and how
they should be interpreted. This expands on the control packages already had
using the `main` field.

With this feature enabled, the `LOAD_NODE_MODULES` changes are:
5. Otherwise, load the remainder of X relative to this package as if it
was loaded via `LOAD_NODE_MODULES` with a name in `package.json`.
```txt
LOAD_NODE_MODULES(X, START)
1. let DIRS = NODE_MODULES_PATHS(START)
2. for each DIR in DIRS:
a. let FILE_PATH = RESOLVE_BARE_SPECIFIER(DIR, X)
b. LOAD_AS_FILE(FILE_PATH)
c. LOAD_AS_DIRECTORY(FILE_PATH)
RESOLVE_BARE_SPECIFIER(DIR, X)
LOAD_PACKAGE_EXPORTS(DIR, X)
1. Try to interpret X as a combination of name and subpath where the name
may have a @scope/ prefix and the subpath begins with a slash (`/`).
2. If X matches this pattern and DIR/name/package.json is a file:
a. Parse DIR/name/package.json, and look for "exports" field.
b. If "exports" is null or undefined, GOTO 3.
c. If "exports" is an object with some keys starting with "." and some keys
not starting with ".", throw "invalid config".
d. If "exports" is a string, or object with no keys starting with ".", treat
it as having that value as its "." object property.
e. If subpath is "." and "exports" does not have a "." entry, GOTO 3.
f. Find the longest key in "exports" that the subpath starts with.
g. If no such key can be found, throw "not found".
h. let RESOLVED_URL =
PACKAGE_EXPORTS_TARGET_RESOLVE(pathToFileURL(DIR/name), exports[key],
subpath.slice(key.length), ["node", "require"]), as defined in the ESM
resolver.
i. return fileURLToPath(RESOLVED_URL)
3. return DIR/X
2. If X does not match this pattern or DIR/name/package.json is not a file,
return.
3. Parse DIR/name/package.json, and look for "exports" field.
4. If "exports" is null or undefined, return.
5. If "exports" is an object with some keys starting with "." and some keys
not starting with ".", throw "invalid config".
6. If "exports" is a string, or object with no keys starting with ".", treat
it as having that value as its "." object property.
7. If subpath is "." and "exports" does not have a "." entry, return.
8. Find the longest key in "exports" that the subpath starts with.
9. If no such key can be found, throw "not found".
10. let RESOLVED =
fileURLToPath(PACKAGE_EXPORTS_TARGET_RESOLVE(pathToFileURL(DIR/name),
exports[key], subpath.slice(key.length), ["node", "require"])), as defined
in the ESM resolver.
11. If key ends with "/":
a. LOAD_AS_FILE(RESOLVED)
b. LOAD_AS_DIRECTORY(RESOLVED)
12. Otherwise
a. If RESOLVED is a file, load it as its file extension format. STOP
13. Throw "not found"
```

`"exports"` is only honored when loading a package "name" as defined above. Any
`"exports"` values within nested directories and packages must be declared by
the `package.json` responsible for the "name".

## Caching

<!--type=misc-->
Expand Down
130 changes: 53 additions & 77 deletions lib/internal/modules/cjs/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ const {
ObjectPrototypeHasOwnProperty,
ObjectSetPrototypeOf,
ReflectSet,
RegExpPrototypeTest,
SafeMap,
String,
StringPrototypeIndexOf,
Expand Down Expand Up @@ -125,7 +126,7 @@ function enrichCJSError(err) {
after a comment block and/or after a variable definition.
*/
if (err.message.startsWith('Unexpected token \'export\'') ||
(/^\s*import(?=[ {'"*])\s*(?![ (])/).test(lineWithErr)) {
(RegExpPrototypeTest(/^\s*import(?=[ {'"*])\s*(?![ (])/, lineWithErr))) {
// Emit the warning synchronously because we are in the middle of handling
// a SyntaxError that will throw and likely terminate the process before an
// asynchronous warning would be emitted.
Expand Down Expand Up @@ -352,10 +353,11 @@ const realpathCache = new Map();
// absolute realpath.
function tryFile(requestPath, isMain) {
const rc = stat(requestPath);
if (rc !== 0) return;
if (preserveSymlinks && !isMain) {
return rc === 0 && path.resolve(requestPath);
return path.resolve(requestPath);
}
return rc === 0 && toRealPath(requestPath);
return toRealPath(requestPath);
}

function toRealPath(requestPath) {
Expand Down Expand Up @@ -392,52 +394,7 @@ function findLongestRegisteredExtension(filename) {
return '.js';
}

function resolveBasePath(basePath, exts, isMain, trailingSlash, request) {
let filename;

const rc = stat(basePath);
if (!trailingSlash) {
if (rc === 0) { // File.
if (!isMain) {
if (preserveSymlinks) {
filename = path.resolve(basePath);
} else {
filename = toRealPath(basePath);
}
} else if (preserveSymlinksMain) {
// For the main module, we use the preserveSymlinksMain flag instead
// mainly for backward compatibility, as the preserveSymlinks flag
// historically has not applied to the main module. Most likely this
// was intended to keep .bin/ binaries working, as following those
// symlinks is usually required for the imports in the corresponding
// files to resolve; that said, in some use cases following symlinks
// causes bigger problems which is why the preserveSymlinksMain option
// is needed.
filename = path.resolve(basePath);
} else {
filename = toRealPath(basePath);
}
}

if (!filename) {
// Try it with each of the extensions
if (exts === undefined)
exts = ObjectKeys(Module._extensions);
filename = tryExtensions(basePath, exts, isMain);
}
}

if (!filename && rc === 1) { // Directory.
// try it with each of the extensions at "index"
if (exts === undefined)
exts = ObjectKeys(Module._extensions);
filename = tryPackage(basePath, exts, isMain, request);
}

return filename;
}

function trySelf(parentPath, isMain, request) {
function trySelf(parentPath, request) {
const { data: pkg, path: basePath } = readPackageScope(parentPath) || {};
if (!pkg || pkg.exports === undefined) return false;
if (typeof pkg.name !== 'string') return false;
Expand All @@ -451,20 +408,11 @@ function trySelf(parentPath, isMain, request) {
return false;
}

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
return tryPackage(basePath, exts, isMain, request);
return tryFile(fromExports, false);
}
assert(fromExports !== false);
}

function isConditionalDotExportSugar(exports, basePath) {
Expand Down Expand Up @@ -496,7 +444,7 @@ function applyExports(basePath, expansion) {

let pkgExports = readPackageExports(basePath);
if (pkgExports === undefined || pkgExports === null)
return path.resolve(basePath, mappingKey);
return false;

if (isConditionalDotExportSugar(pkgExports, basePath))
pkgExports = { '.': pkgExports };
Expand All @@ -520,8 +468,24 @@ function applyExports(basePath, expansion) {
if (dirMatch !== '') {
const mapping = pkgExports[dirMatch];
const subpath = StringPrototypeSlice(mappingKey, dirMatch.length);
return resolveExportsTarget(pathToFileURL(basePath + '/'), mapping,
subpath, mappingKey);
const resolved = resolveExportsTarget(pathToFileURL(basePath + '/'),
mapping, subpath, mappingKey);
// Extension searching for folder exports only
const rc = stat(resolved);
if (rc === 0) return resolved;
if (!(RegExpPrototypeTest(trailingSlashRegex, resolved))) {
const exts = ObjectKeys(Module._extensions);
const filename = tryExtensions(resolved, exts, false);
if (filename) return filename;
}
if (rc === 1) {
const exts = ObjectKeys(Module._extensions);
const filename = tryPackage(resolved, exts, false,
basePath + expansion);
if (filename) return filename;
}
// Undefined means not found
return;
}
}

Expand All @@ -532,20 +496,20 @@ function applyExports(basePath, expansion) {
// 1. name/.*
// 2. @scope/name/.*
const EXPORTS_PATTERN = /^((?:@[^/\\%]+\/)?[^./\\%][^/\\%]*)(\/.*)?$/;
function resolveExports(nmPath, request, absoluteRequest) {
function resolveExports(nmPath, request) {
// The implementation's behavior is meant to mirror resolution in ESM.
if (!absoluteRequest) {
const [, name, expansion = ''] =
StringPrototypeMatch(request, EXPORTS_PATTERN) || [];
if (!name) {
return path.resolve(nmPath, request);
}

const basePath = path.resolve(nmPath, name);
return applyExports(basePath, expansion);
const [, name, expansion = ''] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using array destructuring means it will break if Symbol.iterator is deleted off of Array.prototype. this one has a lot of places in the codebase to fix, tho, so it’s probably fine to skip for now.

StringPrototypeMatch(request, EXPORTS_PATTERN) || [];
if (!name) {
return false;
}

return path.resolve(nmPath, request);
const basePath = path.resolve(nmPath, name);
const fromExports = applyExports(basePath, expansion);
if (fromExports) {
return tryFile(fromExports, false);
}
return fromExports;
}

function isArrayIndex(p) {
Expand Down Expand Up @@ -636,6 +600,7 @@ function resolveExportsTarget(baseUrl, target, subpath, mappingKey) {
StringPrototypeSlice(baseUrl.pathname, 0, -1), mappingKey, subpath, target);
}

const trailingSlashRegex = /(?:^|\/)\.?\.$/;
Module._findPath = function(request, paths, isMain) {
const absoluteRequest = path.isAbsolute(request);
if (absoluteRequest) {
Expand All @@ -654,15 +619,26 @@ Module._findPath = function(request, paths, isMain) {
let trailingSlash = request.length > 0 &&
request.charCodeAt(request.length - 1) === CHAR_FORWARD_SLASH;
if (!trailingSlash) {
trailingSlash = /(?:^|\/)\.?\.$/.test(request);
trailingSlash = RegExpPrototypeTest(trailingSlashRegex, request);
}

// For each path
for (let i = 0; i < paths.length; i++) {
// Don't search further if path doesn't exist
const curPath = paths[i];
if (curPath && stat(curPath) < 1) continue;
const basePath = resolveExports(curPath, request, absoluteRequest);

if (!absoluteRequest) {
const exportsResolved = resolveExports(curPath, request);
// Undefined means not found, false means no exports
if (exportsResolved === undefined)
break;
if (exportsResolved) {
return exportsResolved;
}
}

const basePath = path.resolve(curPath, request);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will delete require('path').resolve break this code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, although there are a lot of calls to path.resolve within this module, so changing that is best done as a separate refactoring.

let filename;

const rc = stat(basePath);
Expand Down Expand Up @@ -1005,7 +981,7 @@ Module._resolveFilename = function(request, parent, isMain, options) {
}

if (parent && parent.filename) {
const filename = trySelf(parent.filename, isMain, request);
const filename = trySelf(parent.filename, request);
if (filename) {
const cacheKey = request + '\x00' +
(paths.length === 1 ? paths[0] : paths.join('\x00'));
Expand Down
38 changes: 30 additions & 8 deletions test/es-module/test-esm-exports.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,14 @@ import fromInside from '../fixtures/node_modules/pkgexports/lib/hole.js';
['pkgexports-sugar', { default: 'main' }],
]);

if (isRequire) {
validSpecifiers.set('pkgexports/subpath/file', { default: 'file' });
validSpecifiers.set('pkgexports/subpath/dir1', { default: 'main' });
validSpecifiers.set('pkgexports/subpath/dir1/', { default: 'main' });
validSpecifiers.set('pkgexports/subpath/dir2', { default: 'index' });
validSpecifiers.set('pkgexports/subpath/dir2/', { default: 'index' });
}

for (const [validSpecifier, expected] of validSpecifiers) {
if (validSpecifier === null) continue;

Expand Down Expand Up @@ -118,14 +126,28 @@ import fromInside from '../fixtures/node_modules/pkgexports/lib/hole.js';
}));
}

// Covering out bases - not a file is still not a file after dir mapping.
loadFixture('pkgexports/sub/not-a-file.js').catch(mustCall((err) => {
strictEqual(err.code, (isRequire ? '' : 'ERR_') + 'MODULE_NOT_FOUND');
// ESM returns a full file path
assertStartsWith(err.message, isRequire ?
'Cannot find module \'pkgexports/sub/not-a-file.js\'' :
'Cannot find module');
}));
const notFoundExports = new Map([
// Non-existing file
['pkgexports/sub/not-a-file.js', 'pkgexports/sub/not-a-file.js'],
// No extension lookups
['pkgexports/no-ext', 'pkgexports/no-ext'],
]);

if (!isRequire) {
notFoundExports.set('pkgexports/subpath/file', 'pkgexports/subpath/file');
notFoundExports.set('pkgexports/subpath/dir1', 'pkgexports/subpath/dir1');
notFoundExports.set('pkgexports/subpath/dir2', 'pkgexports/subpath/dir2');
}

for (const [specifier, request] of notFoundExports) {
loadFixture(specifier).catch(mustCall((err) => {
strictEqual(err.code, (isRequire ? '' : 'ERR_') + 'MODULE_NOT_FOUND');
// ESM returns a full file path
assertStartsWith(err.message, isRequire ?
`Cannot find module '${request}'` :
'Cannot find module');
}));
}

// The use of %2F escapes in paths fails loading
loadFixture('pkgexports/sub/..%2F..%2Fbar.js').catch(mustCall((err) => {
Expand Down
4 changes: 3 additions & 1 deletion test/fixtures/node_modules/pkgexports/package.json
View file
Open in desktop

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/subpath/dir1/dir1.js
View file
Open in desktop

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

Loading