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

🚀 Feature: if loading a file as CJS and ESM both fail, display both errors instead of just ESM #5235

Open
3 tasks done
jedwards1211 opened this issue Oct 25, 2024 · 15 comments
Labels
status: accepting prs Mocha can use your help with this one! type: feature enhancement proposal

Comments

@jedwards1211
Copy link

jedwards1211 commented Oct 25, 2024

Feature Request Checklist

Overview

Ever since chai v5 dropped support for CJS, we get errors like this when we accidentally install chai 5+:

$ mocha 

 Exception during run: TypeError: Unknown file extension ".ts" for /Users/user/src/battalion/storage-site-sim/src/__tests__/SeededRNG.test.ts
    at Object.getFileProtocolModuleFormat [as file:] (node:internal/modules/esm/get_format:160:9)
    at defaultGetFormat (node:internal/modules/esm/get_format:203:36)
    at defaultLoad (node:internal/modules/esm/load:141:22)
    at ModuleLoader.load (node:internal/modules/esm/loader:409:7)
    at ModuleLoader.moduleProvider (node:internal/modules/esm/loader:291:45)
    at link (node:internal/modules/esm/module_job:76:21) {
  code: 'ERR_UNKNOWN_FILE_EXTENSION'
}

This threw us for a loop at first, because normally our tests work fine with @babel/register handling .ts files.

What's happening is first Mocha tries to load our file as CJS, which make it as far as require('chai') which fails with ERROR_REQUIRE_ESM. Then Mocha tries to load our file as ESM instead, which triggers the above error.

Suggested Solution

If Mocha would print out the error from loading our file as CJS in addition to the above error, we would have seen what caused the problem more quickly.

Alternatives

I've thought about adding code to our toolchain to check for chai v5+ and explicitly warn...but this same issue could occur with any package we're using in tests that drops CJS support

I guess another solution would be a mocharc option to force Mocha to only try one of CJS or ESM, not both.

I can't just put type: "commonjs" in my package.json because I made my toolchain run tests in CJS mode with @babel/register, and then in ESM mode with babel-register-esm, since I'm transpiling my TypeScript source to both CJS and ESM for publishing.

Additional Info

No response

@jedwards1211 jedwards1211 added status: in triage a maintainer should (re-)triage (review) this issue type: feature enhancement proposal labels Oct 25, 2024
@JoshuaKGoldberg
Copy link
Member

This is tricky. If both CJS and ESM fail to import the file because it actually doesn't exist, then displaying both errors would be extra noise for users. If this were to be implemented it would probably need some kind of heuristic for determining whether the errors are different enough to both warrant being displayed. I don't know how straightforward that would be to implement.

Would you be able to post a minimum reproduction project we could look at to better understand this?

@JoshuaKGoldberg JoshuaKGoldberg added status: waiting for author waiting on response from OP - more information needed and removed status: in triage a maintainer should (re-)triage (review) this issue labels Oct 29, 2024
@jedwards1211
Copy link
Author

Okay I'll work on making a repro soon

@jedwards1211
Copy link
Author

I don't think the extra noise would be worse, because what it currently displays is very confusing at first in a situation like this.

Theoretically the heuristic could be whether ERR_REQUIRE_ESM came from the user's own project files or from node_modules

@JoshuaKGoldberg
Copy link
Member

👋 just checking in @jedwards1211, any luck on a repro?

@JoshuaKGoldberg JoshuaKGoldberg added the stale this has been inactive for a while... label Dec 4, 2024
@jedwards1211
Copy link
Author

Sorry I forgot about that, here you go: https://github.com/jedwards1211/mocha-cjs-esm

@JoshuaKGoldberg JoshuaKGoldberg added status: in triage a maintainer should (re-)triage (review) this issue and removed status: waiting for author waiting on response from OP - more information needed stale this has been inactive for a while... labels Dec 4, 2024
@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Jan 2, 2025

Ok thanks - I got a chance to look at the repro and it makes sense. The heuristic for which error is thrown comes from #4803 -> #4807:

The requireErr caught from the CJS require is what you're looking for. But what's actually throwing is the throw err on line 73 here:

