mocha
mocha copied to clipboard
feat: use require to load esm
PR Checklist
- [x] Addresses an existing open issue: fixes #5314, fixes #5317
- [x] That issue was marked as
status: accepting prs - [x] Steps in CONTRIBUTING.md were taken
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
The committers listed above are authorized under a signed CLA.
- :white_check_mark: login: legendecas / name: Chengzhong Wu (5cde7f50eabb4d9af0fff1d15998362824294896)
friendly ping @JoshuaKGoldberg
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?
Oh and @legendecas in the meantime, could you please try to fix the lint error? It's failing in CI.
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!
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. 🔥
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
🎉
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.
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!
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 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.
It also breaks @hyperse/ts-node/register. It was working in Mocha 11.6.0.
We believe we have a fix:
- #5383: adds a test that exercises at least one of the kinds of cases now failing in 11.7.0
- #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.