From 2174159598e39cb031c139ccbccbf7ad9bb3c044 Mon Sep 17 00:00:00 2001 From: Daniele Belardi Date: Sat, 21 Mar 2020 16:38:45 +0100 Subject: [PATCH] esm: improve commonjs hint on module not found Run CommonJS resolver only if `error.code` is ERR_MODULE_NOT_FOUND. Avoid using absolute paths in hint by: * using a parent-relative path if the specifier is a relative path * using a `pkg/x.js` format if the specifier is bare (e.g. `pkg/x`) PR-URL: https://github.com/nodejs/node/pull/31906 Reviewed-By: Guy Bedford Reviewed-By: Myles Borins --- lib/internal/modules/esm/resolve.js | 90 +++++++++++++++++-- .../esm_loader_not_found_cjs_hint_bare.mjs | 5 ++ .../node_modules/some_module/index.js | 3 + test/fixtures/node_modules/some_module/obj.js | 3 + .../node_modules/some_module/package.json | 12 +++ .../esm_loader_not_found_cjs_hint_bare.js | 17 ++++ .../esm_loader_not_found_cjs_hint_bare.out | 16 ++++ ...esm_loader_not_found_cjs_hint_relative.mjs | 3 + ...esm_loader_not_found_cjs_hint_relative.out | 20 +++++ 9 files changed, 163 insertions(+), 6 deletions(-) create mode 100644 test/fixtures/esm_loader_not_found_cjs_hint_bare.mjs create mode 100644 test/fixtures/node_modules/some_module/index.js create mode 100644 test/fixtures/node_modules/some_module/obj.js create mode 100644 test/fixtures/node_modules/some_module/package.json create mode 100644 test/message/esm_loader_not_found_cjs_hint_bare.js create mode 100644 test/message/esm_loader_not_found_cjs_hint_bare.out create mode 100644 test/message/esm_loader_not_found_cjs_hint_relative.mjs create mode 100644 test/message/esm_loader_not_found_cjs_hint_relative.out diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 8b8582544494a5..c66e0554ad2746 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -2,17 +2,22 @@ const { ArrayIsArray, + ArrayPrototypeJoin, + ArrayPrototypeShift, JSONParse, JSONStringify, ObjectFreeze, ObjectGetOwnPropertyNames, ObjectPrototypeHasOwnProperty, + RegExp, SafeMap, SafeSet, StringPrototypeEndsWith, StringPrototypeIncludes, StringPrototypeIndexOf, + StringPrototypeReplace, StringPrototypeSlice, + StringPrototypeSplit, StringPrototypeStartsWith, StringPrototypeSubstr, } = primordials; @@ -29,8 +34,8 @@ const { Stats, } = require('fs'); const { getOptionValue } = require('internal/options'); -const { sep } = require('path'); - +const { sep, relative } = require('path'); +const { Module: CJSModule } = require('internal/modules/cjs/loader'); const preserveSymlinks = getOptionValue('--preserve-symlinks'); const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main'); const typeFlag = getOptionValue('--input-type'); @@ -611,9 +616,11 @@ function packageResolve(specifier, base, conditions) { throw new ERR_MODULE_NOT_FOUND(packageName, fileURLToPath(base)); } -function shouldBeTreatedAsRelativeOrAbsolutePath(specifier) { - if (specifier === '') return false; - if (specifier[0] === '/') return true; +function isBareSpecifier(specifier) { + return specifier[0] && specifier[0] !== '/' && specifier[0] !== '.'; +} + +function isRelativeSpecifier(specifier) { if (specifier[0] === '.') { if (specifier.length === 1 || specifier[1] === '/') return true; if (specifier[1] === '.') { @@ -623,6 +630,12 @@ function shouldBeTreatedAsRelativeOrAbsolutePath(specifier) { return false; } +function shouldBeTreatedAsRelativeOrAbsolutePath(specifier) { + if (specifier === '') return false; + if (specifier[0] === '/') return true; + return isRelativeSpecifier(specifier); +} + /** * @param {string} specifier * @param {URL} base @@ -645,6 +658,51 @@ function moduleResolve(specifier, base, conditions) { return finalizeResolution(resolved, base); } +/** + * Try to resolve an import as a CommonJS module + * @param {string} specifier + * @param {string} parentURL + * @returns {boolean|string} + */ +function resolveAsCommonJS(specifier, parentURL) { + try { + const parent = fileURLToPath(parentURL); + const tmpModule = new CJSModule(parent, null); + tmpModule.paths = CJSModule._nodeModulePaths(parent); + + let found = CJSModule._resolveFilename(specifier, tmpModule, false); + + // If it is a relative specifier return the relative path + // to the parent + if (isRelativeSpecifier(specifier)) { + found = relative(parent, found); + // Add '.separator if the path does not start with '..separator' + // This should be a safe assumption because when loading + // esm modules there should be always a file specified so + // there should not be a specifier like '..' or '.' + if (!StringPrototypeStartsWith(found, `..${sep}`)) { + found = `.${sep}${found}`; + } + } else if (isBareSpecifier(specifier)) { + // If it is a bare specifier return the relative path within the + // module + const pkg = StringPrototypeSplit(specifier, '/')[0]; + const index = StringPrototypeIndexOf(found, pkg); + if (index !== -1) { + found = StringPrototypeSlice(found, index); + } + } + // Normalize the path separator to give a valid suggestion + // on Windows + if (process.platform === 'win32') { + found = StringPrototypeReplace(found, new RegExp(`\\${sep}`, 'g'), '/'); + } + return found; + } catch { + return false; + } +} + function defaultResolve(specifier, context = {}, defaultResolveUnused) { let { parentURL, conditions } = context; let parsed; @@ -685,7 +743,27 @@ function defaultResolve(specifier, context = {}, defaultResolveUnused) { } conditions = getConditionsSet(conditions); - let url = moduleResolve(specifier, parentURL, conditions); + let url; + try { + url = moduleResolve(specifier, parentURL, conditions); + } catch (error) { + // Try to give the user a hint of what would have been the + // resolved CommonJS module + if (error.code === 'ERR_MODULE_NOT_FOUND') { + const found = resolveAsCommonJS(specifier, parentURL); + if (found) { + // Modify the stack and message string to include the hint + const lines = StringPrototypeSplit(error.stack, '\n'); + const hint = `Did you mean to import ${found}?`; + error.stack = + ArrayPrototypeShift(lines) + '\n' + + hint + '\n' + + ArrayPrototypeJoin(lines, '\n'); + error.message += `\n${hint}`; + } + } + throw error; + } if (isMain ? !preserveSymlinksMain : !preserveSymlinks) { const urlPath = fileURLToPath(url); diff --git a/test/fixtures/esm_loader_not_found_cjs_hint_bare.mjs b/test/fixtures/esm_loader_not_found_cjs_hint_bare.mjs new file mode 100644 index 00000000000000..4eb5f190af43e4 --- /dev/null +++ b/test/fixtures/esm_loader_not_found_cjs_hint_bare.mjs @@ -0,0 +1,5 @@ +'use strict'; + +import obj from 'some_module/obj'; + +throw new Error('Should have errored'); diff --git a/test/fixtures/node_modules/some_module/index.js b/test/fixtures/node_modules/some_module/index.js new file mode 100644 index 00000000000000..a2d739266cdca4 --- /dev/null +++ b/test/fixtures/node_modules/some_module/index.js @@ -0,0 +1,3 @@ +'use strict' + +module.exports = { main: true } diff --git a/test/fixtures/node_modules/some_module/obj.js b/test/fixtures/node_modules/some_module/obj.js new file mode 100644 index 00000000000000..249478e5ca506f --- /dev/null +++ b/test/fixtures/node_modules/some_module/obj.js @@ -0,0 +1,3 @@ +'use strict'; + +module.exports = { obj: 'module' } diff --git a/test/fixtures/node_modules/some_module/package.json b/test/fixtures/node_modules/some_module/package.json new file mode 100644 index 00000000000000..95a52cc61071a5 --- /dev/null +++ b/test/fixtures/node_modules/some_module/package.json @@ -0,0 +1,12 @@ +{ + "name": "some_module", + "version": "1.0.0", + "description": "", + "main": "index.js", + "scripts": { + "test": "echo \"Error: no test specified\" && exit 1" + }, + "keywords": [], + "author": "", + "license": "ISC" +} diff --git a/test/message/esm_loader_not_found_cjs_hint_bare.js b/test/message/esm_loader_not_found_cjs_hint_bare.js new file mode 100644 index 00000000000000..b049fd203fef06 --- /dev/null +++ b/test/message/esm_loader_not_found_cjs_hint_bare.js @@ -0,0 +1,17 @@ +'use strict'; + +require('../common'); +const { spawn } = require('child_process'); +const { join } = require('path'); +const { fixturesDir } = require('../common/fixtures'); + +spawn( + process.execPath, + [ + join(fixturesDir, 'esm_loader_not_found_cjs_hint_bare.mjs') + ], + { + cwd: fixturesDir, + stdio: 'inherit' + } +); diff --git a/test/message/esm_loader_not_found_cjs_hint_bare.out b/test/message/esm_loader_not_found_cjs_hint_bare.out new file mode 100644 index 00000000000000..e56f1da0f6e76e --- /dev/null +++ b/test/message/esm_loader_not_found_cjs_hint_bare.out @@ -0,0 +1,16 @@ +internal/modules/run_main.js:* + internalBinding('errors').triggerUncaughtException( + ^ + +Error [ERR_MODULE_NOT_FOUND]: Cannot find module '*test*fixtures*node_modules*some_module*obj' imported from *test*fixtures*esm_loader_not_found_cjs_hint_bare.mjs +Did you mean to import some_module/obj.js? + at finalizeResolution (internal/modules/esm/resolve.js:*:*) + at packageResolve (internal/modules/esm/resolve.js:*:*) + at moduleResolve (internal/modules/esm/resolve.js:*:*) + at Loader.defaultResolve [as _resolve] (internal/modules/esm/resolve.js:*:*) + at Loader.resolve (internal/modules/esm/loader.js:*:*) + at Loader.getModuleJob (internal/modules/esm/loader.js:*:*) + at ModuleWrap. (internal/modules/esm/module_job.js:*:*) + at link (internal/modules/esm/module_job.js:*:*) { + code: 'ERR_MODULE_NOT_FOUND' +} diff --git a/test/message/esm_loader_not_found_cjs_hint_relative.mjs b/test/message/esm_loader_not_found_cjs_hint_relative.mjs new file mode 100644 index 00000000000000..928186318bb09a --- /dev/null +++ b/test/message/esm_loader_not_found_cjs_hint_relative.mjs @@ -0,0 +1,3 @@ +// Flags: --experimental-loader ./test/common/fixtures +import '../common/index.mjs'; +console.log('This should not be printed'); diff --git a/test/message/esm_loader_not_found_cjs_hint_relative.out b/test/message/esm_loader_not_found_cjs_hint_relative.out new file mode 100644 index 00000000000000..76df3163bb728c --- /dev/null +++ b/test/message/esm_loader_not_found_cjs_hint_relative.out @@ -0,0 +1,20 @@ +(node:*) ExperimentalWarning: --experimental-loader is an experimental feature. This feature could change at any time +(Use `node --trace-warnings ...` to show where the warning was created) +internal/modules/run_main.js:* + internalBinding('errors').triggerUncaughtException( + ^ + +Error [ERR_MODULE_NOT_FOUND]: Cannot find module '*test*common*fixtures' imported from * +Did you mean to import ./test/common/fixtures.js? + at finalizeResolution (internal/modules/esm/resolve.js:*:*) + at moduleResolve (internal/modules/esm/resolve.js:*:*) + at Loader.defaultResolve [as _resolve] (internal/modules/esm/resolve.js:*:*) + at Loader.resolve (internal/modules/esm/loader.js:*:*) + at Loader.getModuleJob (internal/modules/esm/loader.js:*:*) + at Loader.import (internal/modules/esm/loader.js:*:*) + at internal/process/esm_loader.js:*:* + at Object.initializeLoader (internal/process/esm_loader.js:*:*) + at runMainESM (internal/modules/run_main.js:*:*) + at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:*:*) { + code: 'ERR_MODULE_NOT_FOUND' +}