Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve ERR_REQUIRE_ESM error reports by including root cause exception report. #4675

Closed
GerHobbelt opened this issue Jul 3, 2021 · 3 comments · Fixed by #4687
Closed

improve ERR_REQUIRE_ESM error reports by including root cause exception report. #4675

GerHobbelt opened this issue Jul 3, 2021 · 3 comments · Fixed by #4687
Assignees
Labels
area: node.js command-line-or-Node.js-specific type: bug a defect, confirmed by a maintainer

Comments

@GerHobbelt
Copy link

Is your feature request related to a problem or a nice-to-have??

/Problem/! Saving time & money when the shit hits fan.

Actual scenario:

node v16, mocha 9.0.2, + custom markdown-it-footnote derivative under test -- which' code uses all imports, zero requires. Worked with mocha 9.0.1 for a while.

Confusing and /wrong/ error report by mocha 9.0.2:

Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: Z:\lib\js\markdown-it-footnote\test\test.js
require() of ES modules is not supported.
require() of Z:\lib\js\markdown-it-footnote\test\test.js from Z:\lib\js\markdown-it-footnote\node_modules\mocha\lib\esm-utils.js is an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope as ES modules.
Instead rename test.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from Z:\lib\js\markdown-it-footnote\package.json.

    at new NodeError (node:internal/errors:363:5)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1112:13)
    at Module.load (node:internal/modules/cjs/loader:975:32)
    at Function.Module._load (node:internal/modules/cjs/loader:816:12)
    at Module.require (node:internal/modules/cjs/loader:999:19)
    at require (node:internal/modules/cjs/helpers:93:18)
    at Object.exports.requireOrImport (Z:\lib\js\markdown-it-footnote\node_modules\mocha\lib\esm-utils.js:56:18)
    at async Object.exports.loadFilesAsync (Z:\lib\js\markdown-it-footnote\node_modules\mocha\lib\esm-utils.js:75:20)
    at async singleRun (Z:\lib\js\markdown-it-footnote\node_modules\mocha\lib\cli\run-helpers.js:125:3)
    at async Object.exports.handler (Z:\lib\js\markdown-it-footnote\node_modules\mocha\lib\cli\run.js:366:5)

After some serious WTF?!?! we patched mocha\lib\esm-utils.js to find out and added one line in there immediately after the catch before the error location @ line 50:

console.error(err)

which gives us this very helpful information before mocha barfs a hairball as before:

Error [ERR_MODULE_NOT_FOUND]: Cannot find module 'Z:\lib\js\markdown-it-footnote\dist\markdownitFootnote.js' imported from Z:\lib\js\markdown-it-footnote\test\t
est.js
    at new NodeError (node:internal/errors:363:5)
    at finalizeResolution (node:internal/modules/esm/resolve:321:11)
    at moduleResolve (node:internal/modules/esm/resolve:756:10)
    at Loader.defaultResolve [as _resolve] (node:internal/modules/esm/resolve:867:11)
    at Loader.resolve (node:internal/modules/esm/loader:89:40)
    at Loader.getModuleJob (node:internal/modules/esm/loader:242:28)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:73:40)
    at link (node:internal/modules/esm/module_job:72:36) {
  code: 'ERR_MODULE_NOT_FOUND'
}

This immediately gives us something to check: turns out there's a typo in the package.json. Problem fixed.

My suggestion

When dumping errors (and hint messages like you do), include the root cause exception message because this mocha failure is due to some code migration happening (the markdown-it plugin at hand) but is far removed from what you point at in the "helpful hint" about cjs/mjs/etc., which actually did cost us more time, rather than saving time.

Simply keep the original exception handy when the second attempt happens to fail and include it in the mocha error report so the end user can do their diagnostic work. In this particular example, having seen the initial exception error message, I can wholly ignore the follow-up error + hint as it'll be clear to me where to start looking then!

Post Mortem

I do realize the entire import vs require + npm situation is a horror show (bad design if you ask me, but this is way out of scope and we gotta play the cards we're dealt, sometimes): see related issues like #4652 (comment), which amazingly kicked us in the teeth at the CI but nowhere on the local dev machines (Windows based). That kind of context being given, and error diagnostics being a hard target generally speaking, this didn't help exactly as the mocha error report was quite misleading in this case.

@GerHobbelt GerHobbelt added the type: feature enhancement proposal label Jul 3, 2021
GerHobbelt added a commit to GerHobbelt/markdown-it-footnote that referenced this issue Jul 3, 2021
updated build environment
fixed some bother, filed mochajs/mocha#4675
noted istanbuljs/nyc#1381 but no dice
@giltayar
Copy link
Contributor

giltayar commented Jul 4, 2021

@GerHobbelt makes total sense. Thanks! I'll try it out in the coming days and try and figure it out how to incorporate two errors in one 😁.

My 2c on ESM migration (and totally off topic!): I actually think that the design of ESM in Node.js was (mostly) correct, and their insistence on keeping with the spec and not deviating from it will in the long run be a good thing. Even in the short run, keeping ESM async (and thus making the migration from sync CJS difficult) enables things like top-level await. I believe in the future this will also enable optimizations like faster loads, and importing from HTTP (a-la deno).

So, yes, tough migration is coming in the next 1-2 years, but the end result will be a better (and standards-compliant) ecosystem.

@giltayar
Copy link
Contributor

giltayar commented Jul 4, 2021

@GerHobbelt just to make sure that I understand the problem, can you create a repo that reproduces this problem, or at least a very detailed description of the problem, so that I have something to work with?

@GerHobbelt
Copy link
Author

@giltayar: thanks for the quick response & here's a little repo to play with: https://github.com/GerHobbelt/mocha-PoC-4675

giltayar added a commit to giltayar/mocha that referenced this issue Jul 10, 2021
@juergba juergba changed the title improve ERR_REQUIRE_ESM error reports by including root cause exception report: now we have very confusing and /wrong/ hints in the mocha error diag improve ERR_REQUIRE_ESM error reports by including root cause exception report. Jul 12, 2021
@juergba juergba added type: bug a defect, confirmed by a maintainer area: node.js command-line-or-Node.js-specific and removed type: feature enhancement proposal labels Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: node.js command-line-or-Node.js-specific type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants