mocha icon indicating copy to clipboard operation
mocha copied to clipboard

feat: use require to load esm

Open legendecas opened this issue 6 months ago • 5 comments

PR Checklist

Overview

This allows compilers based on require.extensions continue to work, like ts-node with Node.js built-in TypeScript support enabled.

Hopefully unblock https://github.com/nodejs/node/pull/57298.

Fixes: https://github.com/mochajs/mocha/issues/5314 Fixes: https://github.com/mochajs/mocha/issues/5317

legendecas avatar May 13 '25 13:05 legendecas

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: legendecas / name: Chengzhong Wu (5cde7f50eabb4d9af0fff1d15998362824294896)

friendly ping @JoshuaKGoldberg

marco-ippolito avatar Jun 09 '25 07:06 marco-ippolito

Hey thanks for the ping! I was out for a bit and am catching up on things now. I'm going to try to get to testing this thoroughly this week (hopefully tomorrow or so). But the more folks who can test this out, the better.

Note that we're pretty close to releasing Mocha 12 (#5357). I also have a vague memory of someone mentioning somewhere that once Mocha drops support for pre-require(esm) versions of Node.js, this whole area of code could be simplified. I don't have the time to spelunk for it right now, but will if nobody posts that soon.

IMO it might make sense to either...

  • ⚡ Faster: Merge this in as a feature in Mocha 11.x, then take on the drastic simplification in Mocha 12
  • 🦺 More careful: merge this in as a feature in Mocha 12.0, then file the simplification as a followup (either Mocha 12 or Mocha 13, depending on how breaking it is)

I'm personally leaning to ⚡ since this change is pretty targeted, and because of how broken things are at the moment for Node.js versions with type stripping. cc @mochajs/committers - WDYT?

JoshuaKGoldberg avatar Jun 09 '25 10:06 JoshuaKGoldberg

Oh and @legendecas in the meantime, could you please try to fix the lint error? It's failing in CI.

JoshuaKGoldberg avatar Jun 09 '25 10:06 JoshuaKGoldberg

Fixed lint issue.

"Merge this in as a feature in Mocha 11.x, then take on the drastic simplification in Mocha 12" sounds good to me!

legendecas avatar Jun 09 '25 12:06 legendecas

Just to be safe, I ran this PR's changes through another set of tests today on:

  • https://github.com/eslint/eslint
  • https://github.com/Microsoft/TypeScript
  • https://github.com/legendecas/mocha-register-node-v23 (from #5314)
  • https://github.com/mbtts/babel-node-mocha-test (from #5317)

...across Node.js versions:

  • 18.18.0
  • 20.9.0
  • 21.1.0
  • 22.16.0
  • 23.11.1
  • 24.2.0

Confirmed that this fixes both of those issues and generally works. Great! I'll merge this now and publish a new release. Thanks a bunch @legendecas for the clean fix and patience with us on the PR review, this is great work! Additional shoutout to @marco-ippolito from the Node.js side and for helping push it forward. 🔥

JoshuaKGoldberg avatar Jun 18 '25 01:06 JoshuaKGoldberg

Released as 11.7.0:

  • https://github.com/mochajs/mocha/releases/tag/v11.7.0
  • https://www.npmjs.com/package/mocha/v/11.7.0

🎉

JoshuaKGoldberg avatar Jun 18 '25 01:06 JoshuaKGoldberg

This PR has the unfortunate side-effect of removing support for setups that use asynchronous Node.js loaders/hooks.

Before this PR: try import(). if that fails- try require().

After this PR: try require(). if that fails and error is ERR_REQUIRE_ASYNC_MODULE- try import(). if require() fails for any other reason (e.g. needing the hooks being registered), it won't even try to import(), not making use of any asynchronous hooks.

AviVahl avatar Jun 18 '25 11:06 AviVahl

Looks like this change had broken testing on MDN's browser-compat-data: https://github.com/mdn/browser-compat-data/actions/runs/15731639024/job/44333918076?pr=27094

Exception during run: Error: Transform failed with 1 error: /home/runner/work/browser-compat-data/browser-compat-data/index.ts:56:15: ERROR: Top-level await is currently not supported with the "cjs" output format

Also affecting the mdn-bcd-collector project in the same way: https://github.com/openwebdocs/mdn-bcd-collector/actions/runs/15732266958/job/44335981656?pr=2479

Not sure if it's an issue with tsx in particular, but something seems to be blocking us from upgrading to Mocha v11.7. Will try to dig in to see if it's something with our setup tomorrow!

queengooborg avatar Jun 18 '25 12:06 queengooborg

Lovely. Thanks @AviVahl and @queengooborg, that's good to know. Any investigation you can do would be greatly appreciated. Fixing this area is a high priority for Mocha right now.

JoshuaKGoldberg avatar Jun 18 '25 12:06 JoshuaKGoldberg

@JoshuaKGoldberg it breaks mocha with custom ts loader. https://github.com/jhipster/generator-jhipster/pull/29827 It's probably because the required module doesn't exist so it does not fallback to import due to condition. Typescript loaders takes a non existing .js module and tries to load .ts alternative and I think there is no loader support for require. I will do some test.

mshima avatar Jun 20 '25 20:06 mshima

It also breaks @hyperse/ts-node/register. It was working in Mocha 11.6.0.

MuTsunTsai avatar Jun 23 '25 01:06 MuTsunTsai

We believe we have a fix:

  1. #5383: adds a test that exercises at least one of the kinds of cases now failing in 11.7.0
  2. #5384 fixes at least all of the cases that have been reported:
    • #5380
    • #5387
    • #5388

If anybody is seeing a new error as of Mocha 11.7.0 that's not in any of those issues (and doesn't reproduce on Mocha 11.6.0) please do file a new issue.

JoshuaKGoldberg avatar Jun 23 '25 15:06 JoshuaKGoldberg