-
Notifications
You must be signed in to change notification settings - Fork 123
Migrate to connect-es #4417
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 to connect-es #4417
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.
Seems like the changes are all of the same type. Everything reads a lot cleaner
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.
Did not manually test due to hardcoded file in package.json.
Code LGTM. Thanks for doing this change!
const client = new MovementSensorClient(robotClient, name, { | ||
requestLogger: rcLogConditionally, |
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.
[aside] Do you know if these clients expensive to construct? I traced the trail down through connect-es PromiseClient but didn't go much further.
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.
they are O(n) calls where n is the number of proto methods. If I were rewriting how this would work now, I wouldn't keep creating clients but just have each component have a cached client or robot client.
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.
Code LGTM, manual testing now
movement_sensor: 'movementSensorService', | ||
navigation: 'navigationService', | ||
power_sensor: 'powerSensorService', | ||
sensors: 'sensorsService', |
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.
Does the sensors service no longer exist? I was under the impression it still existed.
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.
there's no reified client for it other than the promise client. is anyone running a do command against a sensors service though?
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.
They shouldn't be - we have every intention to get rid of it - but if it exists, I'm sure someone is using it. We can skip it, was just curious.
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.
if a problem comes up with it, it wont be hard to get it working again!
|
||
return response?.toObject().sensorNamesList ?? []; | ||
const request = new sensorsApi.GetSensorsRequest({ name }); | ||
const resp = await robotClient.sensorsService.getSensors(request); |
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.
Why not create SensorsClient
like the other wrappers?
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.
because the TS SDK never had one
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.
note there's a SensorClient but not a SensorsClient
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.
not a problem then!
web/frontend/src/api/power-sensor.ts
Outdated
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.
Why are the responses for getVoltage
, getCurrent
, and getPower
arrays now?
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.
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.
Ah interesting, what is the bool that is the second entry? Could we use array destructuring?
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 think it's something about it being AC or not
console.error('done streaming robot status'); | ||
$statusStream = null; | ||
}); | ||
})().catch(console.error); |
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.
Will this catch actually catch anything since the whole thing is wrapped in a try/catch?
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.
nope good... catch
Manual testing LGTM - I wasn't seeing any operations but not sure that I would with the fakes. There are hopefully very few users on this so I think we're safe and can fix forward if anything comes up. |
Ah yeah I managed to get one using a motor gofor, many of them are too instantaneous though |
92c8f27
to
37794d5
Compare
Depends on viamrobotics/viam-typescript-sdk#368
There's nothing very interesting here and testing so far seems to have this work.