Use consistent error codes for MODULE_NOT_FOUND

[jkrems]

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.

[guybedford]

Great point, agreed this should be MODULE_NOT_FOUND for both.

Update - see below.

[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.

[jkrems]

I think for the same reason that we do nodejs/node#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)".

[MylesBorins]

Is there more work to do here?

[guybedford]

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.

[jkrems]

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 ***@***.***> 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 <#360?email_source=notifications&email_token=AAEKR5ATVXIIKBEKZ4ARYDLQUGLL7A5CNFSM4IGSHNZ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEITZEA#issuecomment-554777744>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEKR5HA5YAPHLF6PEWMIRDQUGLL7ANCNFSM4IGSHNZQ> .

[ljharb]

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

[jkrems]

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.

[ljharb]

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