chore: add jsdoc types and typechecking #515
Open
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
I opted to spin this off into its own PR because I make some substantial (and potentially controversial) additions.
While contributing to this repository, it wasn't a pleasant experience without the strong type checking that I'm used to. Axel Navarro did the first stage to addressing this (not sure if intentionally) by adding JSDoc types.
This pushes that forward, applies all feedback that was given, makes some minor revisions, reformats the placement of types to fit the JSDoc conventions, and finally, enforces the type.
I'll like to emphasize that the commit message and PR title say type checking, not TypeScript. I do propose that we add the
typescriptpackage to our dev dependencies, but we would not be adopting the TypeScript language.TypeScript (or rather the tool,
tsc) is capable of type checking JavaScript through JSDoc type annotations. (We can use TypeScript for real if people want, but I'm actually not a big fan of*.ts, I just like usingtscin my plain-old JavaScript files.)This has already allowed us to catch a few small bugs:
Math.logdoesn't take a second argument. We should've been usingMath.log10, notMath.log(_, 10).globdoesn't take a callback, it's a Promise only API. Though we never used the third parameter anyway, so I don't think this changed anything.This will add stronger code completion and type checking in most editors and enforce correct use of types in CI. Some types can definitely be improved, but I wanted to minimize code changes for now.
Feedback welcome!
Checklist
Please review this checklist before submitting a pull request.
npm run test:all)Related