-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
chore: move callback and object typedefs to a new types.d.ts #5351
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
Conversation
|
Why everything in one large Also: You can try to use (still sometimes buggy) the /** @import { Foo } from './bar.js' */ |
I don't have preferences on where to organize types, since this feels like a transient "we'll clean things up over time anyway" kind of situation to me 😄. Do you have a preference for where things should go? |
|
ping @voxpelli did you mean for #5351 (comment) to be a full review? I'm not clear on whether you're requesting changes and waiting on me, or just posting thoughts pending a full review? |
|
@JoshuaKGoldberg Posting thoughts pending a full review, thanks for pinging and asking 🙏 |
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.
Pretty good, requested a few changes :)
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.
Pull Request Overview
This PR centralizes JSDoc type definitions into a new lib/types.d.ts, moves error constants to lib/error-constants.js, and updates module files to import those typedefs. It also adds a tsconfig.json and GitHub Actions job to run tsc for type checking.
- Consolidated all
@callback/@typedefdeclarations intotypes.d.ts - Extracted error constants into
lib/error-constants.jsand updated imports - Added a
tscscript and workflow step for validating the new types
Reviewed Changes
Copilot reviewed 47 out of 47 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| lib/reporters/base.js | Removed inline FullErrorStack typedef and added import from types.d.ts |
| lib/plugin-loader.js | Removed inline PluginDefinition and PluginLoaderOptions typedefs, added imports |
| lib/nodejs/worker.js | Removed inline BufferedEvent/MochaOptions typedefs, added imports and updated @param |
| lib/nodejs/serializer.js | Removed inline SerializedEvent/SerializedWorkerResult typedefs, added imports |
| lib/nodejs/reporters/parallel-buffered.js | Removed inline BufferedEvent typedef and added import |
| lib/nodejs/parallel-buffered-runner.js | Removed inline SigIntListener/FileRunner typedefs, added imports |
| lib/nodejs/buffered-worker-pool.js | Removed inline typedefs, added imports for WorkerPoolOptions, MochaOptions, results |
| lib/mocha.js | Removed many inline typedefs, added imports for DoneCB, fixtures, options, reporter types |
| lib/interfaces/tdd.js | Added Suite typedef import |
| lib/interfaces/qunit.js | Added Suite typedef import |
| lib/interfaces/common.js | Added Context and Mocha typedef imports |
| lib/interfaces/bdd.js | Added Suite typedef import |
| lib/errors.js | Removed inline constants, imported from error-constants.js, no longer exports constants |
| lib/error-constants.js | New file exporting all Mocha error constants |
| lib/context.js | Added Runnable typedef import |
| lib/cli/watch-run.js | Added typedef imports for FSWatcher, Mocha, BeforeWatchRun, FileCollectionOptions, etc. |
| lib/cli/run-option-metadata.js | Updated JSDoc {{string:string[]}} to Record<string,string[]> |
| lib/cli/run-helpers.js | Added imports for Mocha, MochaOptions, UnmatchedFile, Runner |
| lib/cli/collect-files.js | Switched error import to error-constants, added FileCollectionOptions/Response imports |
| .github/workflows/mocha.yml | Added tsc job to run the TypeScript compiler for validation |
Comments suppressed due to low confidence (2)
lib/errors.js:418
- The
constantsobject was removed from this module’s exports but is still documented undermodule:lib/errors. If backward compatibility is required, re-exportconstantshere or update documentation to reflect the new location.
module.exports = {
lib/error-constants.js:4
- The
@memberof module:lib/errorstag is misleading in this standalone file. Consider changing it tomodule:lib/error-constantsor removing the@memberofdirective.
* @memberof module:lib/errors
Co-authored-by: Mark Wiemer <[email protected]>
|
@JoshuaKGoldberg I'm good to merge this if you are |
PR Checklist
status: accepting prsOverview
Creates a new
lib/types.d.tsand moves every@callbackand@typedef {Object}to it. A newtsconfig.json&tscscript can be run to verify that the types in that file can be type checked.Does not yet validate types in
.jsfiles (#4154, #4228). You can preview those by switchingtsconfig.json'sallowJstocheckJs. I made sure there are no more "Cannot find name ..." errors but fixing the rest of the ~650+ will be a lot more work. Much of the code will be much easier to get type-safe when it's ported from manual function prototypes to classes (#5025). I also held off porting all the info from@types/mochathere to keep the changes minimal.Does not yet try to pull in types or descriptions from
@types/mocha. That would be a bigger change too.Moves error constants from
errors.jstoerror-constants.jsto avoid circular dependency errors.