From 98dec2c28ed163f7a916204ab090dba0ee80327a Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Thu, 19 Mar 2020 01:49:49 +0200 Subject: [PATCH 1/2] module: path-only CJS exports extension searching PR-URL: https://github.com/nodejs/node/pull/32351 Reviewed-By: Geoffrey Booth Reviewed-By: Bradley Farias Reviewed-By: Jan Krems Reviewed-By: Myles Borins --- doc/api/esm.md | 2 +- doc/api/modules.md | 70 ++++++--------- lib/internal/modules/cjs/loader.js | 88 ++++++++++++------- test/es-module/test-esm-exports.mjs | 38 ++++++-- .../node_modules/pkgexports/package.json | 4 +- .../pkgexports/subpath/dir1/dir1.js | 1 + .../pkgexports/subpath/dir1/package.json | 3 + .../pkgexports/subpath/dir2/index.js | 1 + .../node_modules/pkgexports/subpath/file.js | 2 + 9 files changed, 125 insertions(+), 84 deletions(-) create mode 100644 test/fixtures/node_modules/pkgexports/subpath/dir1/dir1.js create mode 100644 test/fixtures/node_modules/pkgexports/subpath/dir1/package.json create mode 100644 test/fixtures/node_modules/pkgexports/subpath/dir2/index.js create mode 100644 test/fixtures/node_modules/pkgexports/subpath/file.js diff --git a/doc/api/esm.md b/doc/api/esm.md index 3b2d859b190ea6..da2f5184ed9d4d 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -1645,7 +1645,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 diff --git a/doc/api/modules.md b/doc/api/modules.md index 639516467297c3..10a532e335ccab 100644 --- a/doc/api/modules.md +++ b/doc/api/modules.md @@ -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 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 @@ -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) @@ -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 diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 734dffa343a522..e0faea3dadbcd7 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -34,6 +34,7 @@ const { ObjectKeys, ObjectPrototypeHasOwnProperty, ReflectSet, + RegExpPrototypeTest, SafeMap, String, StringPrototypeIndexOf, @@ -122,7 +123,10 @@ 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. process.emitWarning( 'To load an ES module, set "type": "module" in the package.json or use ' + 'the .mjs extension.', @@ -349,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) { @@ -434,7 +439,7 @@ function resolveBasePath(basePath, exts, isMain, trailingSlash, request) { return filename; } -function trySelf(parentPath, isMain, request) { +function trySelf(parentPath, request) { if (!experimentalModules) return false; const { data: pkg, path: basePath } = readPackageScope(parentPath) || {}; if (!pkg || pkg.exports === undefined) return false; @@ -449,20 +454,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) { @@ -494,7 +490,7 @@ function applyExports(basePath, expansion) { let pkgExports = readPackageExports(basePath); if (pkgExports === undefined || pkgExports === null || !experimentalModules) - return path.resolve(basePath, mappingKey); + return false; if (isConditionalDotExportSugar(pkgExports, basePath)) pkgExports = { '.': pkgExports }; @@ -518,8 +514,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; } } @@ -530,20 +542,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 = ''] = + 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) { @@ -635,6 +647,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) { @@ -653,7 +666,7 @@ 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 @@ -661,7 +674,18 @@ Module._findPath = function(request, paths, isMain) { // 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); let filename; const rc = stat(basePath); @@ -953,7 +977,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) { emitExperimentalWarning('Package name self resolution'); const cacheKey = request + '\x00' + diff --git a/test/es-module/test-esm-exports.mjs b/test/es-module/test-esm-exports.mjs index cc576e083a236d..8aaa66aae47d19 100644 --- a/test/es-module/test-esm-exports.mjs +++ b/test/es-module/test-esm-exports.mjs @@ -36,6 +36,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; @@ -119,14 +127,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) => { diff --git a/test/fixtures/node_modules/pkgexports/package.json b/test/fixtures/node_modules/pkgexports/package.json index 7f417ad5457bfc..200c028f88c9d3 100644 --- a/test/fixtures/node_modules/pkgexports/package.json +++ b/test/fixtures/node_modules/pkgexports/package.json @@ -32,6 +32,7 @@ "nomatch": "./nothing.js" } }], + "./no-ext": "./asdf", "./resolve-self": { "require": "./resolve-self.js", "import": "./resolve-self.mjs" @@ -39,6 +40,7 @@ "./resolve-self-invalid": { "require": "./resolve-self-invalid.js", "import": "./resolve-self-invalid.mjs" - } + }, + "./subpath/": "./subpath/" } } diff --git a/test/fixtures/node_modules/pkgexports/subpath/dir1/dir1.js b/test/fixtures/node_modules/pkgexports/subpath/dir1/dir1.js new file mode 100644 index 00000000000000..dfdd47b877319c --- /dev/null +++ b/test/fixtures/node_modules/pkgexports/subpath/dir1/dir1.js @@ -0,0 +1 @@ +module.exports = 'main'; diff --git a/test/fixtures/node_modules/pkgexports/subpath/dir1/package.json b/test/fixtures/node_modules/pkgexports/subpath/dir1/package.json new file mode 100644 index 00000000000000..ca1584ec6fe659 --- /dev/null +++ b/test/fixtures/node_modules/pkgexports/subpath/dir1/package.json @@ -0,0 +1,3 @@ +{ + "main": "dir1" +} diff --git a/test/fixtures/node_modules/pkgexports/subpath/dir2/index.js b/test/fixtures/node_modules/pkgexports/subpath/dir2/index.js new file mode 100644 index 00000000000000..d17bd0f1e9ba0f --- /dev/null +++ b/test/fixtures/node_modules/pkgexports/subpath/dir2/index.js @@ -0,0 +1 @@ +module.exports = 'index'; diff --git a/test/fixtures/node_modules/pkgexports/subpath/file.js b/test/fixtures/node_modules/pkgexports/subpath/file.js new file mode 100644 index 00000000000000..1485f701f49e67 --- /dev/null +++ b/test/fixtures/node_modules/pkgexports/subpath/file.js @@ -0,0 +1,2 @@ +module.exports = 'file'; + From 48effee911c8dd22d2e94eee7a866eb785bee3fc Mon Sep 17 00:00:00 2001 From: Myles Borins Date: Thu, 16 Apr 2020 11:44:18 -0400 Subject: [PATCH 2/2] fixup: remove resolveBasePath --- lib/internal/modules/cjs/loader.js | 45 ------------------------------ 1 file changed, 45 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index e0faea3dadbcd7..f6f550d7335a60 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -394,51 +394,6 @@ 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, request) { if (!experimentalModules) return false; const { data: pkg, path: basePath } = readPackageScope(parentPath) || {};