Skip to content

Conversation

cheukt
Copy link
Member

@cheukt cheukt commented May 8, 2023

status stream used to not work at all, reworked so it works? maybe.

pretty open to suggestions as to where I should be putting the new class, and its name, but all it is is a concrete impl of ResponseStream leveraging the existing EventDispatcher class.

tested using the below block

  let stream = robot.streamStatus([], new Duration().setNanos(500_000_000));
  stream.on('data', (response: VIAM.robotApi.Status[]) => {
    console.log(response);
  });
  stream.on('status', (newStatus?: { details: unknown }) => {
    console.error('error streaming robot status', newStatus);
  });
  stream.on('end', () => {
    console.error('done streaming robot status');
  });

Copy link
Contributor

@maximpertsov maximpertsov left a comment

Choose a reason for hiding this comment

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

This seems okay - just some minor changes.

In addition, let's codify your testing example as a unit test:

let stream = robot.streamStatus([], new Duration().setNanos(500_000_000));
  stream.on('data', (response: VIAM.robotApi.Status[]) => {
    console.log(response);
  });
  stream.on('status', (newStatus?: { details: unknown }) => {
    console.error('error streaming robot status', newStatus);
  });
  stream.on('end', () => {
    console.error('done streaming robot status');
  });

src/responses.ts Outdated
Comment on lines 2 to 18
import type { ResponseStream } from './gen/robot/v1/robot_pb_service';

export class IResponseStream<T> extends EventDispatcher {
private stream: ResponseStream<any>;

constructor(stream: ResponseStream<any>) {
super();
this.stream = stream;
}

override on(
type: string,
handler: (message: any) => void
): IResponseStream<T> {
super.on(type, handler);
return this;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import type { ResponseStream } from './gen/robot/v1/robot_pb_service';
export class IResponseStream<T> extends EventDispatcher {
private stream: ResponseStream<any>;
constructor(stream: ResponseStream<any>) {
super();
this.stream = stream;
}
override on(
type: string,
handler: (message: any) => void
): IResponseStream<T> {
super.on(type, handler);
return this;
}
import type { ResponseStream as ProtoResponseStream } from './gen/robot/v1/robot_pb_service';
export class ResponseStream<T> extends EventDispatcher {
private stream: ProtoResponseStream<any>;
constructor(stream: ProtoResponseStream<any>) {
super();
this.stream = stream;
}
override on(
type: string,
handler: (message: any) => void
): ResponseStream<T> {
super.on(type, handler);
return this;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

since this isn't an interface, let's just call it ResponseStream and alias the raw proto version. if you agree let's follow this pattern for the rest of this PR.

Also, we should export this type in main to make it available to users.

Copy link
Member Author

Choose a reason for hiding this comment

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

renamed the class to ViamResponseStream (but did not export) and added alias for RobotStatusStream

cheukt and others added 3 commits May 10, 2023 16:31
@cheukt cheukt requested a review from maximpertsov May 11, 2023 18:51
@cheukt
Copy link
Member Author

cheukt commented May 11, 2023

@maximpertsov this was tested using the sample code from app and adding the bit I wrote above

@@ -1,4 +1,4 @@
export type { Robot } from './robot/robot';
export type { Robot, RobotStatusStream } from './robot/robot';
Copy link
Contributor

Choose a reason for hiding this comment

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

we should export this in src/main.ts as well so users have access to this type

@cheukt cheukt merged commit 807bfcf into viamrobotics:main May 18, 2023
@cheukt cheukt deleted the fix-status-stream branch May 18, 2023 18:07
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