From 9a0a78fc7a7ff3304a21261804a0a2b835ba7c06 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Wed, 24 Oct 2018 16:50:32 +0100 Subject: [PATCH 1/3] esm: minimal resolver spec and implementation refinements --- doc/api/errors.md | 13 +- doc/api/esm.md | 49 +++++- 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, 117 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 500fb7801b..4706e52b40 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..21865ca7d5 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,51 @@ 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 _"node:${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 +> _"${parentURL}/node_modules/${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 5fec67d6c7..35b091fda2 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', @@ -894,11 +896,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 889597fbd1..8f0d7eba58 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -466,104 +466,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(); } @@ -572,26 +505,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) { @@ -613,10 +547,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 2c52007c51..456466efcd 100644 --- a/src/node_errors.h +++ b/src/node_errors.h @@ -65,8 +65,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); From cc73c3695781c264b28e2a417b6673cb35f23749 Mon Sep 17 00:00:00 2001 From: guybedford Date: Wed, 21 Nov 2018 23:20:52 +0200 Subject: [PATCH 2/3] newline lint fix --- doc/api/esm.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/api/esm.md b/doc/api/esm.md index 21865ca7d5..63f443d5dc 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -166,7 +166,8 @@ _ESM_RESOLVE_: > 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. 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 From 2c72b1994d9019afdd34556f1755bde7914ba3df Mon Sep 17 00:00:00 2001 From: guybedford Date: Thu, 22 Nov 2018 00:12:17 +0200 Subject: [PATCH 3/3] explicit string concatenation --- doc/api/esm.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/api/esm.md b/doc/api/esm.md index 63f443d5dc..d2277044a9 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -179,11 +179,11 @@ _ESM_RESOLVE_: **PACKAGE_RESOLVE**(_packageSpecifier_, _parentURL_) > 1. Assert: _packageSpecifier_ is a bare specifier. > 1. If _packageSpecifier_ is a Node.js builtin module then, -> 1. Return _"node:${packageSpecifier}"_. +> 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 -> _"${parentURL}/node_modules/${packageSpecifier}"_. +> 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.