Improve the error message for missing modules

[mcollina]

Currently, the error that is produced in case of a missing module is not really user friendly:

node:internal/process/esm_loader:74
    internalBinding('errors').triggerUncaughtException(
                              ^
Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'not-existing-module' imported from /Users/matteo/temp/a.mjs
    at new NodeError (node:internal/errors:363:5)
    at packageResolve (node:internal/modules/esm/resolve:698:9)
    at moduleResolve (node:internal/modules/esm/resolve:739:18)
    at Loader.defaultResolve [as _resolve] (node:internal/modules/esm/resolve:853:11)
    at Loader.resolve (node:internal/modules/esm/loader:89:40)
    at Loader.getModuleJob (node:internal/modules/esm/loader:242:28)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:73:40)
    at link (node:internal/modules/esm/module_job:72:36) {
  code: 'ERR_MODULE_NOT_FOUND'
}

Rust for example provide more user friendly errors:

error[E0432]: unresolved import `crate::constants`
 --> src/primes.rs:5:12
  |
5 | use crate::constants::{FNUM, PNUM, SMAX};
  |            ^^^^^^^^^ maybe a missing crate `constants`?

Note that our errors do not list which line the dependency was not specified.

What do you think? I think it would improve things long term.

[mcollina]

cc @nodejs/modules

[ljharb]

What's the content of a.mjs where it's not immediately apparent which line is importing "not-existing-module"?

[bmeck]

I think the issue is more about having a link with line/column number printed than anything. Showing the exact line of what the import looks like is a nicety though.

[mcollina]

I think the issue is more about having a link with line/column number printed than anything. Showing the exact line of what the import looks like is a nicety though.

Yes exactly.

The problem is the user friendliness of the error. Rust error is really friendly, while ours is not.

The one for CJS is not friendly either:

$ node a.cjs
node:internal/modules/cjs/loader:944
  throw err;
  ^

Error: Cannot find module 'not-existing-module'
Require stack:
- /Users/matteo/temp/a.cjs
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:941:15)
    at Function.Module._load (node:internal/modules/cjs/loader:774:27)
    at Module.require (node:internal/modules/cjs/loader:1013:19)
    at require (node:internal/modules/cjs/helpers:93:18)
    at Object.<anonymous> (/Users/matteo/temp/a.cjs:1:1)
    at Module._compile (node:internal/modules/cjs/loader:1109:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1138:10)
    at Module.load (node:internal/modules/cjs/loader:989:32)
    at Function.Module._load (node:internal/modules/cjs/loader:829:14)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [ '/Users/matteo/temp/a.cjs' ]
}

However this at least shows the actual line where the problem happens.

[ljharb]

node has a ton of places where eliding the stack frames for "node internals" would make things friendlier; I'm not sure why node doesn't try to do that.

ftr having a line or line/column is great and i'm all for it, i'm just skeptical that it makes a difference in practice in this case.

[targos]

Can we change what's printed before the stack trace?

What I personally find useless/confusing is this part:

