-
Notifications
You must be signed in to change notification settings - Fork 404
Migrate library to TS (Base) #905
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
Migrate library to TS (Base) #905
Conversation
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.
I leave the rest up to the other reviewers. They have more TS knowledge than me
4938ec6
to
f1ae570
Compare
package.json
Outdated
"@eslint/js": "^9.32.0", | ||
"@testing-library/react": "^16.0.0", | ||
"@types/node": "^24.0.1", | ||
"@types/ws": "^8.5.10", | ||
"eslint": "^9.0.0", | ||
"eslint": "^9.32.0", | ||
"globals": "^16.0.0", | ||
"jsdoc": "^4.0.2", | ||
"jsdom": "^26.0.0", | ||
"typescript": "^5.2.2", | ||
"typescript": "^5.9.2", | ||
"typescript-eslint": "^8.39.0", |
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.
Sorry to keep being a stickler about PR separation of concerns - I promise I'll be responsive to reviewing this over the next few business days in return!
Could we do the typescript config changes and the eslint config changes in a separate PR before this one? They should be incrementally adoptable since we already are using both typescript and ESLint.
I mainly ask because the great majority of the diff on this PR is the package-lock.json
, lol.
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.
@EzraBrooks #902 Already bumped typescript up to this version, I'm just reflecting it in the package.json. The package-lock diff should just be the eslint/typescript-eslint package dependencies.
I can split it any way you want though, this one can be tsconfig/eslint and the next can be vite/file renames?
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.
@EzraBrooks created at #909
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.
merged that one, thanks!
argh, I forgot this repo uses squash-merge - so you'll have to rebase this manually since the commit hash gets changed by merging. sorry about that!
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.
No problem, rebased and ready :)
f1ae570
to
04c0796
Compare
Include Vite Config in Intellisense tsconfig.json for type checking Signed-off-by: Drew Hoener <[email protected]>
…figuration. Remove deprecated config. Vitest provides its own defineConfig that should be used when creating test configuration in `vite.config.js`. Convert `environmentMatchGlobs` to `projects`, since the former has been deprecated Signed-off-by: Drew Hoener <[email protected]>
Easy refactor since they only have exports defined, and helps prep for converting Signed-off-by: Drew Hoener <[email protected]>
04c0796
to
a523ad7
Compare
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.
Sweet!
'./test/examples/*.js', | ||
], | ||
exclude: ['dist'], | ||
environmentMatchGlobs: [ | ||
// React example requires DOM emulation | ||
['examples/react-example/**', 'jsdom'] |
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.
This change of running everything in jsdom makes sense to me since it shouldn't be harmful, but I figured I'd leave a comment pointing out that it is a change regardless.
Public API Changes
None
Description
This is a first pass at implementing changes that I discussed in my PR #851
@EzraBrooks recommended a more incremental approach, and I'm finally back to deliver (I haven't had a lot of time because of project work, sorry).
Overview
This PR is the start of my overall attempt to migrate RosLibJS to Typescript. While the library has generated types through JSDoc, it lacks the robustness that
TypeScript
andtypescript-eslint
can provide. Among other things, a move to TS can provide greater intellisense throughout the project (e.g. typed event names in EventEmitter methods, etc).Contents
I think the commits largely speak for themselves so I'll keep it brief, I tried to make the commit messages as detailed as I could for review.
tsconfig
with some more lenient rules for now, as the migration is incrementalvite
instead ofvitest
and migrate away from a deprecated test config field.ts
extension to kickstart the process#904 outlines my general plan for the refactor, I figured it would be good if I'm going to make multiple PRs
Please let me know if you have any feedback :)