From fe4675b301f0e3e26dcb88ef9f68fe35403330a0 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Wed, 11 Oct 2017 15:45:21 +0200 Subject: [PATCH] module: fix main resolution and not found updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This simplifies the top-level load when ES modules are enabled as we can entirely delegate the module resolver, which will hand over to CommonJS where appropriate. All not found errors are made consistent to throw during resolve and have the MODULE_NOT_FOUND code. Includes the test case from https://github.com/nodejs/node/pull/15736. Fixes: https://github.com/nodejs/node/issues/15732 PR-URL: https://github.com/nodejs/node/pull/16147 Reviewed-By: Franziska Hinkelmann Reviewed-By: James M Snell Reviewed-By: Michaël Zasso Reviewed-By: Colin Ihrig --- lib/internal/loader/Loader.js | 1 + lib/internal/loader/ModuleRequest.js | 9 +++- lib/module.js | 49 +++++++------------ src/module_wrap.cc | 8 ++- ...-esm-preserve-symlinks-not-found-plain.mjs | 3 ++ .../test-esm-preserve-symlinks-not-found.mjs | 3 ++ .../not-found-assert-loader.mjs | 22 +++++++++ test/parallel/test-module-main-fail.js | 21 ++++++++ ...test-module-main-preserve-symlinks-fail.js | 21 ++++++++ 9 files changed, 104 insertions(+), 33 deletions(-) create mode 100644 test/es-module/test-esm-preserve-symlinks-not-found-plain.mjs create mode 100644 test/es-module/test-esm-preserve-symlinks-not-found.mjs create mode 100644 test/fixtures/es-module-loaders/not-found-assert-loader.mjs create mode 100644 test/parallel/test-module-main-fail.js create mode 100644 test/parallel/test-module-main-preserve-symlinks-fail.js diff --git a/lib/internal/loader/Loader.js b/lib/internal/loader/Loader.js index 57c70188d66..b1b99814f4f 100644 --- a/lib/internal/loader/Loader.js +++ b/lib/internal/loader/Loader.js @@ -47,6 +47,7 @@ class Loader { throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'parentURL', 'string'); } + const { url, format } = await this.resolver(specifier, parentURL, ModuleRequest.resolve); diff --git a/lib/internal/loader/ModuleRequest.js b/lib/internal/loader/ModuleRequest.js index 88e48ae9d3e..72f3dd3ee57 100644 --- a/lib/internal/loader/ModuleRequest.js +++ b/lib/internal/loader/ModuleRequest.js @@ -88,7 +88,14 @@ exports.resolve = (specifier, parentURL) => { }; } - let url = search(specifier, parentURL); + let url; + try { + url = search(specifier, parentURL); + } catch (e) { + if (e.message && e.message.startsWith('Cannot find module')) + e.code = 'MODULE_NOT_FOUND'; + throw e; + } if (url.protocol !== 'file:') { throw new errors.Error('ERR_INVALID_PROTOCOL', diff --git a/lib/module.js b/lib/module.js index 92b8fbbc392..d210c916252 100644 --- a/lib/module.js +++ b/lib/module.js @@ -424,39 +424,28 @@ Module._load = function(request, parent, isMain) { debug('Module._load REQUEST %s parent: %s', request, parent.id); } - var filename = null; - - if (isMain) { - try { - filename = Module._resolveFilename(request, parent, isMain); - } catch (e) { - // try to keep stack - e.stack; - throw e; - } - if (experimentalModules) { - (async () => { - // loader setup - if (!ESMLoader) { - ESMLoader = new Loader(); - const userLoader = process.binding('config').userLoader; - if (userLoader) { - const hooks = await new Loader().import(userLoader); - ESMLoader.hook(hooks); - } + if (isMain && experimentalModules) { + (async () => { + // loader setup + if (!ESMLoader) { + ESMLoader = new Loader(); + const userLoader = process.binding('config').userLoader; + if (userLoader) { + const hooks = await new Loader().import(userLoader); + ESMLoader.hook(hooks); } - await ESMLoader.import(getURLFromFilePath(filename).href); - })() - .catch((e) => { - console.error(e); - process.exit(1); - }); - return; - } - } else { - filename = Module._resolveFilename(request, parent, isMain); + } + await ESMLoader.import(getURLFromFilePath(request).href); + })() + .catch((e) => { + console.error(e); + process.exit(1); + }); + return; } + var filename = Module._resolveFilename(request, parent, isMain); + var cachedModule = Module._cache[filename]; if (cachedModule) { updateChildren(parent, cachedModule, true); diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 829248b681c..bda760369e9 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -442,6 +442,11 @@ URL resolve_directory(const URL& search, bool read_pkg_json) { URL Resolve(std::string specifier, const URL* base, bool read_pkg_json) { URL pure_url(specifier); if (!(pure_url.flags() & URL_FLAGS_FAILED)) { + // just check existence, without altering + auto check = check_file(pure_url, true); + if (check.failed) { + return URL(""); + } return pure_url; } if (specifier.length() == 0) { @@ -493,9 +498,8 @@ void ModuleWrap::Resolve(const FunctionCallbackInfo& args) { URL result = node::loader::Resolve(*specifier_utf, &url, true); if (result.flags() & URL_FLAGS_FAILED) { - std::string msg = "module "; + std::string msg = "Cannot find module "; msg += *specifier_utf; - msg += " not found"; env->ThrowError(msg.c_str()); return; } diff --git a/test/es-module/test-esm-preserve-symlinks-not-found-plain.mjs b/test/es-module/test-esm-preserve-symlinks-not-found-plain.mjs new file mode 100644 index 00000000000..bfeb71ef3a6 --- /dev/null +++ b/test/es-module/test-esm-preserve-symlinks-not-found-plain.mjs @@ -0,0 +1,3 @@ +// Flags: --experimental-modules --loader ./test/fixtures/es-module-loaders/not-found-assert-loader.mjs +/* eslint-disable required-modules */ +import './not-found.js'; diff --git a/test/es-module/test-esm-preserve-symlinks-not-found.mjs b/test/es-module/test-esm-preserve-symlinks-not-found.mjs new file mode 100644 index 00000000000..22c888028e7 --- /dev/null +++ b/test/es-module/test-esm-preserve-symlinks-not-found.mjs @@ -0,0 +1,3 @@ +// Flags: --experimental-modules --loader ./test/fixtures/es-module-loaders/not-found-assert-loader.mjs +/* eslint-disable required-modules */ +import './not-found'; diff --git a/test/fixtures/es-module-loaders/not-found-assert-loader.mjs b/test/fixtures/es-module-loaders/not-found-assert-loader.mjs new file mode 100644 index 00000000000..7718cc7c4ba --- /dev/null +++ b/test/fixtures/es-module-loaders/not-found-assert-loader.mjs @@ -0,0 +1,22 @@ +import assert from 'assert'; + +// a loader that asserts that the defaultResolve will throw "not found" +// (skipping the top-level main of course) +let mainLoad = true; +export async function resolve (specifier, base, defaultResolve) { + if (mainLoad) { + mainLoad = false; + return defaultResolve(specifier, base); + } + try { + await defaultResolve(specifier, base); + } + catch (e) { + assert.equal(e.code, 'MODULE_NOT_FOUND'); + return { + format: 'builtin', + url: 'fs' + }; + } + assert.fail(`Module resolution for ${specifier} should be throw MODULE_NOT_FOUND`); +} diff --git a/test/parallel/test-module-main-fail.js b/test/parallel/test-module-main-fail.js new file mode 100644 index 00000000000..a6457f33b65 --- /dev/null +++ b/test/parallel/test-module-main-fail.js @@ -0,0 +1,21 @@ +'use strict'; +require('../common'); +const assert = require('assert'); +const { execFileSync } = require('child_process'); + +const entryPoints = ['iDoNotExist', 'iDoNotExist.js', 'iDoNotExist.mjs']; +const flags = [[], ['--experimental-modules']]; +const node = process.argv[0]; + +for (const args of flags) { + for (const entryPoint of entryPoints) { + try { + execFileSync(node, args.concat(entryPoint), { stdio: 'pipe' }); + } catch (e) { + assert(e.toString().match(/Error: Cannot find module/)); + continue; + } + assert.fail('Executing node with inexistent entry point should ' + + `fail. Entry point: ${entryPoint}, Flags: [${args}]`); + } +} diff --git a/test/parallel/test-module-main-preserve-symlinks-fail.js b/test/parallel/test-module-main-preserve-symlinks-fail.js new file mode 100644 index 00000000000..b46497b6252 --- /dev/null +++ b/test/parallel/test-module-main-preserve-symlinks-fail.js @@ -0,0 +1,21 @@ +'use strict'; +require('../common'); +const assert = require('assert'); +const { execFileSync } = require('child_process'); + +const entryPoints = ['iDoNotExist', 'iDoNotExist.js', 'iDoNotExist.mjs']; +const flags = [[], ['--experimental-modules', '--preserve-symlinks']]; +const node = process.argv[0]; + +for (const args of flags) { + for (const entryPoint of entryPoints) { + try { + execFileSync(node, args.concat(entryPoint)); + } catch (e) { + assert(e.toString().match(/Error: Cannot find module/)); + continue; + } + assert.fail('Executing node with inexistent entry point should ' + + `fail. Entry point: ${entryPoint}, Flags: [${args}]`); + } +}