SocketAdapter TypeScript refactor#926
SocketAdapter TypeScript refactor#926drewhoener wants to merge 8 commits intoRobotWebTools:developfrom
Conversation
cbor-js was last published 10+ years ago, stable support is provided by `cbor`, and now the `cbor2` package. The `cbor2` library is written natively in typescript and supports all the tags we were shimming with cborTypedArrayTags.js. It also supports adding new encoders/decoders later as part of its feature set. Should be fixed up in the future to address loss of precision expected by old cbor typing functions Signed-off-by: Drew Hoener <drewhoener@gmail.com>
Signed-off-by: Drew Hoener <drewhoener@gmail.com>
…not included or bundled in any way Signed-off-by: Drew Hoener <drewhoener@gmail.com>
…evant ROS2 messages. Signed-off-by: Drew Hoener <drewhoener@gmail.com>
Signed-off-by: Drew Hoener <drewhoener@gmail.com>
Signed-off-by: Drew Hoener <drewhoener@gmail.com>
…e in the future. Signed-off-by: Drew Hoener <drewhoener@gmail.com>
…module Signed-off-by: Drew Hoener <drewhoener@gmail.com>
2e6dbf0 to
8004243
Compare
| export default function SocketAdapter(client: Ros): ISocketAdapter { | ||
| let decoder: Nullable<((raw: unknown, outputFunc: (message: object) => void) => void)> = null; | ||
| // FIXME: Ros Client types not ready yet | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access |
There was a problem hiding this comment.
Is this chicken-or-the-egg or could we do this first?
There was a problem hiding this comment.
More of a chicken-egg thing. If I did the ROS client first, I'd have the same comments in there defering the lint check until the socket types are ready.
FWIW I have the Ros client mostly done, but I figured this would make sense as 2 PRs
| }, | ||
| }, | ||
| rules: { | ||
| '@typescript-eslint/no-explicit-any': 0, |
There was a problem hiding this comment.
Personally I'd prefer we hold unknown data in unknown not any, but I won't be super picky about that if you feel strongly since I could see it being a barrier to contribution for people who aren't TypeScript experts.
There was a problem hiding this comment.
I can defer this one, I had it at one point because I had a really uncooperative type but I ended up smoothing it out. It can probably be dropped for now
| action: this.name, | ||
| values: result, | ||
| status: GoalStatus.STATUS_CANCELED, | ||
| status: GoalStatus.Canceled, |
There was a problem hiding this comment.
Isn't changing these member names technically an API-breaking change?
There was a problem hiding this comment.
I didn't think so. GoalStatus was just used in Action and wasn't exported from an index.js file, so it wasn't exposed in the actual "public api". People might have been using it, but it would've been from the dist folder which IMO doesn't count as public API. Happy to discuss it though
There was a problem hiding this comment.
looks like git mv isn't cooperating here..
There was a problem hiding this comment.
If you want the file history I can do the same dance with the linter/checker we did before?
|
Superseded by #1004 |
Public API Changes
None
Overview
This PR is a refactor of the
SocketAdaptermodule to TypeScript.While doing it, I ran into a few curiosities - a good portion of the JSDoc doesn't seem to match up with how the events emitted are actually used in examples.
When reviewing, see
SocketAdapter.ts, there's a few FIXME items in particular I'd like to get answers for before merging.This PR also replaces the old
cbor-jsdependancy withcbor2which is much more active;cbor-jshasn't been published in 10 years 0.o.I don't think it constitutes a breaking change since everything works the same in terms of public API but it's worth calling out as a significant refactor under the hood.
I'm half expecting for you to ask me to move it to a new PR lol, lmk if that's what you want.
Migrating to cbor2 is a good step for a native typescript project. cbor-js didn't have any typedefs beyond
encodeanddecodefunctions that returnedanyand didn't document the extra decoding function that was used.Speaking of which,
cbor2natively supports all the types that were being bootstrapped by thecborTypedArraymodule, so it's no longer needed.I did have a bit of an issue when implementing it though, the tests expect BigInt64 and BigUInt64 decode methods to return a plain array of numbers, even when
cbor2correctly decodes it as a BigInt64Array. I added in a shim utility method to support the truncation that was being done before but it struck me as a bit weird...Other things of note
anyin eslint, it's helpful for typing the tricker elements whenunknownjust won't do the trickAs always, please feel free to leave feedback :)
Supports #904