ES6 Class Declarations (Part One) #5491
Conversation
Related to mochajs#5025 This is the (nearly) complete migration to the ES6 class syntax, from the original constructor function implementation. This was originally intended to include changes for two more files, but they both were having challenges with passing tests. Those were `lib/mocha.js`, and `lib/reporters/base.js`. As they seemingly will require more formatting than simple syntax changes, I am leaving them for a separate commit.
There was a problem hiding this comment.
Looks very good, just a few small comments and questions :) thanks for this and for keeping the PR focused on one core issue!
If we are no longer using the inherits package, let's see if we can remove it from the repo entirely in this PR as well (package.json, etc.)
| * @param {Function} fn | ||
| * @private | ||
| */ | ||
| Runner.immediately = global.setImmediate || process.nextTick; |
There was a problem hiding this comment.
please move this back up to line ~250, a tiny part of me is worried these values might be changed somewhere in the next 1000 lines :D
There was a problem hiding this comment.
Ideally it would be able to stay there, I would wished it could have, too. But since ES2022 class field syntax isn't supported by the tooling we currently have, I'm not sure if there are any other ways to define them without moving it out of the ES6 class syntax scope (normal variable assignments aren't valid there).
There was a problem hiding this comment.
The source code changes look great! You've hit the right patterns IMO. Just, the test failures show there's more work to be done:
77) XUnit reporter
event handlers
on 'pending', 'pass' and 'fail' events
should add test to tests called on 'end' event:
TypeError: Class constructor XUnit cannot be invoked without 'new'
at Context.<anonymous> (test/reporters/xunit.spec.js:188:15)
at callFn (lib/runnable.js:111:584)
...
Thanks for getting started & making so much progress on this very tedious task! 🤎
JoshuaKGoldberg
added
the
status: waiting for author
Co-authored-by: Mark Wiemer <[email protected]>
This is a second commit to the first part, which was committed just before this. I am not completely familiar with code review yet, so I may need some more practice with things overall. Co-authored-by: Mark Wiemer <[email protected]>
PR Checklist
status: accepting prsOverview
This is the (nearly) complete migration to the ES6 class syntax, from the original constructor function implementation.
This was originally intended to include changes for two more files, but they both were having challenges with passing tests. Those were
lib/mocha.js, andlib/reporters/base.js. As they seemingly will require more formatting than simple syntax changes, I am leaving them for a separate commit.A concern of mine:
For the goal of diff clarity, I did not perform a Prettier format step. I am happy to add a second commit, which does the formatting separately!