From a5f328466a269ba017d84b6187ff25391fe8d891 Mon Sep 17 00:00:00 2001 From: guybedford Date: Wed, 7 Nov 2018 14:40:49 +0200 Subject: [PATCH] folder main reversion, error unification --- doc/api/errors.md | 13 +--- doc/api/esm.md | 71 +++++++------------ lib/internal/errors.js | 15 ++-- lib/internal/modules/esm/default_resolve.js | 15 ++-- src/module_wrap.cc | 78 +++++++++------------ src/node_errors.h | 2 +- test/es-module/test-esm-dynamic-import.js | 2 +- test/es-module/test-esm-loader-search.js | 17 ----- test/es-module/test-esm-main-lookup.mjs | 2 +- 9 files changed, 75 insertions(+), 140 deletions(-) delete mode 100644 test/es-module/test-esm-loader-search.js diff --git a/doc/api/errors.md b/doc/api/errors.md index 7cdee52e79..b41e6b34eb 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1369,13 +1369,6 @@ a `dynamicInstantiate` hook. A `MessagePort` was found in the object passed to a `postMessage()` call, but not provided in the `transferList` for that call. - -### ERR_MISSING_MODULE - -> Stability: 1 - Experimental - -An [ES6 module][] could not be resolved. - ### ERR_MISSING_PLATFORM_FOR_WORKER @@ -1383,12 +1376,12 @@ The V8 platform used by this instance of Node.js does not support creating Workers. This is caused by lack of embedder support for Workers. In particular, this error will not occur with standard builds of Node.js. - -### ERR_MODULE_RESOLUTION_LEGACY + +### ERR_MODULE_NOT_FOUND > Stability: 1 - Experimental -A failure occurred resolving imports in an [ES6 module][]. +An [ESM module][] could not be resolved. ### ERR_MULTIPLE_CALLBACK diff --git a/doc/api/esm.md b/doc/api/esm.md index 51feedfec7..c62d91096d 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -152,6 +152,7 @@ The resolver has the following properties: * Support for builtin module loading * Relative and absolute URL resolution * No default extensions +* No folder mains * Bare specifier package resolution lookup through node_modules * Avoids the package fallback which breaks package isolation. This is the problem that an import to `x/y.js` will keep checking subsequent node_modules @@ -174,59 +175,43 @@ Relative and absolute resolution without directory or extension resolution (URL resolution): * ESM_RESOLVE("./a.asdf", "file:///parent/path") -> "file:///parent/a.asdf" -* ESMRESOLVE("/a", "file:///parent/path") -> "file:///a" -* ESMRESOLVE("file:///a", "file:///parent/path") -> "file:///a" +* ESM_RESOLVE("/a", "file:///parent/path") -> "file:///a" +* ESM_RESOLVE("file:///a", "file:///parent/path") -> "file:///a" Package resolution: -1. ESMRESOLVE("pkg/x", "file:///path/to/project") -> +1. ESM_RESOLVE("pkg/x", "file:///path/to/project") -> "file:///path/to/nodemodules/pkg/x" (if it exists) -1. ESMRESOLVE("pkg/x", "file:///path/to/project") -> +1. ESM_RESOLVE("pkg/x", "file:///path/to/project") -> Not Found otherwise, even if "file:///path/nodemodules/pkg/x" exists. -Main resolution: - -1. ESMRESOLVE("pkg", "file:///path/to/project") -> - "file:///path/to/project/nodemodules/pkg/index.mjs" (if it exists) -1. ESMRESOLVE("pkg", "file:///path/to/project") -> Not Found otherwise, - even if "file:///path/to/nodemodules/pkg/index.mjs" exists. -1. ESMRESOLVE("file:///path/to/project") -> - "file:///path/to/project/index.mjs" (if it exists) -1. ESMRESOLVE("file:///path/to/project") -> - Not Found, if "file:///path/to/project/index.mjs" does not exist. - ### Resolver Algorithm The algorithm to resolve an ES module specifier is provided through _ESM_RESOLVE_: **ESM_RESOLVE**(_specifier_, _parentURL_) +> 1. Let _resolvedURL_ be _undefined_. > 1. If _specifier_ is a valid URL then, -> 1. Let _resolvedURL_ be the result of parsing and reserializing +> 1. Set _resolvedURL_ to the result of parsing and reserializing > _specifier_ as a URL. -> 1. Return the result of **PATH_RESOLVE**(_resolvedURL_). -> 1. If _specifier_ starts with _"/"_, _"./"_ or _"../"_ then, -> 1. Let _resolvedURL_ be the URL resolution of _specifier_ to _parentURL_. -> 1. Return the result of **PATH_RESOLVE**(_resolvedURL_). -> 1. Note: _specifier_ is now a bare specifier. -> 1. Return the result of **PACKAGE_RESOLVE**(_specifier_, _parentURL_). - -**PATH_RESOLVE**(_resolvedURL_) -> 1. If the file at _resolvedURL_ exists then, -> 1. Return _resolvedURL_. -> 1. Let _packageMainURL_ be the result of -> **PACKAGE_MAIN_RESOLVE**(_resolvedURL_). -> 1. If _packageMainURL_ is not _undefined_ then, -> 1. Return _packageMainURL_. -> 1. Throw a _Module Not Found_ error. +> 1. Otherwise, if _specifier_ starts with _"/"_, _"./"_ or _"../"_ then, +> 1. Set _resolvedURL_ to the URL resolution of _specifier_ to _parentURL_. +> 1. Otherwise, +> 1. Note: _specifier_ is now a bare specifier. +> 1. Set _resolvedURL_ the result of +> **PACKAGE_RESOLVE**(_specifier_, _parentURL_). +> 1. If the file at _resolvedURL_ does not exist then, +> 1. Throw a _Module Not Found_ error. +> 1. Return _resolvedURL_. **PACKAGE_RESOLVE**(_packageSpecifier_, _parentURL_) > 1. Assert: _packageSpecifier_ is a bare specifier. -> 1. If _packageSpecifier_ is an empty string then, -> 1. Throw a _Module Not Found_ error. > 1. Let _packageName_ be _undefined_. > 1. Let _packagePath_ be _undefined_. > 1. If _packageSpecifier_ does not start with _"@"_ then, +> 1. If _packageSpecifier_ is an empty string then, +> 1. Throw a _Invalid Package Name_ error. > 1. Set _packageName_ to the substring of _packageSpecifier_ until the > first _"/"_ separator or the end of the string. > 1. If _packageSpecifier_ starts with _"@"_ then, @@ -239,7 +224,7 @@ _ESM_RESOLVE_: > 1. Assert: _packageName_ is a valid package name or scoped package name. > 1. Assert: _packagePath_ is either empty, or a path without a leading > separator. -> 1. Note: Further package name encoding validations can be added here. +> 1. Note: Further package name validations can be added here. > 1. If _packagePath_ is empty and _packageName_ is a Node.js builtin > module then, > 1. Return _"node:${packageName}"_. @@ -254,23 +239,15 @@ _ESM_RESOLVE_: > 1. Continue the next loop iteration. > 1. If _packagePath_ is empty then, > 1. Let _url_ be the result of **PACKAGE_MAIN_RESOLVE**(_packageURL_). -> 1. If _url_ is not _undefined_ then, -> 1. Return _url_. +> 1. If _url_ is _undefined_ then, +> 1. Throw a _Module Not Found_ error. +> 1. Return _url_. > 1. Otherwise, -> 1. Let _url_ be equal to the URL resolution of _packagePath_ in -> _packageURL_. -> 1. If the file at _url_ exists then, -> 1. Return _url_. -> 1. Otherwise, if _url_ is a directory then, -> 1. Let _url_ be the result of **PACKAGE_MAIN_RESOLVE**(_url_). -> 1. If _url_ is not _undefined_ then, -> 1. Return _url_. -> 1. Throw a _Module Not Found_ error. +> 1. Return the URL resolution of _packagePath_ in _packageURL_. > 1. Throw a _Module Not Found_ error. **PACKAGE_MAIN_RESOLVE**(_packageURL_) -> 1. If the file at _"${packageURL}/index.mjs"_ exists then, -> 1. Return _"${packageURL}/index.mjs"_. +> 1. Note: Main resolution to be implemented here. > 1. Return _undefined_. diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 4cbfc92048..d0b30d4305 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -762,11 +762,13 @@ E('ERR_MISSING_ARGS', E('ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK', 'The ES Module loader may not return a format of \'dynamic\' when no ' + 'dynamicInstantiate function was provided', Error); -E('ERR_MISSING_MODULE', 'Cannot find module %s', Error); -E('ERR_MODULE_RESOLUTION_LEGACY', - 'Cannot find module \'%s\' imported from %s.' + - ' Legacy behavior in require() would have found it at %s', - Error); +E('ERR_MODULE_NOT_FOUND', (module, base, legacyResolution) => { + let msg = `Cannot find module '${module}' imported from ${base}.`; + if (legacyResolution) + msg += ' Legacy behavior in require() would have found it at ' + + legacyResolution; + return msg; +}, Error); E('ERR_MULTIPLE_CALLBACK', 'Callback called multiple times', Error); E('ERR_NAPI_CONS_FUNCTION', 'Constructor must be a function', TypeError); E('ERR_NAPI_INVALID_DATAVIEW_ARGS', @@ -859,7 +861,8 @@ E('ERR_UNKNOWN_CREDENTIAL', '%s identifier does not exist: %s', Error); E('ERR_UNKNOWN_ENCODING', 'Unknown encoding: %s', TypeError); // This should probably be a `TypeError`. -E('ERR_UNKNOWN_FILE_EXTENSION', 'Unknown file extension: %s', Error); +E('ERR_UNKNOWN_FILE_EXTENSION', 'Unknown file extension: \'%s\' imported ' + + 'from %s', Error); E('ERR_UNKNOWN_MODULE_FORMAT', 'Unknown module format: %s', RangeError); E('ERR_UNKNOWN_SIGNAL', 'Unknown signal: %s', TypeError); E('ERR_UNKNOWN_STDIN_TYPE', 'Unknown stdin file type', Error); diff --git a/lib/internal/modules/esm/default_resolve.js b/lib/internal/modules/esm/default_resolve.js index 2ce9453680..484ec0bc51 100644 --- a/lib/internal/modules/esm/default_resolve.js +++ b/lib/internal/modules/esm/default_resolve.js @@ -9,8 +9,7 @@ const { realpathSync } = require('fs'); const preserveSymlinks = !!process.binding('config').preserveSymlinks; const preserveSymlinksMain = !!process.binding('config').preserveSymlinksMain; const { - ERR_MISSING_MODULE, - ERR_MODULE_RESOLUTION_LEGACY, + ERR_MODULE_NOT_FOUND, ERR_UNKNOWN_FILE_EXTENSION } = require('internal/errors').codes; const { resolve: moduleWrapResolve } = internalBinding('module_wrap'); @@ -19,10 +18,6 @@ const { pathToFileURL, fileURLToPath } = require('internal/url'); const realpathCache = new Map(); function search(target, base) { - if (base === undefined) { - // We cannot search without a base. - throw new ERR_MISSING_MODULE(target); - } try { return moduleWrapResolve(target, base); } catch (e) { @@ -34,8 +29,7 @@ function search(target, base) { tmpMod.paths = CJSmodule._nodeModulePaths( new URL('./', questionedBase).pathname); const found = CJSmodule._resolveFilename(target, tmpMod); - error = new ERR_MODULE_RESOLUTION_LEGACY(target, - fileURLToPath(base), found); + error = new ERR_MODULE_NOT_FOUND(target, fileURLToPath(base), found); } catch { // ignore } @@ -78,12 +72,11 @@ function resolve(specifier, parentURL) { if (isMain) format = 'cjs'; else - throw new ERR_UNKNOWN_FILE_EXTENSION(url.pathname); + throw new ERR_UNKNOWN_FILE_EXTENSION(url.pathname, + fileURLToPath(parentURL)); } return { url: `${url}`, format }; } module.exports = resolve; -// exported for tests -module.exports.search = search; diff --git a/src/module_wrap.cc b/src/module_wrap.cc index c05dfcb34d..467e28e53f 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -486,26 +486,21 @@ DescriptorType CheckDescriptor(const std::string& path) { } inline Maybe PackageMainResolve(const URL& search) { - std::string path = search.path(); - std::string last_segment = path.substr(path.find_last_of('/') + 1); - URL main_url = URL("./" + last_segment + "/index.mjs", search); - if (CheckDescriptor(main_url.ToFilePath()) == FILE) - return Just(main_url); return Nothing(); } Maybe PackageResolve(Environment* env, const std::string& specifier, const URL& base) { - if (specifier.length() == 0) return Nothing(); size_t sep_index = specifier.find('/'); + if (specifier[0] == '@' && sep_index == std::string::npos || + specifier.length() == 0) { + std::string msg = "Invalid package name '" + specifier + + "' imported from " + base.ToFilePath(); + node::THROW_ERR_INVALID_PACKAGE_NAME(env, msg.c_str()); + return Nothing(); + } if (specifier[0] == '@') { - if (sep_index == std::string::npos) { - std::string msg = "Invalid package name '" + specifier + - "' imported from " + base.ToFilePath(); - node::THROW_ERR_INVALID_PACKAGE_NAME(env, msg.c_str()); - return Nothing(); - } sep_index = specifier.find('/', sep_index + 1); } std::string pkg_name = specifier.substr(0, @@ -527,24 +522,21 @@ Maybe PackageResolve(Environment* env, } else { url = pkg_url; } + DescriptorType check; if (pkg_path.length()) { DescriptorType check = CheckDescriptor(url.ToFilePath()); - if (check == FILE) { - return Just(url); - } else if (check == DIRECTORY) { - Maybe main_url = PackageMainResolve(url); - if (!main_url.IsNothing()) return main_url; - } + if (check == FILE) return Just(url); + if (check == NONE) check = CheckDescriptor(pkg_url.ToFilePath()); } else { Maybe main_url = PackageMainResolve(url); if (!main_url.IsNothing()) return main_url; + check = CheckDescriptor(pkg_url.ToFilePath()); } // throw not found if we did match a package directory - DescriptorType check = CheckDescriptor(pkg_url.ToFilePath()); if (check == DIRECTORY) { std::string msg = "Cannot find module '" + url.ToFilePath() + "' imported from " + base.ToFilePath(); - node::THROW_ERR_MISSING_MODULE(env, msg.c_str()); + node::THROW_ERR_MODULE_NOT_FOUND(env, msg.c_str()); return Nothing(); } last_path = parent.path(); @@ -554,38 +546,32 @@ Maybe PackageResolve(Environment* env, return Nothing(); } -Maybe PathResolve(Environment* env, - const URL& url, - const URL& base) { - std::string path = url.ToFilePath(); - DescriptorType check = CheckDescriptor(path); - if (check == FILE) return Just(url); - - if (check == DIRECTORY) { - Maybe url_main = PackageMainResolve(url); - if (!url_main.IsNothing()) return url_main; - } - - std::string msg = "Cannot find module '" + path + - "' imported from " + base.ToFilePath(); - node::THROW_ERR_MISSING_MODULE(env, msg.c_str()); - return Nothing(); -} - } // anonymous namespace Maybe Resolve(Environment* env, const std::string& specifier, const URL& base) { - URL pure_url(specifier); - if (!(pure_url.flags() & URL_FLAGS_FAILED)) { - return PathResolve(env, pure_url, base); - } + // Order swapped from spec for minor perf gain. + // Ok since relative URLs cannot parse as URLs. + URL resolved; if (ShouldBeTreatedAsRelativeOrAbsolutePath(specifier)) { - URL resolved(specifier, base); - return PathResolve(env, resolved, base); + resolved = URL(specifier, base); + } else { + URL pure_url(specifier); + if (!(pure_url.flags() & URL_FLAGS_FAILED)) { + resolved = pure_url; + } else { + return PackageResolve(env, specifier, base); + } + } + DescriptorType check = CheckDescriptor(resolved.ToFilePath()); + if (check != FILE) { + std::string msg = "Cannot find module '" + resolved.ToFilePath() + + "' imported from " + base.ToFilePath(); + node::THROW_ERR_MODULE_NOT_FOUND(env, msg.c_str()); + return Nothing(); } - return PackageResolve(env, specifier, base); + return Just(resolved); } void ModuleWrap::Resolve(const FunctionCallbackInfo& args) { @@ -616,7 +602,7 @@ void ModuleWrap::Resolve(const FunctionCallbackInfo& args) { (result.FromJust().flags() & URL_FLAGS_FAILED)) { std::string msg = "Cannot find module '" + specifier_std + "' imported from " + url.ToFilePath(); - node::THROW_ERR_MISSING_MODULE(env, msg.c_str()); + node::THROW_ERR_MODULE_NOT_FOUND(env, msg.c_str()); try_catch.ReThrow(); return; } diff --git a/src/node_errors.h b/src/node_errors.h index cff3d55932..b522629065 100644 --- a/src/node_errors.h +++ b/src/node_errors.h @@ -33,8 +33,8 @@ namespace node { V(ERR_MEMORY_ALLOCATION_FAILED, Error) \ V(ERR_MISSING_ARGS, TypeError) \ V(ERR_MISSING_MESSAGE_PORT_IN_TRANSFER_LIST, TypeError) \ - V(ERR_MISSING_MODULE, Error) \ V(ERR_MISSING_PLATFORM_FOR_WORKER, Error) \ + V(ERR_MODULE_NOT_FOUND, Error) \ V(ERR_OUT_OF_RANGE, RangeError) \ V(ERR_SCRIPT_EXECUTION_INTERRUPTED, Error) \ V(ERR_SCRIPT_EXECUTION_TIMEOUT, Error) \ diff --git a/test/es-module/test-esm-dynamic-import.js b/test/es-module/test-esm-dynamic-import.js index 644dd5e4e7..effe37c70c 100644 --- a/test/es-module/test-esm-dynamic-import.js +++ b/test/es-module/test-esm-dynamic-import.js @@ -19,7 +19,7 @@ function expectErrorProperty(result, propertyKey, value) { } function expectMissingModuleError(result) { - expectErrorProperty(result, 'code', 'MODULE_NOT_FOUND'); + expectErrorProperty(result, 'code', 'ERR_MODULE_NOT_FOUND'); } function expectInvalidUrlError(result) { diff --git a/test/es-module/test-esm-loader-search.js b/test/es-module/test-esm-loader-search.js deleted file mode 100644 index 0ca8990cb7..0000000000 --- a/test/es-module/test-esm-loader-search.js +++ /dev/null @@ -1,17 +0,0 @@ -'use strict'; -// Flags: --expose-internals - -// This test ensures that search throws errors appropriately - -const common = require('../common'); - -const { search } = require('internal/modules/esm/default_resolve'); - -common.expectsError( - () => search('target', undefined), - { - code: 'ERR_MISSING_MODULE', - type: Error, - message: 'Cannot find module target' - } -); diff --git a/test/es-module/test-esm-main-lookup.mjs b/test/es-module/test-esm-main-lookup.mjs index 76c6263853..19c025beab 100644 --- a/test/es-module/test-esm-main-lookup.mjs +++ b/test/es-module/test-esm-main-lookup.mjs @@ -8,7 +8,7 @@ async function main() { try { mod = await import('../fixtures/es-modules/pjson-main'); } catch (e) { - assert.strictEqual(e.code, 'MODULE_NOT_FOUND'); + assert.strictEqual(e.code, 'ERR_MODULE_NOT_FOUND'); } assert.strictEqual(mod, undefined);