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: #31906 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Myles Borins <[email protected]>
nodejs · May 4, 2020 · 2174159 · 2174159
1 parent a4ec01c
commit 2174159
Show file tree

Hide file tree

Showing 9 changed files with 163 additions and 6 deletions.
diff --git 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
@@ -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
diff --git a/test/fixtures/node_modules/some_module/obj.js b/test/fixtures/node_modules/some_module/obj.js
diff --git a/test/fixtures/node_modules/some_module/package.json b/test/fixtures/node_modules/some_module/package.json
diff --git a/test/message/esm_loader_not_found_cjs_hint_bare.js 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
@@ -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.<anonymous> (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
@@ -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
@@ -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'
+}