-
Notifications
You must be signed in to change notification settings - Fork 416
Replace SocketAdapter with Transports #1060
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
Replace SocketAdapter with Transports #1060
Conversation
EzraBrooks
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.
Looks great, just some nits
|
btw I'm planning on really highlighting this feature in the 2.0 release notes because it's cool and it will help anyone who was relying on the almost-certainly-unmaintained NodeTCP and RTC transports :) |
95cde35 to
d7caae8
Compare
|
Since this is nearly there, I'm gonna poke at it, if you don't mind. really trying to get 2.0 out the door before I get busy at my new job 😆 |
|
oop, except this PR doesn't have "allow changes from maintainers" enabled |
|
Here's a proposed change to allow fully lazy-loading ws. With this, once the tests are adjusted to pass, I think we're good to merge. |
Sorry about that! I don't even see where I can enable it =/ I'll work on getting the patch applied. Confused why the tests are failing, they all pass locally 🤔 |
|
Yeah I think it defaults to being turned off when the source branch is from an organization |
probably need to reinstall deps, since I changed the underlying PNG package |
Yep, thanks, I'm reworking the tests that were mocking pngparse |
|
Changes pushed, ready for CI to run 😎 |
doh, removing that... |
Didn't see the callbacks being used outside of this class so made them private, but the lint rules want unused variables removed. Since they were recently added, left them as-is
…uld otherwise be skipped
9fdee87 to
b60160e
Compare
EzraBrooks
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.
LGTM. will re-run that flaked test. need to figure out what the deal with that is.. it happens like 1% of the time
|
thanks so much for the contribution and sorry for the back-and-forth! |
|
Woo hoo! Happy to have contributed, thanks for shepherding this project forward 👏 |
Background
A variation of #1042 based on the comment thread there about extending the factory function concept further to abstract the Ros class from socket implementations.
Changes
SocketAdapterwithTransport, which provide an abstraction over socket implementations (native WebSocket, ws npm module, socket.io, RTC, etc).TransportFactorymodeled after theWebSocketTransportFactory.RosbridgeMessageare now emitted by the transport rather than console logged or swallowed. Gives calling code an easier programmatic option to be notified of these issues.connectiontoopento be consistent with the underlying socket event type.open,close, anderrorevents as pass-throughs of whatever value the transport emitted (e.g. string, Error, Event, etc)Example Usage