From 839b837d267ad5ee8d2a4a57558833a1cd2e919d Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Tue, 17 Dec 2019 01:58:19 -0500 Subject: [PATCH 1/5] module: self resolve bug fix and esm ordering --- doc/api/esm.md | 30 ++++----- lib/internal/modules/cjs/loader.js | 66 ++++++++++--------- src/module_wrap.cc | 46 ++++--------- src/node_file.cc | 1 + test/es-module/test-esm-exports.mjs | 12 ++-- .../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 + 11 files changed, 93 insertions(+), 88 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 5245cef465e3c7..992336bd3277e2 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -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 +> **SELF_REFERENCE_RESOLE**(_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_. @@ -1227,29 +1230,24 @@ _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. 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. 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 an empty result. **PACKAGE_MAIN_RESOLVE**(_packageURL_, _pjson_) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index acdd14b959895b..6ddb749ff8d2ce 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -431,12 +431,12 @@ 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]); + const { data: pkg, path: basePath } = readPackageScope(parentPath) || {}; if (!pkg) return false; if (typeof pkg.name !== 'string') return false; @@ -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 @@ -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; }; @@ -1008,24 +1004,32 @@ Module._resolveFilename = function(request, parent, isMain, options) { // 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 (filename) return filename; + 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; + 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..2ce8fec896d7bc 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,33 +1171,14 @@ 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 (!pkg_subpath.length()) { return PackageMainResolve(env, pjson_url, *pcfg, base); - } else if (found_pjson) { + } else { if (!pcfg->exports.IsEmpty()) { - return PackageExportsResolve(env, pjson_url, subpath, *pcfg, base); + return PackageExportsResolve(env, pjson_url, pkg_subpath, *pcfg, base); } else { - return FinalizeResolution(env, URL(subpath, pjson_url), base); + return FinalizeResolution(env, URL(pkg_subpath, pjson_url), base); } } } @@ -1245,6 +1226,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 +1266,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..47bbc31dfa49db 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; @@ -173,7 +176,7 @@ function assertIncludes(actual, expected) { '/node_modules/pkgexports/resolve-self.js', 'Package name self resolution', ], -].forEach(([flag, file, message]) => { +].forEach(([flag, file, message], index) => { const child = spawn(process.execPath, [flag, path(file)]); let stderr = ''; @@ -182,6 +185,7 @@ function assertIncludes(actual, expected) { stderr += data; }); child.on('close', (code, signal) => { + console.log(stderr.toString()); strictEqual(code, 0); strictEqual(signal, null); ok(stderr.toString().includes( 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'; From b3889b8a00d979110adef25e08f6f36758cd9602 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Tue, 17 Dec 2019 16:41:03 -0500 Subject: [PATCH 2/5] remove unused index reference and logging --- test/es-module/test-esm-exports.mjs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/es-module/test-esm-exports.mjs b/test/es-module/test-esm-exports.mjs index 47bbc31dfa49db..1595d1f6a1e57b 100644 --- a/test/es-module/test-esm-exports.mjs +++ b/test/es-module/test-esm-exports.mjs @@ -176,7 +176,7 @@ function assertIncludes(actual, expected) { '/node_modules/pkgexports/resolve-self.js', 'Package name self resolution', ], -].forEach(([flag, file, message], index) => { +].forEach(([flag, file, message]) => { const child = spawn(process.execPath, [flag, path(file)]); let stderr = ''; @@ -185,7 +185,6 @@ function assertIncludes(actual, expected) { stderr += data; }); child.on('close', (code, signal) => { - console.log(stderr.toString()); strictEqual(code, 0); strictEqual(signal, null); ok(stderr.toString().includes( From 8cca5c9b4f22c9626c0a3c8d91788dfdc1e00c7a Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Wed, 18 Dec 2019 20:10:45 -0500 Subject: [PATCH 3/5] strict self resolution opt-in --- doc/api/esm.md | 16 ++++++++++++---- doc/api/modules.md | 16 +++++++++------- lib/internal/modules/cjs/loader.js | 16 +++++++++++++--- src/module_wrap.cc | 12 ++++++++++-- 4 files changed, 44 insertions(+), 16 deletions(-) diff --git a/doc/api/esm.md b/doc/api/esm.md index 992336bd3277e2..be8f867cbea16e 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -1207,7 +1207,8 @@ _defaultEnv_ is the conditional environment name priority array, > encoded strings for _"/"_ or _"\\"_, then > 1. Throw an _Invalid Specifier_ error. > 1. Set _selfUrl_ to the result of -> **SELF_REFERENCE_RESOLE**(_packageName_, _packageSubpath_, _parentURL_). +> **SELF_REFERENCE_RESOLE**(_packageName_, _packageSubpath_, _parentURL_, +> **true**). > 1. If _selfUrl_ isn't empty, return _selfUrl_. > 1. If _packageSubpath_ is _undefined_ and _packageName_ is a Node.js builtin > module, then @@ -1230,14 +1231,21 @@ _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**(_packageName_, _packageSubpath_, _parentURL_, +> **false**). +> 1. If _selfUrl_ isn't empty, return _selfUrl_. > 1. Throw a _Module Not Found_ error. -**SELF_REFERENCE_RESOLVE**(_packageName_, _packageSubpath_, _parentURL_) +**SELF_REFERENCE_RESOLVE**(_packageName_, _packageSubpath_, _parentURL_, + _encapsulated_) > 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. If _encapsulated_ is **true** and _pjson_ does not include an +> _"exports"_ property, then 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_). @@ -1248,7 +1256,7 @@ _defaultEnv_ is the conditional environment name priority array, > 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. Otherwise, return **undefined**. **PACKAGE_MAIN_RESOLVE**(_packageURL_, _pjson_) diff --git a/doc/api/modules.md b/doc/api/modules.md index 5653fe8f4245bd..3864749e90b92f 100644 --- a/doc/api/modules.md +++ b/doc/api/modules.md @@ -160,9 +160,10 @@ 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), true) +5. LOAD_NODE_MODULES(X, dirname(Y)) +6. LOAD_SELF_REFERENCE(X, dirname(Y), false) +7. THROW "not found" LOAD_AS_FILE(X) 1. If X is a file, load X as JavaScript text. STOP @@ -203,11 +204,12 @@ NODE_MODULES_PATHS(START) d. let I = I - 1 5. return DIRS -LOAD_SELF_REFERENCE(X, START) +LOAD_SELF_REFERENCE(X, START, ENCAPSULATED) 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 ENCAPSULATED is true and 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 6ddb749ff8d2ce..e2eb83dade0bf9 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -431,13 +431,13 @@ function resolveBasePath(basePath, exts, isMain, trailingSlash, request) { return filename; } -function trySelf(parentPath, isMain, request) { +function trySelf(parentPath, isMain, request, encapsulated) { if (!experimentalSelf) { return false; } const { data: pkg, path: basePath } = readPackageScope(parentPath) || {}; - if (!pkg) return false; + if (!pkg || (encapsulated && 'exports' in pkg === false)) return false; if (typeof pkg.name !== 'string') return false; let expansion; @@ -1003,7 +1003,17 @@ Module._resolveFilename = function(request, parent, isMain, options) { } // Look up the filename first, since that's the cache key. - const filename = Module._findPath(request, paths, isMain); + if (parent && parent.filename) { + const filename = trySelf(parent.filename, isMain, request, true); + 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; + } + } + const filename = Module._findPath(request, paths, isMain, false); if (filename) return filename; if (parent && parent.filename) { const filename = trySelf(parent.filename, isMain, request); diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 2ce8fec896d7bc..66a1e33633656d 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -1152,7 +1152,8 @@ Maybe PackageExportsResolve(Environment* env, Maybe ResolveSelf(Environment* env, const std::string& pkg_name, const std::string& pkg_subpath, - const URL& base) { + const URL& base, + bool encapsulated) { if (!env->options()->experimental_resolve_self) { return Nothing(); } @@ -1172,6 +1173,7 @@ Maybe ResolveSelf(Environment* env, } } if (!found_pjson || pcfg->name != pkg_name) return Nothing(); + if (encapsulated && pcfg->exports.IsEmpty()) return Nothing(); if (!pkg_subpath.length()) { return PackageMainResolve(env, pjson_url, *pcfg, base); } else { @@ -1227,7 +1229,7 @@ Maybe PackageResolve(Environment* env, pkg_subpath = "." + specifier.substr(sep_index); } - Maybe self_url = ResolveSelf(env, pkg_name, pkg_subpath, base); + Maybe self_url = ResolveSelf(env, pkg_name, pkg_subpath, base, true); if (self_url.IsJust()) { ProcessEmitExperimentalWarning(env, "Package name self resolution"); return self_url; @@ -1266,6 +1268,12 @@ Maybe PackageResolve(Environment* env, // Cross-platform root check. } while (pjson_path.length() != last_path.length()); + self_url = ResolveSelf(env, pkg_name, pkg_subpath, base, false); + 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()); From ff11612af6ee2185a065e0b7afe4094c331ce1df Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Wed, 18 Dec 2019 20:35:22 -0500 Subject: [PATCH 4/5] always encapsulate on exports --- doc/api/esm.md | 7 +++---- doc/api/modules.md | 7 +++---- lib/internal/modules/cjs/loader.js | 19 +++++-------------- src/module_wrap.cc | 19 ++++--------------- 4 files changed, 15 insertions(+), 37 deletions(-) diff --git a/doc/api/esm.md b/doc/api/esm.md index be8f867cbea16e..d149c743989974 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -1237,15 +1237,14 @@ _defaultEnv_ is the conditional environment name priority array, > 1. If _selfUrl_ isn't empty, return _selfUrl_. > 1. Throw a _Module Not Found_ error. -**SELF_REFERENCE_RESOLVE**(_packageName_, _packageSubpath_, _parentURL_, - _encapsulated_) +**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 **undefined**. > 1. Let _pjson_ be the result of **READ_PACKAGE_JSON**(_packageURL_). -> 1. If _encapsulated_ is **true** and _pjson_ does not include an -> _"exports"_ property, then return **undefined**. +> 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_). diff --git a/doc/api/modules.md b/doc/api/modules.md index 3864749e90b92f..b57e5890379d18 100644 --- a/doc/api/modules.md +++ b/doc/api/modules.md @@ -160,9 +160,8 @@ 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_SELF_REFERENCE(X, dirname(Y), true) +4. LOAD_SELF_REFERENCE(X, dirname(Y)) 5. LOAD_NODE_MODULES(X, dirname(Y)) -6. LOAD_SELF_REFERENCE(X, dirname(Y), false) 7. THROW "not found" LOAD_AS_FILE(X) @@ -204,10 +203,10 @@ NODE_MODULES_PATHS(START) d. let I = I - 1 5. return DIRS -LOAD_SELF_REFERENCE(X, START, ENCAPSULATED) +LOAD_SELF_REFERENCE(X, START) 1. Find the closest package scope to START. 2. If no scope was found, return. -3. If ENCAPSULATED is true and the `package.json` has no "exports", 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 e2eb83dade0bf9..19cbc7ab54d653 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -431,13 +431,13 @@ function resolveBasePath(basePath, exts, isMain, trailingSlash, request) { return filename; } -function trySelf(parentPath, isMain, request, encapsulated) { +function trySelf(parentPath, isMain, request) { if (!experimentalSelf) { return false; } const { data: pkg, path: basePath } = readPackageScope(parentPath) || {}; - if (!pkg || (encapsulated && 'exports' in pkg === false)) return false; + if (!pkg || 'exports' in pkg === false) return false; if (typeof pkg.name !== 'string') return false; let expansion; @@ -1002,9 +1002,8 @@ Module._resolveFilename = function(request, parent, isMain, options) { paths = Module._resolveLookupPaths(request, parent); } - // Look up the filename first, since that's the cache key. if (parent && parent.filename) { - const filename = trySelf(parent.filename, isMain, request, true); + const filename = trySelf(parent.filename, isMain, request); if (filename) { emitExperimentalWarning('Package name self resolution'); const cacheKey = request + '\x00' + @@ -1013,18 +1012,10 @@ Module._resolveFilename = function(request, parent, isMain, options) { 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; - 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; - } - } const requireStack = []; for (let cursor = parent; cursor; diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 66a1e33633656d..454476a1e97815 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -1152,8 +1152,7 @@ Maybe PackageExportsResolve(Environment* env, Maybe ResolveSelf(Environment* env, const std::string& pkg_name, const std::string& pkg_subpath, - const URL& base, - bool encapsulated) { + const URL& base) { if (!env->options()->experimental_resolve_self) { return Nothing(); } @@ -1173,15 +1172,11 @@ Maybe ResolveSelf(Environment* env, } } if (!found_pjson || pcfg->name != pkg_name) return Nothing(); - if (encapsulated && pcfg->exports.IsEmpty()) return Nothing(); + if (pcfg->exports.IsEmpty()) return Nothing(); if (!pkg_subpath.length()) { return PackageMainResolve(env, pjson_url, *pcfg, base); } else { - if (!pcfg->exports.IsEmpty()) { - return PackageExportsResolve(env, pjson_url, pkg_subpath, *pcfg, base); - } else { - return FinalizeResolution(env, URL(pkg_subpath, pjson_url), base); - } + return PackageExportsResolve(env, pjson_url, pkg_subpath, *pcfg, base); } } @@ -1229,7 +1224,7 @@ Maybe PackageResolve(Environment* env, pkg_subpath = "." + specifier.substr(sep_index); } - Maybe self_url = ResolveSelf(env, pkg_name, pkg_subpath, base, true); + Maybe self_url = ResolveSelf(env, pkg_name, pkg_subpath, base); if (self_url.IsJust()) { ProcessEmitExperimentalWarning(env, "Package name self resolution"); return self_url; @@ -1268,12 +1263,6 @@ Maybe PackageResolve(Environment* env, // Cross-platform root check. } while (pjson_path.length() != last_path.length()); - self_url = ResolveSelf(env, pkg_name, pkg_subpath, base, false); - 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()); From 1ddbc0e9e7cf7c4b8cb96c3946e4dc695b29adc3 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Wed, 18 Dec 2019 20:51:16 -0500 Subject: [PATCH 5/5] fixup remaining doc lines --- doc/api/esm.md | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/doc/api/esm.md b/doc/api/esm.md index d149c743989974..c4315453c8f9a4 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -1207,8 +1207,7 @@ _defaultEnv_ is the conditional environment name priority array, > encoded strings for _"/"_ or _"\\"_, then > 1. Throw an _Invalid Specifier_ error. > 1. Set _selfUrl_ to the result of -> **SELF_REFERENCE_RESOLE**(_packageName_, _packageSubpath_, _parentURL_, -> **true**). +> **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 @@ -1231,10 +1230,6 @@ _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**(_packageName_, _packageSubpath_, _parentURL_, -> **false**). -> 1. If _selfUrl_ isn't empty, return _selfUrl_. > 1. Throw a _Module Not Found_ error. **SELF_REFERENCE_RESOLVE**(_packageName_, _packageSubpath_, _parentURL_)