feat!: change root package.json type to module#6034
Conversation
|
Build failures seem to just be codecov/codecov-action#1876. |
|
Tests are passing on my Windows machine, Node 20.19.0, with the patched #6040 (ref mark-wiemer@08f921b), no unusual output aside from expected EBADENGINE when installing |
That bug is old and about random failures, which matches #6036 . I've created a new bug for consistent failures today: #6042 . |
|
Test failed on my Windows machine with Node v22.12.0: I'm going to pause my review now given this is in draft state and you're probably aware of some of these issues :) Happy to continue running Windows- or Linux-specific checks as needed, of course :) thanks for leading this one! |
mark-wiemer
left a comment
There was a problem hiding this comment.
Partial review given draft state, matches what I want though! Mostly! ;)
| ``` | ||
|
|
||
| `mocha.run()` returns a `Runner` instance which emits many [events](https://github.com/mochajs/mocha/blob/9a7053349589344236b20301de965030eaabfea9/lib/runner.js#L52) of interest. | ||
| `mocha.run()` returns a `Runner` instance which emits many [events](https://github.com/mochajs/mocha/blob/9a7053349589344236b20301de965030eaabfea9/lib/runner.cjs#L52) of interest. |
There was a problem hiding this comment.
| `mocha.run()` returns a `Runner` instance which emits many [events](https://github.com/mochajs/mocha/blob/9a7053349589344236b20301de965030eaabfea9/lib/runner.cjs#L52) of interest. | |
| `mocha.run()` returns a `Runner` instance which emits many [events](https://github.com/mochajs/mocha/blob/9a7053349589344236b20301de965030eaabfea9/lib/runner.js#L52) of interest. |
There was a problem hiding this comment.
Applied in my PR (along with the changes in main to change blob/9a7.../lib to blob/main/lib
|
|
||
| ```js | ||
| // test/setup.js | ||
| // test/setup.cjs |
There was a problem hiding this comment.
I'm on the fence about changing docs to recommend .cjs extensions for projects that others are creating. It might seem that Mocha only supports CJS files but of course we also support ESM. I think we should keep all examples of user code using the generic .js, both for simplifying this PR and for clarity.
| // test/setup.cjs | |
| // test/setup.js |
There was a problem hiding this comment.
Applied the change to setup.js in my PR
|
|
||
| ```bash | ||
| mocha --file "./test/setup.js" "./test/**/*.spec.js" | ||
| mocha --file "./test/setup.cjs" "./test/**/*.spec.js" |
There was a problem hiding this comment.
| mocha --file "./test/setup.cjs" "./test/**/*.spec.js" | |
| mocha --file "./test/setup.js" "./test/**/*.spec.js" |
There was a problem hiding this comment.
Applied this change in my PR
There was a problem hiding this comment.
Hmm, I thought we agreed to keep mjs and cjs extensions and avoid js if at all possible. Maybe this is a premature review, nbd if you're already planning on reverting this change.
If we are going to keep some files as CJS and some as ESM in v12 (which I'm OK with), we should always use .cjs and .mjs file extensions, never .js and ideally even add a lint rule for that. Just for absolute clarity :)
- Me, with a like from Josh
|
Ha yeah, I figured I'd come back to this after the Codecov issues were done. |
|
See also my PR: #6078 |
| return resolved; | ||
| } | ||
|
|
||
| if (path.extname(fixture) === ".js") { |
There was a problem hiding this comment.
Should we even keep this safety-check? Or should we just remove it and manually fix failing tests?
(I will remove it and see if any tests fail... if none fail, I will be scared of false negatives though!)
| ? fixture | ||
| : path.resolve(__dirname, "fixtures", fixture); | ||
|
|
||
| for (const extension of [".cjs", ".js", ".mjs", ".ts"]) { |
There was a problem hiding this comment.
This "find a fixture with a matching name and non-matching extension" seems like a significantly new behavior :/
| }, | ||
| "to throw", | ||
| "Unable to read /something/wherever: bad file message", | ||
| "Unable to read /something/wherever: bad file message: Sinon-provided bad file message", |
There was a problem hiding this comment.
is this a change we want to keep?
| sourceType: "module", | ||
| ecmaVersion: 2025, | ||
| }, | ||
| rules: { |
| '" "' + | ||
| path.join("bin", "mocha") + | ||
| '" -R json --require test/compiler-fixtures/js.fixture "test/compiler-cjs/*.js"', | ||
| '" -R json --require test/setup.cjs --require test/compiler-fixtures/js.fixture.cjs "test/compiler-cjs/*.js"', |
There was a problem hiding this comment.
why the new require in these few files?
PR Checklist
status: accepting prsOverview
At long last:
package.json'stypeis switched from"commonjs"to"module"The only "backwards" change here is
lib/cli/options.mjs->lib/cli/options.cjs. It's used inrequiremocktests and we haven't migrated to a mocker that supports ESM yet.💖