esm: refine ERR_REQUIRE_ESM errors

PR-URL: #39175
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

Author: guybedford

lib/internal/errors.js

@@ -14,6 +14,7 @@ const {
  AggregateError,
  ArrayFrom,
  ArrayIsArray,
+ ArrayPrototypeFilter,
  ArrayPrototypeIncludes,
  ArrayPrototypeIndexOf,
  ArrayPrototypeJoin,
@@ -788,6 +789,34 @@ const fatalExceptionStackEnhancers = {
 }
 };
 
+// Ensures the printed error line is from user code.
+let_kArrowMessagePrivateSymbol,_setHiddenValue;
+functionsetArrowMessage(err,arrowMessage){
+if(!_kArrowMessagePrivateSymbol){
+({
+arrow_message_private_symbol: _kArrowMessagePrivateSymbol,
+setHiddenValue: _setHiddenValue,
+}=internalBinding('util'));
+}
+_setHiddenValue(err,_kArrowMessagePrivateSymbol,arrowMessage);
+}
+
+// Hide stack lines before the first user code line.
+functionhideInternalStackFrames(error){
+overrideStackTrace.set(error,(error,stackFrames)=>{
+letframes=stackFrames;
+if(typeofstackFrames==='object'){
+frames=ArrayPrototypeFilter(
+stackFrames,
+(frm)=>!StringPrototypeStartsWith(frm.getFileName(),
+'node:internal')
+);
+}
+ArrayPrototypeUnshift(frames,error);
+returnArrayPrototypeJoin(frames,'\n at ');
+});
+}
+
 // Node uses an AbortError that isn't exactly the same as the DOMException
 // to make usage of the error in userland and readable-stream easier.
 // It is a regular error with `.code` and `.name`.
@@ -806,8 +835,10 @@ module.exports = {
  exceptionWithHostPort,
  getMessage,
  hideStackFrames,
+ hideInternalStackFrames,
  isErrorStackTraceLimitWritable,
  isStackOverflowError,
+ setArrowMessage,
  connResetException,
  uvErrmapGet,
  uvException,
@@ -842,6 +873,7 @@ module.exports = {
 // Note: Please try to keep these in alphabetical order
 //
 // Note: Node.js specific errors must begin with the prefix ERR_
+
 E('ERR_AMBIGUOUS_ARGUMENT','The "%s" argument is ambiguous. %s',TypeError);
 E('ERR_ARG_NOT_ITERABLE','%s must be iterable',TypeError);
 E('ERR_ASSERTION','%s',Error);
@@ -1406,23 +1438,32 @@ E('ERR_PERFORMANCE_INVALID_TIMESTAMP',
 '%d is not a valid timestamp',TypeError);
 E('ERR_PERFORMANCE_MEASURE_INVALID_OPTIONS','%s',TypeError);
 E('ERR_REQUIRE_ESM',
-(filename,parentPath=null,packageJsonPath=null)=>{
-letmsg=`Must use import to load ES Module: ${filename}`;
-if(parentPath&&packageJsonPath){
-constpath=require('path');
-constbasename=path.basename(filename)===path.basename(parentPath) ?
-filename : path.basename(filename);
-msg+=
-'\nrequire() of ES modules is not supported.\nrequire() of '+
-`${filename} from ${parentPath} `+
-'is an ES module file as it is a .js file whose nearest parent '+
-'package.json contains "type": "module" which defines all .js '+
-'files in that package scope as ES modules.\nInstead rename '+
-`${basename} to end in .cjs, change the requiring code to use `+
-'import(), or remove "type": "module" from '+
-`${packageJsonPath}.\n`;
+function(filename,hasEsmSyntax,parentPath=null,packageJsonPath=null){
+hideInternalStackFrames(this);
+letmsg=`require() of ES Module ${filename}${parentPath ? ` from ${
+parentPath}` : ''} not supported.`;
+if(!packageJsonPath){
+if(StringPrototypeEndsWith(filename,'.mjs'))
+msg+=`\nInstead change the require of ${filename} to a dynamic `+
+'import() which is available in all CommonJS modules.';
+returnmsg;
+}
+constpath=require('path');
+constbasename=path.basename(filename)===path.basename(parentPath) ?
+filename : path.basename(filename);
+if(hasEsmSyntax){
+msg+=`\nInstead change the require of ${basename} in ${parentPath} to`+
+' a dynamic import() which is available in all CommonJS modules.';
 returnmsg;
 }
+msg+=`\n${basename} is treated as an ES module file as it is a .js `+
+'file whose nearest parent package.json contains "type": "module" '+
+'which declares all .js files in that package scope as ES modules.'+
+`\nInstead rename ${basename} to end in .cjs, change the requiring `+
+'code to use dynamic import() which is available in all CommonJS '+
+'modules, or change "type": "module" to "type": "commonjs" in '+
+`${packageJsonPath} to treat all .js files as CommonJS (using .mjs for `+
+'all ES modules instead).\n';
 returnmsg;
 },Error);
 E('ERR_SCRIPT_EXECUTION_INTERRUPTED',

lib/internal/modules/cjs/helpers.js

@@ -3,6 +3,7 @@
 const{
  ArrayPrototypeForEach,
  ArrayPrototypeJoin,
+ ArrayPrototypeSome,
  ObjectDefineProperty,
  ObjectPrototypeHasOwnProperty,
  SafeMap,
@@ -184,9 +185,26 @@ function normalizeReferrerURL(referrer) {
 returnnewURL(referrer).href;
 }
 
+// For error messages only - used to check if ESM syntax is in use.
+functionhasEsmSyntax(code){
+constparser=require('internal/deps/acorn/acorn/dist/acorn').Parser;
+letroot;
+try{
+root=parser.parse(code,{sourceType: 'module',ecmaVersion: 'latest'});
+}catch{
+returnfalse;
+}
+
+returnArrayPrototypeSome(root.body,(stmt)=>
+stmt.type==='ExportNamedDeclaration'||
+stmt.type==='ImportDeclaration'||
+stmt.type==='ExportAllDeclaration');
+}
+
 module.exports={
  addBuiltinLibsToObject,
  cjsConditions,
+ hasEsmSyntax,
  loadNativeModule,
  makeRequireFunction,
  normalizeReferrerURL,

lib/internal/modules/cjs/loader.js

@@ -48,6 +48,7 @@ const {
  Proxy,
  ReflectApply,
  ReflectSet,
+ RegExpPrototypeExec,
  RegExpPrototypeTest,
  SafeMap,
  SafeWeakMap,
@@ -58,6 +59,7 @@ const {
  StringPrototypeLastIndexOf,
  StringPrototypeIndexOf,
  StringPrototypeMatch,
+ StringPrototypeRepeat,
  StringPrototypeSlice,
  StringPrototypeSplit,
  StringPrototypeStartsWith,
@@ -88,11 +90,12 @@ const { internalModuleStat } = internalBinding('fs');
 constpackageJsonReader=require('internal/modules/package_json_reader');
 const{ safeGetenv }=internalBinding('credentials');
 const{
+ cjsConditions,
+ hasEsmSyntax,
+ loadNativeModule,
  makeRequireFunction,
  normalizeReferrerURL,
  stripBOM,
- cjsConditions,
- loadNativeModule
 }=require('internal/modules/cjs/helpers');
 const{ getOptionValue }=require('internal/options');
 constpreserveSymlinks=getOptionValue('--preserve-symlinks');
@@ -107,11 +110,14 @@ const policy = getOptionValue('--experimental-policy') ?
 lethasLoadedAnyUserCJSModule=false;
 
 const{
-ERR_INVALID_ARG_VALUE,
-ERR_INVALID_MODULE_SPECIFIER,
-ERR_REQUIRE_ESM,
-ERR_UNKNOWN_BUILTIN_MODULE,
-}=require('internal/errors').codes;
+codes: {
+ERR_INVALID_ARG_VALUE,
+ERR_INVALID_MODULE_SPECIFIER,
+ERR_REQUIRE_ESM,
+ERR_UNKNOWN_BUILTIN_MODULE,
+},
+ setArrowMessage,
+}=require('internal/errors');
 const{ validateString }=require('internal/validators');
 constpendingDeprecation=getOptionValue('--pending-deprecation');
 
@@ -970,7 +976,7 @@ Module.prototype.load = function(filename) {
 constextension=findLongestRegisteredExtension(filename);
 // allow .mjs to be overridden
 if(StringPrototypeEndsWith(filename,'.mjs')&&!Module._extensions['.mjs'])
-thrownewERR_REQUIRE_ESM(filename);
+thrownewERR_REQUIRE_ESM(filename,true);
 
 Module._extensions[extension](this,filename);
 this.loaded=true;
@@ -1102,16 +1108,6 @@ Module.prototype._compile = function(content, filename) {
 
 // Native extension for .js
 Module._extensions['.js']=function(module,filename){
-if(StringPrototypeEndsWith(filename,'.js')){
-constpkg=readPackageScope(filename);
-// Function require shouldn't be used in ES modules.
-if(pkg?.data?.type==='module'){
-constparent=moduleParentCache.get(module);
-constparentPath=parent?.filename;
-constpackageJsonPath=path.resolve(pkg.path,'package.json');
-thrownewERR_REQUIRE_ESM(filename,parentPath,packageJsonPath);
-}
-}
 // If already analyzed the source, then it will be cached.
 constcached=cjsParseCache.get(module);
 letcontent;
@@ -1121,6 +1117,39 @@ Module._extensions['.js'] = function(module, filename) {
 }else{
 content=fs.readFileSync(filename,'utf8');
 }
+if(StringPrototypeEndsWith(filename,'.js')){
+constpkg=readPackageScope(filename);
+// Function require shouldn't be used in ES modules.
+if(pkg?.data?.type==='module'){
+constparent=moduleParentCache.get(module);
+constparentPath=parent?.filename;
+constpackageJsonPath=path.resolve(pkg.path,'package.json');
+constusesEsm=hasEsmSyntax(content);
+consterr=newERR_REQUIRE_ESM(filename,usesEsm,parentPath,
+packageJsonPath);
+// Attempt to reconstruct the parent require frame.
+if(Module._cache[parentPath]){
+letparentSource;
+try{
+parentSource=fs.readFileSync(parentPath,'utf8');
+}catch{}
+if(parentSource){
+consterrLine=StringPrototypeSplit(
+StringPrototypeSlice(err.stack,StringPrototypeIndexOf(
+err.stack,' at ')),'\n',1)[0];
+const{1: line,2: col}=
+RegExpPrototypeExec(/(\d+):(\d+)\)/,errLine)||[];
+if(line&&col){
+constsrcLine=StringPrototypeSplit(parentSource,'\n')[line-1];
+constframe=`${parentPath}:${line}\n${srcLine}\n${
+StringPrototypeRepeat(' ',col-1)}^\n`;
+setArrowMessage(err,frame);
+}
+}
+}
+throwerr;
+}
+}
 module._compile(content,filename);
 };
 

src/node_errors.cc

@@ -215,6 +215,12 @@ void AppendExceptionLine(Environment* env,
  Local<Object> err_obj;
 if (!er.IsEmpty() && er->IsObject()) {
  err_obj = er.As<Object>();
+// If arrow_message is already set, skip.
+auto maybe_value = err_obj->GetPrivate(env->context(),
+ env->arrow_message_private_symbol());
+ Local<Value> lvalue;
+if (!maybe_value.ToLocal(&lvalue) || lvalue->IsString())
+return;
  }
 
 bool added_exception_line = false;

test/es-module/test-cjs-esm-warn.js

@@ -6,36 +6,59 @@ const { spawn } = require('child_process');
 constassert=require('assert');
 constpath=require('path');
 
-constrequiring=path.resolve(fixtures.path('/es-modules/cjs-esm.js'));
-constrequired=path.resolve(
-fixtures.path('/es-modules/package-type-module/cjs.js')
-);
+constrequiringCjsAsEsm=path.resolve(fixtures.path('/es-modules/cjs-esm.js'));
+constrequiringEsm=path.resolve(fixtures.path('/es-modules/cjs-esm-esm.js'));
 constpjson=path.resolve(
 fixtures.path('/es-modules/package-type-module/package.json')
 );
 
-constbasename='cjs.js';
+{
+constrequired=path.resolve(
+fixtures.path('/es-modules/package-type-module/cjs.js')
+);
+constbasename='cjs.js';
+constchild=spawn(process.execPath,[requiringCjsAsEsm]);
+letstderr='';
+child.stderr.setEncoding('utf8');
+child.stderr.on('data',(data)=>{
+stderr+=data;
+});
+child.on('close',common.mustCall((code,signal)=>{
+assert.strictEqual(code,1);
+assert.strictEqual(signal,null);
+
+assert.ok(stderr.replaceAll('\r','').includes(
+`Error [ERR_REQUIRE_ESM]: require() of ES Module ${required} from ${
+requiringCjsAsEsm} not supported.\n`));
+assert.ok(stderr.replaceAll('\r','').includes(
+`Instead rename ${basename} to end in .cjs, change the requiring `+
+'code to use dynamic import() which is available in all CommonJS '+
+`modules, or change "type": "module" to "type": "commonjs" in ${pjson} to `+
+'treat all .js files as CommonJS (using .mjs for all ES modules '+
+'instead).\n'));
+}));
+}
 
-constchild=spawn(process.execPath,[requiring]);
-letstderr='';
-child.stderr.setEncoding('utf8');
-child.stderr.on('data',(data)=>{
-stderr+=data;
-});
-child.on('close',common.mustCall((code,signal)=>{
-assert.strictEqual(code,1);
-assert.strictEqual(signal,null);
+{
+constrequired=path.resolve(
+fixtures.path('/es-modules/package-type-module/esm.js')
+);
+constbasename='esm.js';
+constchild=spawn(process.execPath,[requiringEsm]);
+letstderr='';
+child.stderr.setEncoding('utf8');
+child.stderr.on('data',(data)=>{
+stderr+=data;
+});
+child.on('close',common.mustCall((code,signal)=>{
+assert.strictEqual(code,1);
+assert.strictEqual(signal,null);
 
-assert.ok(stderr.replace(/\r/g,'').includes(
-`Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: ${required}`+
-'\nrequire() of ES modules is not supported.\nrequire() of '+
-`${required} from ${requiring} `+
-'is an ES module file as it is a .js file whose nearest parent '+
-'package.json contains "type": "module" which defines all .js '+
-'files in that package scope as ES modules.\nInstead rename '+
-`${basename} to end in .cjs, change the requiring code to use `+
-'import(), or remove "type": "module" from '+
-`${pjson}.\n`));
-assert.ok(stderr.includes(
-'Error [ERR_REQUIRE_ESM]: Must use import to load ES Module'));
-}));
+assert.ok(stderr.replace(/\r/g,'').includes(
+`Error [ERR_REQUIRE_ESM]: require() of ES Module ${required} from ${
+requiringEsm} not supported.\n`));
+assert.ok(stderr.replace(/\r/g,'').includes(
+`Instead change the require of ${basename} in ${requiringEsm} to`+
+' a dynamic import() which is available in all CommonJS modules.\n'));
+}));
+}

test/es-module/test-esm-type-flag-errors.js

@@ -29,8 +29,8 @@ try {
 }catch(e){
 assert.strictEqual(e.name,'Error');
 assert.strictEqual(e.code,'ERR_REQUIRE_ESM');
-assert(e.toString().match(/MustuseimporttoloadESModule/g));
-assert(e.message.match(/MustuseimporttoloadESModule/g));
+assert(e.toString().match(/require\(\)ofESModule/g));
+assert(e.message.match(/require\(\)ofESModule/g));
 }
 
 functionexpect(opt='',inputFile,want,wantsError=false){

test/fixtures/es-modules/cjs-esm-esm.js

@@ -0,0 +1 @@
+require('./package-type-module/esm.js');

test/fixtures/es-modules/package-type-module/esm.js

@@ -0,0 +1 @@
+exportvarp=5;

test/parallel/test-require-mjs.js

@@ -5,7 +5,7 @@ const assert = require('assert');
 assert.throws(
 ()=>require('../fixtures/es-modules/test-esm-ok.mjs'),
 {
-message: /MustuseimporttoloadESModule/,
+message: /dynamicimport\(\)whichisavailableinallCommonJSmodules/,
 code: 'ERR_REQUIRE_ESM'
 }
 );