From fd557200241438198460dec788d4c75aba09601d Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Wed, 24 Oct 2018 16:50:32 +0100 Subject: [PATCH] esm: minimal resolver spec and implementation refinements PR URL: https://github.com/nodejs/ecmascript-modules/pull/12 --- doc/api/errors.md | 13 +- doc/api/esm.md | 50 ++++++- lib/internal/errors.js | 16 +- lib/internal/modules/esm/default_resolve.js | 27 +--- src/module_wrap.cc | 158 +++++++------------- 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, 118 insertions(+), 169 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 3b4ea6156f..3a277e5b3c 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1393,13 +1393,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 @@ -1407,12 +1400,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 6eaee64920..d2277044a9 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -63,10 +63,6 @@ ESM must have the `.mjs` extension. You must provide a file extension when using the `import` keyword. -### No importing directories - -There is no support for importing directories. - ### No NODE_PATH `NODE_PATH` is not part of resolving `import` specifiers. Please use symlinks @@ -146,6 +142,52 @@ fs.readFileSync = () => Buffer.from('Hello, ESM'); fs.readFileSync === readFileSync; ``` +## Resolution Algorithm + +### Features + +The resolver has the following properties: + +* FileURL-based resolution as is used by ES modules +* Support for builtin module loading +* Relative and absolute URL resolution +* No default extensions +* No folder mains +* Bare specifier package resolution lookup through node_modules + +### 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. Set _resolvedURL_ to the result of parsing and reserializing +> _specifier_ as a URL. +> 1. Otherwise, if _specifier_ starts with _"/"_, _"./"_ or _"../"_ then, +> 1. Set _resolvedURL_ to the URL resolution of _specifier_ relative 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 a Node.js builtin module then, +> 1. Return the string _"node:"_ concatenated with _packageSpecifier_. +> 1. While _parentURL_ contains a non-empty _pathname_, +> 1. Set _parentURL_ to the parent folder URL of _parentURL_. +> 1. Let _packageURL_ be the URL resolution of the string concatenation of +> _parentURL_, _"/node_modules/"_ and _"packageSpecifier"_. +> 1. If the file at _packageURL_ exists then, +> 1. Return _packageURL_. +> 1. Throw a _Module Not Found_ error. + [Node.js EP for ES Modules]: https://github.com/nodejs/node-eps/blob/master/002-es-modules.md [`module.createRequireFromPath()`]: modules.html#modules_module_createrequirefrompath_filename [ESM Minimal Kernel]: https://github.com/nodejs/modules/blob/master/doc/plan-for-new-modules-implementation.md diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 0ed23a9438..9c7aa71bd4 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -800,11 +800,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', - '%s not found by import in %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', @@ -898,11 +900,13 @@ E('ERR_UNHANDLED_ERROR', if (err === undefined) return msg; return `${msg} (${err})`; }, Error); +// This should probably be a `TypeError`. 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 5976cab99c..4d47d8124e 100644 --- a/lib/internal/modules/esm/default_resolve.js +++ b/lib/internal/modules/esm/default_resolve.js @@ -10,21 +10,15 @@ const { getOptionValue } = require('internal/options'); const preserveSymlinks = getOptionValue('--preserve-symlinks'); const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main'); 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'); -const StringStartsWith = Function.call.bind(String.prototype.startsWith); 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) { @@ -36,7 +30,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, base, found); + error = new ERR_MODULE_NOT_FOUND(target, fileURLToPath(base), found); } catch { // ignore } @@ -57,16 +51,8 @@ function resolve(specifier, parentURL) { }; } - let url; - try { - url = search(specifier, - parentURL || pathToFileURL(`${process.cwd()}/`).href); - } catch (e) { - if (typeof e.message === 'string' && - StringStartsWith(e.message, 'Cannot find module')) - e.code = 'MODULE_NOT_FOUND'; - throw e; - } + let url = search(specifier, + parentURL || pathToFileURL(`${process.cwd()}/`).href); const isMain = parentURL === undefined; @@ -87,12 +73,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 1768a3da33..2b6628d975 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -467,104 +467,37 @@ std::string ReadFile(uv_file file) { return contents; } -enum CheckFileOptions { - LEAVE_OPEN_AFTER_CHECK, - CLOSE_AFTER_CHECK +enum DescriptorType { + NONE, + FILE, + DIRECTORY }; -Maybe CheckFile(const std::string& path, - CheckFileOptions opt = CLOSE_AFTER_CHECK) { +DescriptorType CheckDescriptor(const std::string& path) { uv_fs_t fs_req; - if (path.empty()) { - return Nothing(); - } - - uv_file fd = uv_fs_open(nullptr, &fs_req, path.c_str(), O_RDONLY, 0, nullptr); - uv_fs_req_cleanup(&fs_req); - - if (fd < 0) { - return Nothing(); - } - - uv_fs_fstat(nullptr, &fs_req, fd, nullptr); - uint64_t is_directory = fs_req.statbuf.st_mode & S_IFDIR; - uv_fs_req_cleanup(&fs_req); - - if (is_directory) { - CHECK_EQ(0, uv_fs_close(nullptr, &fs_req, fd, nullptr)); - uv_fs_req_cleanup(&fs_req); - return Nothing(); - } - - if (opt == CLOSE_AFTER_CHECK) { - CHECK_EQ(0, uv_fs_close(nullptr, &fs_req, fd, nullptr)); + int rc = uv_fs_stat(nullptr, &fs_req, path.c_str(), nullptr); + if (rc == 0) { + uint64_t is_directory = fs_req.statbuf.st_mode & S_IFDIR; uv_fs_req_cleanup(&fs_req); + return is_directory ? DIRECTORY : FILE; } - - return Just(fd); -} - -enum ResolveExtensionsOptions { - TRY_EXACT_NAME, - ONLY_VIA_EXTENSIONS -}; - -inline bool ResolvesToFile(const URL& search) { - std::string filePath = search.ToFilePath(); - Maybe check = CheckFile(filePath); - return !check.IsNothing(); -} - -template -Maybe ResolveExtensions(const URL& search) { - if (options == TRY_EXACT_NAME) { - std::string filePath = search.ToFilePath(); - Maybe check = CheckFile(filePath); - if (!check.IsNothing()) { - return Just(search); - } - } - - for (const char* extension : EXTENSIONS) { - URL guess(search.path() + extension, &search); - Maybe check = CheckFile(guess.ToFilePath()); - if (!check.IsNothing()) { - return Just(guess); - } - } - - return Nothing(); -} - -inline Maybe ResolveIndex(const URL& search) { - return ResolveExtensions(URL("index", search)); + uv_fs_req_cleanup(&fs_req); + return NONE; } -Maybe ResolveModule(Environment* env, - const std::string& specifier, - const URL& base) { +Maybe PackageResolve(Environment* env, + const std::string& specifier, + const URL& base) { URL parent(".", base); - URL dir(""); + std::string last_path; do { - dir = parent; - Maybe check = - Resolve(env, "./node_modules/" + specifier, dir); - if (!check.IsNothing()) { - const size_t limit = specifier.find('/'); - const size_t spec_len = - limit == std::string::npos ? specifier.length() : - limit + 1; - std::string chroot = - dir.path() + "node_modules/" + specifier.substr(0, spec_len); - if (check.FromJust().path().substr(0, chroot.length()) != chroot) { - return Nothing(); - } - return check; - } else { - // TODO(bmeck) PREVENT FALLTHROUGH - } - parent = URL("..", &dir); - } while (parent.path() != dir.path()); + URL pkg_url("./node_modules/" + specifier, &parent); + DescriptorType check = CheckDescriptor(pkg_url.ToFilePath()); + if (check == FILE) return Just(pkg_url); + last_path = parent.path(); + parent = URL("..", &parent); + // cross-platform root check + } while (parent.path() != last_path); return Nothing(); } @@ -573,26 +506,27 @@ Maybe ResolveModule(Environment* env, Maybe Resolve(Environment* env, const std::string& specifier, const URL& base) { - URL pure_url(specifier); - if (!(pure_url.flags() & URL_FLAGS_FAILED)) { - // just check existence, without altering - Maybe check = CheckFile(pure_url.ToFilePath()); - if (check.IsNothing()) { - return Nothing(); + // Order swapped from spec for minor perf gain. + // Ok since relative URLs cannot parse as URLs. + URL resolved; + if (ShouldBeTreatedAsRelativeOrAbsolutePath(specifier)) { + 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); } - return Just(pure_url); } - if (specifier.length() == 0) { + 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(); } - if (ShouldBeTreatedAsRelativeOrAbsolutePath(specifier)) { - URL resolved(specifier, base); - if (ResolvesToFile(resolved)) - return Just(resolved); - return Nothing(); - } else { - return ResolveModule(env, specifier, base); - } + return Just(resolved); } void ModuleWrap::Resolve(const FunctionCallbackInfo& args) { @@ -614,10 +548,18 @@ void ModuleWrap::Resolve(const FunctionCallbackInfo& args) { env, "second argument is not a URL string"); } + TryCatch try_catch(env->isolate()); Maybe result = node::loader::Resolve(env, specifier_std, url); - if (result.IsNothing() || (result.FromJust().flags() & URL_FLAGS_FAILED)) { - std::string msg = "Cannot find module " + specifier_std; - return node::THROW_ERR_MISSING_MODULE(env, msg.c_str()); + if (try_catch.HasCaught()) { + try_catch.ReThrow(); + return; + } else if (result.IsNothing() || + (result.FromJust().flags() & URL_FLAGS_FAILED)) { + std::string msg = "Cannot find module '" + specifier_std + + "' imported from " + url.ToFilePath(); + node::THROW_ERR_MODULE_NOT_FOUND(env, msg.c_str()); + try_catch.ReThrow(); + return; } args.GetReturnValue().Set(result.FromJust().ToObject(env)); diff --git a/src/node_errors.h b/src/node_errors.h index 7638ff4251..7b516fb140 100644 --- a/src/node_errors.h +++ b/src/node_errors.h @@ -63,8 +63,8 @@ void FatalException(const v8::FunctionCallbackInfo& args); 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);