Skip to content

Commit fe4675b

Browse files
guybedfordtargos
guybedford
authored and
targos
committed
module: fix main resolution and not found updates
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 nodejs#15736. Fixes: nodejs#15732 PR-URL: nodejs#16147 Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
1 parent c4c6381 commit fe4675b

9 files changed

Lines changed: 104 additions & 33 deletions

‎lib/internal/loader/Loader.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ class Loader {
4747
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
4848
'parentURL', 'string');
4949
}
50+
5051
const { url, format } = await this.resolver(specifier, parentURL,
5152
ModuleRequest.resolve);
5253

‎lib/internal/loader/ModuleRequest.js‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,14 @@ exports.resolve = (specifier, parentURL) => {
8888
};
8989
}
9090

91-
let url = search(specifier, parentURL);
91+
let url;
92+
try {
93+
url = search(specifier, parentURL);
94+
} catch (e) {
95+
if (e.message && e.message.startsWith('Cannot find module'))
96+
e.code = 'MODULE_NOT_FOUND';
97+
throw e;
98+
}
9299

93100
if (url.protocol !== 'file:') {
94101
throw new errors.Error('ERR_INVALID_PROTOCOL',

‎lib/module.js‎

Lines changed: 19 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -424,39 +424,28 @@ Module._load = function(request, parent, isMain) {
424424
debug('Module._load REQUEST %s parent: %s', request, parent.id);
425425
}
426426

427-
var filename = null;
428-
429-
if (isMain) {
430-
try {
431-
filename = Module._resolveFilename(request, parent, isMain);
432-
} catch (e) {
433-
// try to keep stack
434-
e.stack;
435-
throw e;
436-
}
437-
if (experimentalModules) {
438-
(async () => {
439-
// loader setup
440-
if (!ESMLoader) {
441-
ESMLoader = new Loader();
442-
const userLoader = process.binding('config').userLoader;
443-
if (userLoader) {
444-
const hooks = await new Loader().import(userLoader);
445-
ESMLoader.hook(hooks);
446-
}
427+
if (isMain && experimentalModules) {
428+
(async () => {
429+
// loader setup
430+
if (!ESMLoader) {
431+
ESMLoader = new Loader();
432+
const userLoader = process.binding('config').userLoader;
433+
if (userLoader) {
434+
const hooks = await new Loader().import(userLoader);
435+
ESMLoader.hook(hooks);
447436
}
448-
await ESMLoader.import(getURLFromFilePath(filename).href);
449-
})()
450-
.catch((e) => {
451-
console.error(e);
452-
process.exit(1);
453-
});
454-
return;
455-
}
456-
} else {
457-
filename = Module._resolveFilename(request, parent, isMain);
437+
}
438+
await ESMLoader.import(getURLFromFilePath(request).href);
439+
})()
440+
.catch((e) => {
441+
console.error(e);
442+
process.exit(1);
443+
});
444+
return;
458445
}
459446

447+
var filename = Module._resolveFilename(request, parent, isMain);
448+
460449
var cachedModule = Module._cache[filename];
461450
if (cachedModule) {
462451
updateChildren(parent, cachedModule, true);

‎src/module_wrap.cc‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -442,6 +442,11 @@ URL resolve_directory(const URL& search, bool read_pkg_json) {
442442
URL Resolve(std::string specifier, const URL* base, bool read_pkg_json) {
443443
URL pure_url(specifier);
444444
if (!(pure_url.flags() & URL_FLAGS_FAILED)) {
445+
// just check existence, without altering
446+
auto check = check_file(pure_url, true);
447+
if (check.failed) {
448+
return URL("");
449+
}
445450
return pure_url;
446451
}
447452
if (specifier.length() == 0) {
@@ -493,9 +498,8 @@ void ModuleWrap::Resolve(const FunctionCallbackInfo<Value>& args) {
493498

494499
URL result = node::loader::Resolve(*specifier_utf, &url, true);
495500
if (result.flags() & URL_FLAGS_FAILED) {
496-
std::string msg = "module ";
501+
std::string msg = "Cannot find module ";
497502
msg += *specifier_utf;
498-
msg += " not found";
499503
env->ThrowError(msg.c_str());
500504
return;
501505
}

‎test/es-module/test-esm-preserve-symlinks-not-found-plain.mjs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
// Flags: --experimental-modules --loader ./test/fixtures/es-module-loaders/not-found-assert-loader.mjs
2+
/* eslint-disable required-modules */
3+
import './not-found.js';

‎test/es-module/test-esm-preserve-symlinks-not-found.mjs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
// Flags: --experimental-modules --loader ./test/fixtures/es-module-loaders/not-found-assert-loader.mjs
2+
/* eslint-disable required-modules */
3+
import './not-found';

‎test/fixtures/es-module-loaders/not-found-assert-loader.mjs‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
import assert from 'assert';
2+
3+
// a loader that asserts that the defaultResolve will throw "not found"
4+
// (skipping the top-level main of course)
5+
let mainLoad = true;
6+
export async function resolve (specifier, base, defaultResolve) {
7+
if (mainLoad) {
8+
mainLoad = false;
9+
return defaultResolve(specifier, base);
10+
}
11+
try {
12+
await defaultResolve(specifier, base);
13+
}
14+
catch (e) {
15+
assert.equal(e.code, 'MODULE_NOT_FOUND');
16+
return {
17+
format: 'builtin',
18+
url: 'fs'
19+
};
20+
}
21+
assert.fail(`Module resolution for ${specifier} should be throw MODULE_NOT_FOUND`);
22+
}

‎test/parallel/test-module-main-fail.js‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
'use strict';
2+
require('../common');
3+
const assert = require('assert');
4+
const { execFileSync } = require('child_process');
5+
6+
const entryPoints = ['iDoNotExist', 'iDoNotExist.js', 'iDoNotExist.mjs'];
7+
const flags = [[], ['--experimental-modules']];
8+
const node = process.argv[0];
9+
10+
for (const args of flags) {
11+
for (const entryPoint of entryPoints) {
12+
try {
13+
execFileSync(node, args.concat(entryPoint), { stdio: 'pipe' });
14+
} catch (e) {
15+
assert(e.toString().match(/Error: Cannot find module/));
16+
continue;
17+
}
18+
assert.fail('Executing node with inexistent entry point should ' +
19+
`fail. Entry point: ${entryPoint}, Flags: [${args}]`);
20+
}
21+
}

‎test/parallel/test-module-main-preserve-symlinks-fail.js‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
'use strict';
2+
require('../common');
3+
const assert = require('assert');
4+
const { execFileSync } = require('child_process');
5+
6+
const entryPoints = ['iDoNotExist', 'iDoNotExist.js', 'iDoNotExist.mjs'];
7+
const flags = [[], ['--experimental-modules', '--preserve-symlinks']];
8+
const node = process.argv[0];
9+
10+
for (const args of flags) {
11+
for (const entryPoint of entryPoints) {
12+
try {
13+
execFileSync(node, args.concat(entryPoint));
14+
} catch (e) {
15+
assert(e.toString().match(/Error: Cannot find module/));
16+
continue;
17+
}
18+
assert.fail('Executing node with inexistent entry point should ' +
19+
`fail. Entry point: ${entryPoint}, Flags: [${args}]`);
20+
}
21+
}

0 commit comments

Comments
 (0)