-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
feat: ESM configuration file #5397
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
base: main
Are you sure you want to change the base?
Conversation
lib/cli/config.js
Outdated
| * @returns {Object} Parsed config object | ||
| */ | ||
| exports.loadConfig = filepath => { | ||
| const packageJson = require(path.resolve(utils.cwd(), 'package.json')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if there's a better / more idiomatic way in Mocha to get the user's package.json file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would personally use something like package-up
But, determining whether the caller is CJS or ESM can be more tricky than that. My instinct is it'd be best to not add in specific logic for either. Using require(esm), we can do something simpler like packageJson.default ?? packageJson and call it a day.
|
👋 @thescientist13 just checking in, are you waiting on us for anything? Now that #5358 is merged, this shouldn't have to worry about |
66732d2 to
7613e58
Compare
|
hey @JoshuaKGoldberg , thanks for checking in! Not waiting on anything per se, was just waiting to see what would happen with that other PR. I just rebased against main and pushed that up here with your feedback suggestion from earlier, so I can work on tests / docs if that's good with you. |
|
Had some intermittent test failures locally, but none of them seemed related to the CLI changes here, so throwing this up to see what happens. 😅 |
|
Hi @thescientist13 , thanks for the PR :) I was sick for a while but have recovered, I've resolved merge conflicts and triggered the workflow to run. Please review the results and let me know if you need help addressing any potential new test failures (you can review #5361 for our known issues). I'll check back on this in a bit to do my first review, hopefully all tests pass today! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5397 +/- ##
==========================================
- Coverage 93.69% 93.68% -0.02%
==========================================
Files 57 57
Lines 4396 4399 +3
Branches 849 851 +2
==========================================
+ Hits 4119 4121 +2
- Misses 277 278 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for checking in @mark-wiemer 👋 It looks like everything is passing, the only failures seem to be coming from running some of the codecov tasks? ==> Finishing downloading windows:latest
Version: v11.2.4
gpg: directory '/c/Users/runneradmin/.gnupg' created
gpg: no valid OpenPGP data found.
gpg: Total number processed: 0
==> Verifying GPG signature integrity
-> Downloading https://cli.codecov.io/latest/windows/codecov.exe.SHA256SUM
-> Downloading https://cli.codecov.io/latest/windows/codecov.exe.SHA256SUM.sig
gpg: Signature made Thu Oct 23 15:06:32 2025 CUT
gpg: using RSA key 27034E7FDB850E0BBC2C62FF806BB28AED779869
gpg: Can't check signature: No public key
==> Could not verify signature. Please contact Codecov if problem continues
Exiting...
Error: Process completed with exit code 1.There is still an open TODO item around documentation, so if you're initial review looks good, I can get that added too. Thinking something like this?
(and should I do it for both old and new docs?) |
|
We very recently switched to Codecov, I guess we'll have to work out this kink. But you're right, the tests did pass, so I'll review this one in a bit :) |
|
I'm not reviewing any PRs in detail right this moment, but I will be tomorrow. Turns out recovery from bronchitis has taken much longer than expected and I'm still not quite working 40 hours a week yet, so apologies for the delay. This is currently top of my list (but I have ~20 more PRs to check out tonight ;) ) |
mark-wiemer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Organized the tests a bit because they were bothering me, but the core changes work great, thanks for this! Also, you've taught me about the patch-package trick, so cool!
|
@thescientist13 an update to the docs would be much appreciated, and yes, please update both the old and new docs. Your current draft in your latest readme looks good :) You're welcome to add the changes to this PR |
|
Great, thanks @mark-wiemer . Will get that docs change going now. 👍 |
|
Docs changes pushed! |
PR Checklist
status: accepting prsOverview
Introduces the ability to use ESM configuration files for Mocha, leveraging the
require(ESM)capability of Node >= 20.19.0, and updated current and next docs.TODO
Per my understanding of support
require(ESM)is so far only in >= 20.19.0, so not sure how this will fare with test cases running on Node 18, and or if this is something to just call out in the documentation? Not sure if will get backported to 18.x at this point as it is EOL now...Either way, let me know your thoughts on the current implementation and the implications of Node support. Happy to add a test case / docs once I make sure this changeset is heading in the right direction. 🫡