fix: load mjs files correctly #5429
Conversation
JoshuaKGoldberg
left a comment
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
I would want to give the Node.js folks a chance to respond here just to be safe.
|
@JoshuaKGoldberg what would the Node folks respond with? The documentation is clear that files with |
|
🤷 No specific requests, just that I know they were working on this with us recently. I would feel off making non-trivial logic changes without including them. |
By "the Node folks", do you mean for example @legendecas who was involved with the recent #5366 changes? |
There was a problem hiding this comment.
Would you mind updating the test in https://github.com/mochajs/mocha/blob/main/test/integration/compiler-esm.spec.js as well?
I think the test might be ineffective to catch the case described in #5425.
|
+1 to test coverage, not sure how I missed that! Edit: lolol I was the one that opened this PR, I thought someone else wrote this code. Expect an updated test soon :) |
|
@legendecas, thanks for calling out test coverage, I think the new tests are good but I'd appreciate your review. I refactored existing ones to use a helper function so that the new tests don't triply-duplicate code. |
JoshuaKGoldberg
left a comment
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
Looks great to me! I like the test flow, it's very clear. ✨
|
By the way:
This is lovely to see in PRs again. 🥲 |
|
Failures are just Coveralls, going to merge this one :) (we will be moving to Codecov, ref #5436 ) |
…odules using require: mochajs/mocha#5429
PR Checklist
require()which breaks custom module loaders #5425status: accepting prsOverview
mjs files were sometimes loaded like cjs files despite their extension. In some versions of Node (including Node 20.19.4) this causes breaking issues caught by our tests. A separate PR will bump Node versions in CI.