try {
return dealWithExports(await formattedImport(file, esmDecorator));
} catch (err) {
if (
err.code === 'ERR_MODULE_NOT_FOUND' ||
err.code === 'ERR_UNKNOWN_FILE_EXTENSION' ||
err.code === 'ERR_UNSUPPORTED_DIR_IMPORT'
) {
try {
// Importing a file usually works, but the resolution of `import` is the ESM
// resolution algorithm, and not the CJS resolution algorithm. We may have
// failed because we tried the ESM resolution, so we try to `require` it.
return require(file);
} catch (requireErr) {
if (
requireErr.code === 'ERR_REQUIRE_ESM' ||
(requireErr instanceof SyntaxError &&
requireErr
.toString()
.includes('Cannot use import statement outside a module'))
) {
// ERR_REQUIRE_ESM happens when the test file is a JS file, but via type:module is actually ESM,
// AND has an import to a file that doesn't exist.
// This throws an `ERR_MODULE_NOT_FOUND` error above,
// and when we try to `require` it here, it throws an `ERR_REQUIRE_ESM`.
// What we want to do is throw the original error (the `ERR_MODULE_NOT_FOUND`),
// and not the `ERR_REQUIRE_ESM` error, which is a red herring.
//
// SyntaxError happens when in an edge case: when we're using an ESM loader that loads
// a `test.ts` file (i.e. unrecognized extension), and that file includes an unknown
// import (which throws an ERR_MODULE_NOT_FOUND). `require`-ing it will throw the
// syntax error, because we cannot require a file that has `import`-s.
throw err;
} else {
throw requireErr;
}
}
} else {
throw err;
}

requireErr: Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/josh/repos/mocha-cjs-esm/node_modules/.pnpm/[email protected]/node_modules/chai/chai.js from /Users/josh/repos/mocha-cjs-esm/test.ts not supported.
  Instead change the require of chai.js in /Users/josh/repos/mocha-cjs-esm/test.ts to a dynamic import() which is available in all CommonJS modules.
      at TracingChannel.traceSync (node:diagnostics_channel:315:14)
      at Object.<anonymous> (/Users/josh/repos/mocha-cjs-esm/test.ts:3:13)
      at Module._compile (/Users/josh/repos/mocha-cjs-esm/node_modules/.pnpm/[email protected]/node_modules/pirates/lib/index.js:117:24)
      at Object.newLoader [as .ts] (/Users/josh/repos/mocha-cjs-esm/node_modules/.pnpm/[email protected]/node_modules/pirates/lib/index.js:121:7)
      at TracingChannel.traceSync (node:diagnostics_channel:315:14)
      at exports.requireOrImport (/Users/josh/repos/mocha/lib/nodejs/esm-utils.js:55:16)
      at async exports.loadFilesAsync (/Users/josh/repos/mocha/lib/nodejs/esm-utils.js:103:20)
      at async singleRun (/Users/josh/repos/mocha/lib/cli/run-helpers.js:162:3)
      at async exports.handler (/Users/josh/repos/mocha/lib/cli/run.js:375:5) {
    code: 'ERR_REQUIRE_ESM'

I was able to fix this issue locally by having it throw requireErr instead of throw err when err.code === 'ERR_UNKNOWN_FILE_EXTENSION'. But I haven't dug into the CJS/ESM logic enough to know if that's sensible.

Accepting PRs, with the caveat that fixing this will require being able to explain why the fix is correct. Chesterton's Fence applies. 🙂

@JoshuaKGoldberg JoshuaKGoldberg added status: accepting prs Mocha can use your help with this one! and removed status: in triage a maintainer should (re-)triage (review) this issue labels Jan 2, 2025
@giltayar
Copy link
Contributor

giltayar commented Jan 5, 2025

This makes sense.

@giltayar
Copy link
Contributor

giltayar commented Jan 5, 2025

But maybe a better solution would be to remove the use of require? Just use import to import both ESM and CJS. Especially since v23, require DOES work with ESM, but not all ESM (not those with top-level await). I did this "require and then import" because the implementation of ESM in Node.js was just at the beginning and I didn't want to interfere with the code path of 99.9999% of the users at that time that used CJS test files.

But we're in 2024. I would just use import and this may probably remove all the weirdness here. It's a risky move, but I believe a good one.

@jedwards1211
Copy link
Author

jedwards1211 commented Jan 6, 2025

@giltayar using import for everything would break all users who are using CJS module loader hooks for .ts(x) or experimental syntax like @babel/register or ts-node/register, unfortunately. Because import goes through the separate and different ESM module loader hook system, even if ultimately loading CJS.

@jedwards1211
Copy link
Author

I mean, it would probably end up calling require and going through the CJS module loader if it gets to that point, but it won't get to that point for file extensions like .ts that don't get resolved by the ESM loader without hooks installed

@JoshuaKGoldberg
Copy link
Member

That's a really interesting idea @giltayar. If all test cases pass, then that's lovely.

In fact, I tried the latest Node 23.5.0, and the test case repro no longer fails!

$ node --version
v23.5.0
$ pnpm test

> [email protected] test /Users/josh/repos/mocha-cjs-esm
> mocha



  ✔ testcase

  1 passing (1ms)

https://nodejs.org/docs/latest/api/modules.html#loading-ecmascript-modules-using-require: in 22.12.0 and 23.5.0, require can load ESM by default.

So, I bet we can actually:

  1. Mark this as 'aged away' in that the newest Node versions no longer have the crash
  2. Once Mocha drops support for <22.12.0 -which will be several years from now- consider investigating removing require() altogether

Thoughts?

@jedwards1211
Copy link
Author

@JoshuaKGoldberg as I mentioned above would probably break everyone using typescript... try making testcases that use ts-node/register with CJS...

@JoshuaKGoldberg
Copy link
Member

Right, what I mean is: mark a followup issue for 2. to see if that's still the case in the future.

@jedwards1211
Copy link
Author

Well it should still be an issue for no. 1 in the newest versions of Node.
I think you would at least need to configure the ESM loader to accept .ts/.tsx extensions, even if it's ultimately loading those files as CJS.

@JoshuaKGoldberg
Copy link
Member

Gotcha. For 1, I'm not following how the difference between JS and TS users will be relevant in the newest versions of Node when using --import. If the difference is that both CJS and ESM can generally load ESM now, it shouldn't matter whether the source is JS or TS, right?

Supporting evidence: I pushed up https://github.com/JoshuaKGoldberg/mocha-examples/tree/use-chai-in-typescript-examples using chai in TypeScript-authored tests:

Footnotes

  1. https://github.com/mochajs/mocha-examples/issues/105

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: accepting prs Mocha can use your help with this one! type: feature enhancement proposal
Projects
None yet
Development

No branches or pull requests

3 participants