node:internal/process/esm_loader:74
    internalBinding('errors').triggerUncaughtException(
                              ^

[targos]

I'd love if we could instead have something like:

/Users/matteo/temp/a.mjs:2
import something from 'not-existing-module';
                      ^^^^^^^^^^^^^^^^^^^^^

[bmeck]

I spent some time poking around the codebase and the line is actually in the stack, but the internals are polluting it with noise in a couple ways

1. setting the stack trace limit shows the line in question (part of this issue); [for static imports we are losing it in an async context]:

$ cat a.mjs

import('e404.mjs');

$ NODE_OPTIONS='--stack-trace-limit=100' node ./a.mjs
node:internal/process/promises:246
          triggerUncaughtException(err, true /* fromPromise */);
          ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'e404.mjs' imported from /Users/bfarias/Documents/oss/node/a.mjs
    at new NodeError (node:internal/errors:363:5)
    at packageResolve (node:internal/modules/esm/resolve:698:9)
    at moduleResolve (node:internal/modules/esm/resolve:739:18)
    at Loader.defaultResolve [as _resolve] (node:internal/modules/esm/resolve:853:11)
    at Loader.resolve (node:internal/modules/esm/loader:89:40)
    at Loader.getModuleJob (node:internal/modules/esm/loader:242:28)
    at Loader.import (node:internal/modules/esm/loader:177:28)
    at importModuleDynamically (node:internal/modules/esm/translators:116:35)
    at exports.importModuleDynamicallyCallback (node:internal/process/esm_loader:30:14)
    at file:///Users/bfarias/Documents/oss/node/a.mjs:2:1
    at ModuleJob.run (node:internal/modules/esm/module_job:175:25)
    at async Loader.import (node:internal/modules/esm/loader:178:24)
    at async Object.loadESM (node:internal/process/esm_loader:68:5) {
  code: 'ERR_MODULE_NOT_FOUND'
}

We could temporarily bump the limit while we are in core to be sure it is captured. There doesn't appear to be an API to bump the limit around a stack trace dynamically though.

2. Due to the nature of how we decorate .stack properties we lose the ability to retarget the ^ part of the output. In particular this isn't an ESM issue but a general issue with how promises are catching uncaught exceptions:

node/lib/internal/process/promises.js

Lines 225 to 227 in 09c9e5d

	const err = reason instanceof Error ?
	reason : generateUnhandledRejectionError(reason);
	triggerUncaughtException(err, true /* fromPromise */);

node/lib/internal/process/promises.js

Lines 244 to 246 in 09c9e5d

	const err = reason instanceof Error ?
	reason : generateUnhandledRejectionError(reason);
	triggerUncaughtException(err, true /* fromPromise */);

These actually are adding a confusing stack frame even if we replace the stack using the V8 stack trace API. Doing so also is unreliable due to parts of Node adding a .stack property directly and eagerly.

Likely this section of the codebase needs to just skip internals for the ^ output with some sort of agreement on what that means. Having the Source shown not match the first stack frame is likely a terrible idea.

Honestly, I think hiding the internal errors by default is the best approach for not only this error, but others. Perhaps coalescing them under a [...internals...] string or something so people know something was above the user code is fine. If desired we could have a verbose mode either by existing --trace flags or a new one.

We could likely add a lint rule to avoid direct assignment to .stack properties to ease this in the future, but for the module loader we could use overrideStackTrace instead of the current assignment to fix this part of the code base at least.

[ljharb]

I totally agree; I can't see any use case for showing stack frames from internals for the vast majority of node users (for any error anywhere, not just module errors)

[giltayar]

I played around with this a bit in Node v16.1.0. It seems the information is there, but it's not in the error object itself. To wit:

Let's assume bar.mjs has a syntax error. Now If foo.mjs looks like:

// foo.mjs
await import('./bar.mjs')

Then I get:

$ node foo.mjs
file:///Users/giltayar/code/tmp/try-stack/bar.mjs:4
askdjfhaksdf sadfhask fjhasdf
             ^^^^^^^^

SyntaxError: Unexpected identifier
    at Loader.moduleStrategy (node:internal/modules/esm/translators:147:18)
    at async link (node:internal/modules/esm/module_job:64:21)

Full information needed to figure out the syntax error! Great. Yet if I try and catch the error and log it, as in:

// foo.mjs
await import('./bar.mjs').catch(console.error)

Then I get:

$ node foo.mjs
SyntaxError: Unexpected identifier
    at Loader.moduleStrategy (node:internal/modules/esm/translators:147:18)
    at async link (node:internal/modules/esm/module_job:64:21)

The information about the syntax error is gone! So when Node catches an exception, it finds the full information about the syntax error somewhere. So it's there, and it's just not stored in the Error object.

[targos]

Node has access to an internal method that can add this line.
You can play with it like so:

import internalUtil from 'internal/util';

await import('./bar.mjs').catch((err) => {
  internalUtil.decorateErrorStack(err)
  console.error(err)
})

node --expose-internals ./foo.mjs
file:///Users/targos/test/bar.mjs:1
askdjfhaksdf sadfhask fjhasdf
             ^^^^^^^^
SyntaxError: Unexpected identifier
    at Loader.moduleStrategy (node:internal/modules/esm/translators:147:18)
    at async link (node:internal/modules/esm/module_job:64:21)

[giltayar]

@targos Nice! I was about to run and create a PR for Mocha, and then I saw the --expose-internals. :-(