modules icon indicating copy to clipboard operation
modules copied to clipboard

Use consistent error codes for MODULE_NOT_FOUND

Open hybrist opened this issue 6 years ago • 9 comments

Right now ESM uses code: 'ERR_MODULE_NOT_FOUND' while CJS throws a similar error with code: 'MODULE_NOT_FOUND'. So far that looks to be accidental and worth fixing so code that catches these kinds of mistakes doesn't have to worry about which loader happened to throw it.

hybrist avatar Jul 24 '19 17:07 hybrist

~~Great point, agreed this should be MODULE_NOT_FOUND for both.~~

Update - see below.

guybedford avatar Jul 24 '19 19:07 guybedford

ERR_MODULE_NOT_FOUND was actually an intentional upgrade path in the ESM resolver since all Node.js error codes start with ERR_ these days.

If we want to change it to MODULE_NOT_FOUND though I'm fine with that too.

guybedford avatar Jul 31 '19 01:07 guybedford

I think for the same reason that we do https://github.com/nodejs/node/pull/28905, we should have one MODULE_NOT_FOUND error code for ESM and CJS. EDIT: "same reason" meaning "so people don't have to test X different codes when handling that a dependency can't be loaded because it doesn't exist (or isn't visible)".

hybrist avatar Jul 31 '19 15:07 hybrist

Is there more work to do here?

MylesBorins avatar Nov 17 '19 08:11 MylesBorins

The status is still:

  • CJS resolver throws MODULE_NOT_FOUND
  • ESM resolver throws ERR_MODULE_NOT_FOUND
  • configuration errors throw ERR_INVALID_PACKAGE_CONFIG for both

As a user, you will either be calling the CJS require.resolve and have to watch for MODULE_NOT_FOUND and ERR_INVALID_PACKAGE_CONFIG, or you will be writing a loader and calling the ESM default resolve and have to watch for ERR_MODULE_NOT_FOUND or ERR_INVALID_PACKAGE_CONFIG.

If someone feels strongly they want the new resolver to throw the same MODULE_NOT_FOUND as the old one without the ERR_ prefix now would be the time to post a PR.

Personally I do prefer the upgrade to ERR_MODULE_NOT_FOUND as an error standardization.

guybedford avatar Nov 17 '19 19:11 guybedford

As a user of (dynamic) import, an indirect require would still bubble up the non-ERR variant while a direct require/import would case a different error code IIRC. That does seem confusing. The same issue (a CJS module doesn’t exist) gets different error codes depending on if it’s imported directly or required from something that’s imported.

That being said, I suspect in practice this won’t change much / affect too many people.

On Sun, Nov 17, 2019 at 11:28 AM Guy Bedford [email protected] wrote:

The status is still:

  • CJS resolver throws MODULE_NOT_FOUND
  • ESM resolver throws ERR_MODULE_NOT_FOUND
  • configuration errors throw ERR_INVALID_PACKAGE_CONFIG for both

As a user, you will either be calling the CJS require.resolve and have to watch for MODULE_NOT_FOUND and ERR_INVALID_PACKAGE_CONFIG, or you will be writing a loader and calling the ESM default resolve and have to watch for ERR_MODULE_NOT_FOUND or ERR_INVALID_PACKAGE_CONFIG.

If someone feels strongly they want the new resolver to throw the same MODULE_NOT_FOUND as the old one without the ERR_ prefix now would be the time to post a PR.

Personally I do prefer the upgrade to ERR_MODULE_NOT_FOUND as an error standardization.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/nodejs/modules/issues/360?email_source=notifications&email_token=AAEKR5ATVXIIKBEKZ4ARYDLQUGLL7A5CNFSM4IGSHNZ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEITZEA#issuecomment-554777744, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEKR5HA5YAPHLF6PEWMIRDQUGLL7ANCNFSM4IGSHNZQ .

hybrist avatar Nov 17 '19 21:11 hybrist

It’ll affect everyone using resolve, and probably other resolvers as well.

ljharb avatar Nov 18 '19 05:11 ljharb

It shouldn't affect anybody using require.resolve because it shouldn't ever encounter the import-loader ERR_ codes. The only scenario where the streams cross would be somebody writing the following:

import('./foo.js').catch(e => {
  if (e.code === 'ERR_MODULE_NOT_FOUND') {
    // triggers if ./foo.js isn't there or if foo.js is using `import` to load other files
    // and one of them isn't there.
  }
  if (e.code === 'MODULE_NOT_FOUND') {
    // triggers if ./foo.js is there but something in the dependency graph
    // is using `require` to load other files and one of them isn't there.
  }
});

The reason why I'd say "not an issue" is because most of the use cases for checking the errors that I'm aware of aren't interested in indirect dependencies. I would expect a guard in the code snippet above saying "but only if this error is about foo itself". Because indirect dependencies missing files is almost always a bug, not some optional feature.

hybrist avatar Nov 18 '19 16:11 hybrist

I meant https://npmjs.com/resolve, which does look at those error codes.

ljharb avatar Nov 18 '19 16:11 ljharb