Skip to content

Migrate library to TS #851

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

Draft
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

ahoenerBE
Copy link

-- This PR is currently a draft, please feel free to leave comments if you have recommendations

Public API Changes

The goal is to largely keep the public API the same, while adding concrete type information. I don't plan on changing any functionality, just augmenting the existing code.
There may be additional types exported for utility, but there should be no signature changes other than the addition of types

Additionally, I plan to keep the existing documentation while updating references to proper type declarations to build more comprehensive docs

Description

While TSDoc provides some type information, there isn't quite enough that can be inferred to provide concrete types about things like messages, services, etc. for concrete type checking when using this library.
I frequently contribute to RosReact to improve roslib bindings to react applications (at this point I'm pretty much the only contributor, the author doesn't make many changes), and I've ended up adding some pretty concrete typing on that side.
However, that's with some liberties and type casting that I wouldn't have to do if there was more concrete typing here.

There are concrete types at @types/roslib, but they're not always 100% in line and up to date with this main repo. Using typescript as the primary language removes the need for a separate type library and provides more concrete typing overall.

Copy link
Contributor

@EzraBrooks EzraBrooks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be happy to review this incrementally, but I'm concerned about the scope of modifying the whole codebase in one PR - which is why I took the incremental approach with generating the types from JSDoc in the first place.

For what it's worth, I've been using the develop branch of this library and using the JSDoc for type annotations in my TypeScript application with great success - haven't touched @types/roslib in quite a while.

@ahoenerBE
Copy link
Author

ahoenerBE commented Feb 26, 2025

@EzraBrooks Thanks for the feedback!
Yeah I've been thinking about the size of this PR lol. Like I said in the description, I haven't changed any of the actual function definitions other than to add typing information and things to augment intellisense (like type completion for protocol messages).
I've also been updating the tests as I go, and nothing has broken so far.

I'd be happy to break this up into a few separate PRs if that'd make it easier. I can set it up in vite to do it incrementally and then switch it all over with the last PR. So far, this PR just includes the math and URDF classes, and their respective tests, and could easily be the first to merge after I fix up the build system to do both TS and JS.

Let me know what you think the best way to proceed is

@EzraBrooks
Copy link
Contributor

I appreciate the contribution so I hate to create extra work for you, but I'd like it to be done incrementally so I can actually seriously take a look at the whole diff.

Less because I don't trust a strongly-typed refactor with good unit tests - that speaks for itself - but more because I am curious to take another wide-ranging look at the architecture of the library so we can plan additional future improvements based on the type gymnastics we have to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants