From bd047e827719805a709f62bddcf627634ef64440 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Tue, 17 Dec 2019 01:58:19 -0500 Subject: [PATCH] module: self resolve bug fix and esm ordering PR-URL: https://github.com/nodejs/node/pull/31009 Reviewed-By: Jan Krems --- doc/api/esm.md | 36 ++++----- doc/api/modules.md | 13 ++-- lib/internal/modules/cjs/loader.js | 73 ++++++++++--------- src/module_wrap.cc | 49 ++++--------- src/node_file.cc | 1 + test/es-module/test-esm-exports.mjs | 9 ++- .../node_modules/pkgexports-main/package.json | 2 +- .../node_modules/pkgexports-sugar/main.js | 9 +++ .../pkgexports-sugar/package.json | 1 + .../node_modules/pkgexports/package.json | 9 ++- .../node_modules/pkgexports/resolve-self.js | 3 +- .../node_modules/pkgexports/resolve-self.mjs | 2 + 12 files changed, 106 insertions(+), 101 deletions(-) create mode 100644 test/fixtures/node_modules/pkgexports/resolve-self.mjs diff --git a/doc/api/esm.md b/doc/api/esm.md index 4898edfc3a95a0..9fb9f60b2892e8 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -1207,6 +1207,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_. @@ -1228,30 +1231,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_) diff --git a/doc/api/modules.md b/doc/api/modules.md index 9dc5cd22248dc0..bac8b21d775fae 100644 --- a/doc/api/modules.md +++ b/doc/api/modules.md @@ -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 @@ -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`. ``` diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 95e2c18906eb31..445297449c7b93 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -428,13 +428,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; @@ -446,12 +446,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 @@ -699,13 +702,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; }; @@ -949,26 +945,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; }; diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 568e1ad2fdbba0..454476a1e97815 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -1150,12 +1150,12 @@ Maybe PackageExportsResolve(Environment* env, } Maybe 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(); } - const PackageConfig* pcfg; if (GetPackageScopeConfig(env, base, base).To(&pcfg) && pcfg->exists == Exists::Yes) { @@ -1171,34 +1171,12 @@ Maybe ResolveSelf(Environment* env, found_pjson = true; } } - - if (!found_pjson) { - return Nothing(); - } - - // "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(); - } - } - - if (found_pjson && !subpath.length()) { + if (!found_pjson || pcfg->name != pkg_name) return Nothing(); + if (pcfg->exports.IsEmpty()) return Nothing(); + 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); } } @@ -1245,6 +1223,13 @@ Maybe PackageResolve(Environment* env, } else { pkg_subpath = "." + specifier.substr(sep_index); } + + Maybe 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; @@ -1278,12 +1263,6 @@ Maybe PackageResolve(Environment* env, // Cross-platform root check. } while (pjson_path.length() != last_path.length()); - Maybe 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()); diff --git a/src/node_file.cc b/src/node_file.cc index 577b9c7c4173af..769d76670180fa 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -829,6 +829,7 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo& 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\""))) { diff --git a/test/es-module/test-esm-exports.mjs b/test/es-module/test-esm-exports.mjs index fd85e155b0af21..1595d1f6a1e57b 100644 --- a/test/es-module/test-esm-exports.mjs +++ b/test/es-module/test-esm-exports.mjs @@ -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' }], @@ -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) { @@ -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; diff --git a/test/fixtures/node_modules/pkgexports-main/package.json b/test/fixtures/node_modules/pkgexports-main/package.json index 5546af0c84d7fd..471ae6f7137617 100644 --- a/test/fixtures/node_modules/pkgexports-main/package.json +++ b/test/fixtures/node_modules/pkgexports-main/package.json @@ -1,6 +1,6 @@ { "main": "./main.cjs", "exports": { - "module": "./module.mjs" + "import": "./module.mjs" } } diff --git a/test/fixtures/node_modules/pkgexports-sugar/main.js b/test/fixtures/node_modules/pkgexports-sugar/main.js index dfdd47b877319c..6655b150f2c8a6 100644 --- a/test/fixtures/node_modules/pkgexports-sugar/main.js +++ b/test/fixtures/node_modules/pkgexports-sugar/main.js @@ -1 +1,10 @@ +const assert = require('assert'); module.exports = 'main'; + +// Test self-resolve +require('@exports/sugar'); + +// Test self-resolve dynamic import +import('@exports/sugar').then(impt => { + assert.strictEqual(impt.default, 'main'); +}); diff --git a/test/fixtures/node_modules/pkgexports-sugar/package.json b/test/fixtures/node_modules/pkgexports-sugar/package.json index 5ebad0b4bda380..93f7d18f4a0de9 100644 --- a/test/fixtures/node_modules/pkgexports-sugar/package.json +++ b/test/fixtures/node_modules/pkgexports-sugar/package.json @@ -1,3 +1,4 @@ { + "name": "@exports/sugar", "exports": "./main.js" } diff --git a/test/fixtures/node_modules/pkgexports/package.json b/test/fixtures/node_modules/pkgexports/package.json index 37c28cdc1a950f..f7adc813ac81cc 100644 --- a/test/fixtures/node_modules/pkgexports/package.json +++ b/test/fixtures/node_modules/pkgexports/package.json @@ -1,6 +1,5 @@ { - "name": "@pkgexports/name", - "main": "./asdf.js", + "name": "pkg-exports", "exports": { "./hole": "./lib/hole.js", "./space": "./sp%20ce.js", @@ -19,6 +18,10 @@ "./nofallback1": [], "./nofallback2": [null, {}, "builtin:x"], "./nodemodules": "./node_modules/internalpkg/x.js", - "./condition": [{ "require": "./sp ce.js" }, "./asdf.js"] + "./condition": [{ "require": "./sp ce.js" }, "./asdf.js"], + "./resolve-self": { + "require": "./resolve-self.js", + "import": "./resolve-self.mjs" + } } } diff --git a/test/fixtures/node_modules/pkgexports/resolve-self.js b/test/fixtures/node_modules/pkgexports/resolve-self.js index 7bd3526aaa4e70..38ded86f3b8634 100644 --- a/test/fixtures/node_modules/pkgexports/resolve-self.js +++ b/test/fixtures/node_modules/pkgexports/resolve-self.js @@ -1 +1,2 @@ -require('@pkgexports/name/valid-cjs') +require('pkg-exports/valid-cjs'); +module.exports = 'self-cjs'; diff --git a/test/fixtures/node_modules/pkgexports/resolve-self.mjs b/test/fixtures/node_modules/pkgexports/resolve-self.mjs new file mode 100644 index 00000000000000..1bec0af0ce9066 --- /dev/null +++ b/test/fixtures/node_modules/pkgexports/resolve-self.mjs @@ -0,0 +1,2 @@ +import 'pkg-exports/valid-cjs'; +export default 'self-mjs';