Node: Improve "Cannot find module" error

[tom-dalton-fanduel]

Is your feature request related to a problem? Please describe.

While trying to debug an issue, I found the error message from node to be slightly misleading and/or not as useful as it could have been.

Here is a toy example of the issue:

We have a package called "foo". It consists of a package.json and dist/entry.js:

package.json:

/tmp/node_demo › cat node_modules/foo/package.json 
{
  "name": "foo",
  "version": "0.1.1",
  "main": "dist/entry.js"
}

dist/entry.js

/tmp/node_demo › cat node_modules/foo/dist/entry.js 
console.log("Loaded foo module")

We can import this package, everything is good:

/tmp/node_demo › node
> require("foo")
Loaded foo module
{}

The improvement comes when we delete the entrypoint for our foo module. In my real-world case, this was done by an overzealous package-trimming routing in a CI pipeline, but I guess there are probably other ways it can occur. It's clearly the user's 'fault', but the error message isn't super obvious:

/tmp/node_demo › rm node_modules/foo/dist/entry.js 

/tmp/node_demo › node
> require("foo")
Error: Cannot find module 'foo'
    at Function.Module._resolveFilename (module.js:547:15)
    at Function.Module._load (module.js:474:25)
    at Module.require (module.js:596:17)
    at require (internal/module.js:11:18)
> /tmp/node_demo › 

The issue here is that the error message Error: Cannot find module 'foo' isn't super helpful, because you have a look in node_modules, and you see a directory foo/, and you see that directory contains package.json. Maybe you also see it contains a src/ dir, a tests/ dir, and all sorts of other stuff. In my case, this led me to believe the error was that the node interpreter was somehow configured not to look in the local node_modules directory, and led to a fair bit of wild-goose chasing.

Describe the solution you'd like

It would be better if the error message directly referred to the missing [main/entry-point]file. E.g. if the error was something like:

Error: Cannot find load main file "dist/entry.js" when loading module 'foo'
    at Function....
    ...

it would be immediately obvious why node can't load the module.

Describe alternatives you've considered

The obvious alternative is to do nothing, which would maintain the current error messaging and behaviour.

[cjihrig]

Is this what you're looking for? https://github.com/nodejs/node/pull/25690

[tom-dalton-fanduel]

It sounds pretty similar. In the example there, the stack trace is .... stuff ..., Object.<anonymous> (/Users/ofrobots/tmp/require-error/who-loaded-dis.js: ... stuff and the poster says that that who-loaded-dis line is the important part.

In my case, neither actual missing file nor the package.json [section] that refers to it are mentioned in the stacktrace. I decided to build the latest master (commit 06879aafee892deaf4f58747d1f087a265790492) which seems to include #25690. Repeating my test, the traceback is slightly different but still doesn't seem to mention the missing file:

After removing node_modules/foo/dist/entry.js as per my original example:

/tmp/nodetest › ~/repos/node/node 
> require("foo")
Thrown:
{ Error: Cannot find module 'foo'
Require stack:
- <repl>
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:662:15)
    at Function.Module._load (internal/modules/cjs/loader.js:581:25)
    at Module.require (internal/modules/cjs/loader.js:719:19)
    at require (internal/modules/cjs/helpers.js:14:16) code: 'MODULE_NOT_FOUND', requireStack: [ '<repl>' ] }
> 

So I think the answer is no, that's not quite what I'm looking for, unless I've misunderstood something :-(

[cjihrig]

I see what you mean. Unfortunately, I'm not sure we can _efficiently_ add the information you're looking for to the error.

For example, in your original post, you remove a file from /tmp/node_demo/node_modules. However, the require() algorithm checks other node_modules/ directories, where the same problem could be encountered. The error would need to be updated with all of the the failed lookups. That would almost certainly introduce overhead that, IMO, is not worth slowing down the module loader for.

[tom-dalton-fanduel]

In this specific case, the loader is presumably looking for the file <somewhere>/foo/dist/entry.js. I don't know if there are more general cases of this situation where the filepath/name could be more general, but I think in this specific case at least, even if the loader is looking in several places e.g.

/a/b/c/node_modules/foo/dist/entry.js
/d/e/f/node_modules/foo/dist/entry.js
./node_modules/foo/dist/entry.js

Then the foo/dist/entry.js part is still common and in my case would have been enough to find the root cause quite quickly - from my point of view, showing all the failed lookups wouldn't be needed if the 'missing' base part was shown.

I accept that there's probably other edge cases where you might have the same package installed in 2 locations that will be searched and they're both broken in different ways. I don't know what the current behaviour is but my guess would be that it will bork at the first broken package it encounters once the package.json has been parsed? In which case showing the specific error for that broken package would be fine from my point of view.

I understand this is going to be low priority, if indeed it is worth considering at all, and I am no expert in JS, let alone the internals of the node package loader, so I am happy to defer to your judgement on whether this is worth persuing further. :)

[cjihrig]

In this specific case, the loader is presumably looking for the file <somewhere>/foo/dist/entry.js

That's not necessarily true. Each <somewhere> would have the same name, but could be a completely different implementation.

I don't know what the current behaviour is but my guess would be that it will bork at the first broken package it encounters once the package.json has been parsed?

Not necessarily true either. Sorry, I know the loader has a ton of edge cases in it.

[tom-dalton-fanduel]

Oh well, thanks for taking the time to explain this a bit more anyway. Happy for you to close this as it seems like it's probably not a feasible benefit:cost ratio.

[BridgeAR]

I had a look at this again and I opened a PR to improve the situation:

https://github.com/nodejs/node/pull/26823

[tom-dalton-fanduel]

@BridgeAR thanks so much for implementing this! I can't say I look forward to seeing the new error in production but if I do I will be much happier than before! :D