Skip to content
This repository has been archived by the owner on Sep 2, 2023. It is now read-only.

Better errors on module not found #443

Closed
MylesBorins opened this issue Nov 22, 2019 · 8 comments
Closed

Better errors on module not found #443

MylesBorins opened this issue Nov 22, 2019 · 8 comments

Comments

@MylesBorins
Copy link
Contributor

I think it is going to be a fairly common pattern (for a while) that people will expect they can import a file without an extension. When a module is not found ERR_MODULE_NOT_FOUND is thrown. The error is thrown in module_wrap.cc within Maybe FinalizeResolution

Before throwing ERR_MODULE_NOT_FOUND we could check with the node resolution algorithm if there is a file that would have resolved with that specifier and let the developer know the path of the expected file.

Thoughts?

@ljharb
Copy link
Member

ljharb commented Nov 22, 2019

It'd be a great way to collect feedback if that message included a URL to file an issue as well :-)

@GeoffreyBooth
Copy link
Member

Before throwing ERR_MODULE_NOT_FOUND we could check with the node resolution algorithm if there is a file that would have resolved with that specifier and let the developer know the path of the expected file.

I was reading #442 and thinking exactly the same thing.

Error: Cannot find module /Users/daniel/code/node-esm/a imported from /Users/daniel/code/node-esm/index.js; should this have been /Users/daniel/code/node-esm/a.js?

I’ve also never understood why a Node error includes a stack trace with Node’s internals, but I guess that’s a broader nit.

@rauschma
Copy link

rauschma commented Nov 22, 2019

Potential issues:

  1. False positives that confuse people because they did not mean the mentioned file and still don’t know what they did wrong. (Should normally not be a problem, though.)
  2. What about remote module URLs where you can’t search for files?
  3. What if no file can be found? Then the user should still be told what they did wrong.
  4. What if they forgot a “../” or added one “../” too many?

A more generic warning would be (if none of the supported extensions is used): “Did you forget a filename extension?”

@coreyfarrell
Copy link
Member

@rauschma I agree with your edit "if none of the supported extensions is used". This would allow the missing extension error to appear if someone tries to import config from './rollup.config'. One issue with your proposed generic message is that it doesn't consider directory index resolution.

Not retrying the resolve with node algorithm is beneficial in some edge cases. I'm thinking about babel resolving plugins where someone configures istanbul and then babel tries to load a list of specifiers based on this name. Failing to resolve isn't always an actual failure. Maybe not a huge issue if the missing extension error check is bypassed for specifiers that look like 'package main' imports. This sort of 'import search' algorithm is sometimes used on relative paths but I suspect that is less common.

Another case is import fn from 'pkg1/fn'. Handling of pkg1/fn not found probably should depend on if pkg1 is found and what it's package.json exports says (does exports exist, does it have a directory mapping that could match).

@MylesBorins
Copy link
Contributor Author

FWIW I was imagining tha twe could scope when this happens, likely around existing ERR_MODULE_NOT_FOUND throws... currently we don't support remote URLs, but if we did I could imagine we do not offer this feature for those types of URLs.

If no module was found a generic message, similar to what we currently have, could be used.

If they searched in the wrong directory they would likely get the same generic message as above, it would fail in a similar way to how modules not being found in CJS would work.

@targos
Copy link
Member

targos commented May 8, 2020

Posting here for more visibility: should nodejs/node#31906 be backported to v12.x-staging? I haven't had time to check why, but the tests fail if that PR is applied.

@MylesBorins
Copy link
Contributor Author

I can dig in today... we should backport if we can

@MylesBorins
Copy link
Contributor Author

looks like it's fixed LOL

Closing

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants