esm: fix hint on invalid module specifier

PR-URL: #51223Fixes: #51216 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: James M Snell <[email protected]>

Author: aduh95

lib/internal/modules/esm/resolve.js

@@ -3,11 +3,10 @@
 const{
  ArrayIsArray,
  ArrayPrototypeJoin,
-ArrayPrototypeShift,
+ArrayPrototypeMap,
  JSONStringify,
  ObjectGetOwnPropertyNames,
  ObjectPrototypeHasOwnProperty,
- RegExp,
  RegExpPrototypeExec,
  RegExpPrototypeSymbolReplace,
  SafeMap,
@@ -21,6 +20,7 @@ const{
  StringPrototypeSlice,
  StringPrototypeSplit,
  StringPrototypeStartsWith,
+ encodeURIComponent,
 }=primordials;
 constinternalFS=require('internal/fs/utils');
 const{ BuiltinModule }=require('internal/bootstrap/realm');
@@ -30,7 +30,7 @@ const{getOptionValue } = require('internal/options');
 constpolicy=getOptionValue('--experimental-policy') ?
 require('internal/process/policy') :
 null;
-const{ sep, relative, toNamespacedPath, resolve }=require('path');
+const{ sep,posix: {relative: relativePosixPath}, toNamespacedPath, resolve }=require('path');
 constpreserveSymlinks=getOptionValue('--preserve-symlinks');
 constpreserveSymlinksMain=getOptionValue('--preserve-symlinks-main');
 constexperimentalNetworkImports=
@@ -912,6 +912,7 @@ function moduleResolve(specifier, base, conditions, preserveSymlinks){
  * Try to resolve an import as a CommonJS module.
  * @param{string} specifier - The specifier to resolve.
  * @param{string} parentURL - The base URL.
+ * @returns{string | Buffer | false}
  */
 functionresolveAsCommonJS(specifier,parentURL){
 try{
@@ -924,29 +925,38 @@ function resolveAsCommonJS(specifier, parentURL){
 // 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'
+constfoundURL=pathToFileURL(found).pathname;
+found=relativePosixPath(
+StringPrototypeSlice(parentURL,'file://'.length,StringPrototypeLastIndexOf(parentURL,'/')),
+foundURL);
+
+// Add './' if the path does not start with '../'
 // 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}`;
+if(!StringPrototypeStartsWith(found,'../')){
+found=`./${found}`;
 }
 }elseif(isBareSpecifier(specifier)){
 // If it is a bare specifier return the relative path within the
 // module
-constpkg=StringPrototypeSplit(specifier,'/')[0];
-constindex=StringPrototypeIndexOf(found,pkg);
+consti=StringPrototypeIndexOf(specifier,'/');
+constpkg=i===-1 ? specifier : StringPrototypeSlice(specifier,0,i);
+constneedle=`${sep}node_modules${sep}${pkg}${sep}`;
+constindex=StringPrototypeLastIndexOf(found,needle);
 if(index!==-1){
-found=StringPrototypeSlice(found,index);
+found=pkg+'/'+ArrayPrototypeJoin(
+ArrayPrototypeMap(
+StringPrototypeSplit(StringPrototypeSlice(found,index+needle.length),sep),
+// Escape URL-special characters to avoid generating a incorrect suggestion
+encodeURIComponent,
+),
+'/',
+);
+}else{
+found=`${pathToFileURL(found)}`;
 }
 }
-// Normalize the path separator to give a valid suggestion
-// on Windows
-if(process.platform==='win32'){
-found=RegExpPrototypeSymbolReplace(newRegExp(`\\${sep}`,'g'),
-found,'/');
-}
 returnfound;
 }catch{
 returnfalse;
@@ -1154,14 +1164,14 @@ function defaultResolve(specifier, context ={}){
  */
 functiondecorateErrorWithCommonJSHints(error,specifier,parentURL){
 constfound=resolveAsCommonJS(specifier,parentURL);
-if(found){
+if(found&&found!==specifier){// Don't suggest the same input the user provided.
 // Modify the stack and message string to include the hint
-constlines=StringPrototypeSplit(error.stack,'\n');
-consthint=`Did you mean to import ${found}?`;
+constendOfFirstLine=StringPrototypeIndexOf(error.stack,'\n');
+consthint=`Did you mean to import ${JSONStringify(found)}?`;
 error.stack=
-ArrayPrototypeShift(lines)+'\n'+
-hint+'\n'+
-ArrayPrototypeJoin(lines,'\n');
+StringPrototypeSlice(error.stack,0,endOfFirstLine)+'\n'+
+hint+
+StringPrototypeSlice(error.stack,endOfFirstLine);
 error.message+=`\n${hint}`;
 }
 }

test/es-module/test-esm-module-not-found-commonjs-hint.mjs

@@ -1,32 +1,53 @@
 import{spawnPromisified}from'../common/index.mjs';
-import{fixturesDir}from'../common/fixtures.mjs';
+import{fixturesDir,fileURLasfixtureSubDir}from'../common/fixtures.mjs';
 import{match,notStrictEqual}from'node:assert';
 import{execPath}from'node:process';
 import{describe,it}from'node:test';
 
 
 describe('ESM: module not found hint',{concurrency: true},()=>{
 for(
-const{ input, expected }
+const{ input, expected, cwd =fixturesDir}
 of[
 {
 input: 'import "./print-error-message"',
-// Did you mean to import ../print-error-message.js?
-expected: /\.\.\/print-error-message\.js\?/,
+// Did you mean to import "./print-error-message.js"?
+expected: /"\.\/print-error-message\.js"\?/,
+},
+{
+input: 'import "./es-modules/folder%25with percentage#/index.js"',
+// Did you mean to import "./es-modules/folder%2525with%20percentage%23/index.js"?
+expected: /"\.\/es-modules\/folder%2525with%20percentage%23\/index\.js"\?/,
+},
+{
+input: 'import "../folder%25with percentage#/index.js"',
+// Did you mean to import "../es-modules/folder%2525with%20percentage%23/index.js"?
+expected: /"\.\.\/folder%2525with%20percentage%23\/index\.js"\?/,
+cwd: fixtureSubDir('es-modules/tla/'),
 },
 {
 input: 'import obj from "some_module/obj"',
-expected: /some_module\/obj\.js\?/,
+expected: /"some_module\/obj\.js"\?/,
+},
+{
+input: 'import obj from "some_module/folder%25with percentage#/index.js"',
+expected: /"some_module\/folder%2525with%20percentage%23\/index\.js"\?/,
+},
+{
+input: 'import "@nodejsscope/pkg/index"',
+expected: /"@nodejsscope\/pkg\/index\.js"\?/,
+},
+{
+input: 'import obj from "lone_file.js"',
+expected: /node_modules\/lone_file\.js"\?/,
 },
 ]
 )it('should cite a variant form',async()=>{
 const{ code, stderr }=awaitspawnPromisified(execPath,[
 '--input-type=module',
 '--eval',
 input,
-],{
-cwd: fixturesDir,
-});
+],{ cwd });
 
 match(stderr,expected);
 notStrictEqual(code,0);

test/es-module/test-esm-type-flag-cli-entry.mjs

@@ -26,7 +26,7 @@ describe('--experimental-default-type=module should not support extension search
 cwd: fixtures.path('es-modules/package-without-type'),
 });
 
-match(stderr,/ENOENT.*Didyoumeantoimport.*index\.js\?/s);
+match(stderr,/ENOENT.*Didyoumeantoimport.*index\.js"\?/s);
 strictEqual(stdout,'');
 strictEqual(code,1);
 strictEqual(signal,null);

test/fixtures/es-modules/folder%25with percentage#/index.js

@@ -0,0 +1 @@
+'use strict';