lib: improve error message on missing module

[ofrobots]

The error message we provide when a module is missing is not very useful, specially for new users. Consider:

$ node index.js
internal/modules/cjs/loader.js:605
    throw err;
    ^

Error: Cannot find module 'this-module-does-not-exist'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:603:15)
    at Function.Module._load (internal/modules/cjs/loader.js:529:25)
    at Module.require (internal/modules/cjs/loader.js:657:17)
    at require (internal/modules/cjs/helpers.js:22:18)
    at Object.<anonymous> (/Users/ofrobots/tmp/require-error/who-loaded-dis.js:1:63)
    at Module._compile (internal/modules/cjs/loader.js:721:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:732:10)
    at Module.load (internal/modules/cjs/loader.js:620:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
    at Function.Module._load (internal/modules/cjs/loader.js:552:3)

This has a lot internal details and very little useful information for the user. The key information relevant to the user in the above is the line at Object.<anonymous> (/Users/ofrobots/tmp/require-error/who-loaded-dis.js:1:63). It is less than obvious that this is what the user should pay attention to. In deeply nested dependencies it may not be clear to the user where this require itself came from. Users have to start grepping their node_modules at this point.

This PR proposes to change this to something like:

$ node index.js
internal/modules/cjs/loader.js:625
    throw err;
    ^

Error: Cannot find module 'this-module-does-not-exist'
Require stack:
- /Users/ofrobots/tmp/require-error/who-loaded-dis.js
- /Users/ofrobots/tmp/require-error/deeply-nested-module.js
- /Users/ofrobots/tmp/require-error/direct-dependency.js
- /Users/ofrobots/tmp/require-error/index.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:622:15)
    at Function.Module._load (internal/modules/cjs/loader.js:541:25)
    at Module.require (internal/modules/cjs/loader.js:677:17)
    at require (internal/modules/cjs/helpers.js:20:18)
    at Object.<anonymous> (/Users/ofrobots/tmp/require-error/who-loaded-dis.js:1:63)
    at Module._compile (internal/modules/cjs/loader.js:748:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:759:10)
    at Module.load (internal/modules/cjs/loader.js:640:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:572:12)
    at Function.Module._load (internal/modules/cjs/loader.js:564:3)

I'm somewhat on the fence about the new lines error.message. I think this is the most useful presentation we can give to the user, but I do recognize that user-space may not be expecting newlines before the stacktrace (or have we crossed this bridge already in a different core error message?). Putting the entire string on the same line is possible, but that becomes less readable.

We have been telling users to not rely overly on the precise string, but rather the error codes, but I'm not sure if the ecosystem has absorbed that fully. If not, this would be considered a semver-major.

CI: https://ci.nodejs.org/job/node-test-pull-request/20767/
CI: https://ci.nodejs.org/job/node-test-pull-request/20771/

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

@ofrobots build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2380/pipeline

[sam-github]

I really like the idea, both the more descriptive message and the require stack.

A couple thoughts:

• In the "Required by:" section, line number in the source the require was made from might be useful, if Module has that info available. Some requires are dynamic, so just searching for the required filename might not work while debugging.
• Putting the require stack as a property of the error, rather than changing the message would remove the newlines, so it wouldn't be semver-major.
• When node prints an unhandled error that is causing it to exit, it could pretty print MODULE_NOT_FOUND errors in the much friendlier form. In other cases, it would show up in logs if the entire err object is logged, which it should be, many errors have useful info in error properties that are not .code or .message.

[addaleax]

Can we also make this programmatically available on the Error object, so that we don’t tempt people to parse the error message?

[BridgeAR]

I really like the idea behind this. I am fine with the representation as is even though I would also like a property on the error (ideally an array) to have programmatic access to the require chain.

All Node.js internal logging would print that property fully (console, util.inspect, repl), so there's not a clear need to have this in the error message but the error message is likely just more accessible to the users and clearer to understand (while I personally have a slight preference for the following representation in the message).

$ node index.js
internal/modules/cjs/loader.js:621
    throw err;
    ^

Error: Cannot find module 'this-module-does-not-exist'

Require chain:
 - /Users/ofrobots/tmp/require-error/who-loaded-dis.js
 - /Users/ofrobots/tmp/require-error/deeply-nested-module.js
 - /Users/ofrobots/tmp/require-error/direct-dependency.js
 - /Users/ofrobots/tmp/require-error/index.js

    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:618:15)
    at Function.Module._load (internal/modules/cjs/loader.js:541:25)
    at Module.require (internal/modules/cjs/loader.js:673:17)
    at require (internal/modules/cjs/helpers.js:20:18)
    at Object.<anonymous> (/Users/ofrobots/tmp/require-error/who-loaded-dis.js:1:63)
...

[ofrobots]

Good suggestions 👍. Let me take a look.

[ofrobots]

Alright, I have revised this.

First up, I've added a structured property for the requireStack onto the error itself to avoid the need for anyone to parse.

As for the error message I have chosen to go with an expanded, multi-line, error message using the format suggested by @BridgeAR (thanks!). Rationale:

• it might be possible to add extra logic to the uncaught exception handling code to print additional info for MODULE_NOT_FOUND errors. I do not think it is a good idea to add this in the C++ code that would lead to lots of custom error printing logic dealing with different types of errors. An alternative would be to call a JS function on the error object itself to pretty-format the message. I do not think it is a good idea to call into arbitrary JavaScript on a shutdown path.
• The first line of the error message is unmodified. If users are parsing the error message, I expect they are not parsing beyond the first line. The test changes were quite simple.

This is semver-major. I think that is fine.

There was additional suggestion of including a line-number. I think it is a reasonable idea, but perhaps as a follow-on?

@BridgeAR @sam-github if you can another look at this, that would be great! Thanks!

EDIT: CI: https://ci.nodejs.org/job/node-test-pull-request/20767/
EDIT: CI: https://ci.nodejs.org/job/node-test-pull-request/20771/

[addaleax]

Still LGTM 👍

[joyeecheung]

One thing to note, the ES module resolution also throws MODULE_NOT_FOUND though the search is done in C++, since ESM is still experimental it's probably fine to leave it for now, but it would be helpful to leave a TODO here

node/lib/internal/modules/esm/default_resolve.js

Line 70 in 92e95f1

e.code = 'MODULE_NOT_FOUND';

[ofrobots]

CI is green: https://ci.nodejs.org/job/node-test-pull-request/20783/.

[ofrobots]

Landed in 128170f...a02e3e2.