-
Notifications
You must be signed in to change notification settings - Fork 37
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
RSDK-7153 ReadAnalogReaderResponse update #298
Conversation
src/components/board/board.ts
Outdated
@@ -11,6 +11,12 @@ export interface Tick { | |||
time: number; | |||
} | |||
export type Duration = PBDuration.AsObject; | |||
export interface AnalogValue { |
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.
can we just make this export type AnalogValue = pb.ReadAnalogReaderResponse.AsObject
? I'm not 100% sure it'll work but I would like to typealias it since it doesn't introduce any new values
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.
it works, changed
src/components/board/client.ts
Outdated
return response.getValue(); | ||
const value: AnalogValue = { | ||
value: response.getValue(), | ||
minRange: response.getMinRange(), | ||
maxRange: response.getMaxRange(), | ||
stepSize: response.getStepSize(), | ||
}; | ||
return value; |
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.
can we not just return response.asObject()
here? (or whatever converts a pb.ReadAnalogReaderResponse
into a JS Object?)
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 dont think so, just tried and got Unsafe return of an any typed value.
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.
Did you also try return response.toObject()
?
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.
it says thats not a function
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.
That's... weird. Sorry, I've been spending the last 10 minutes trying to figure out why it would say it's not a function...
I tried it on my end. (Forgot I had to make all
first to get the latest proto changes). I also switched the board.ts
to return a Promise<AnalogValue>;
.
and changed this to: return response.toObject();
. It worked for me
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.
Did you run the test? the test fails with the error AssertionError: promise rejected "TypeError: response.toObject is not a function instead of resolving
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.
Ohhhh yes, that makes sense! Because in your test, you emulate what the other get<>
functions are, but NOT toObject()
. So you would have to change the mock to be like something similar to this:
BoardServiceClient.prototype.readAnalogReader = vi
.fn()
.mockImplementation((_req, _md, cb) => {
cb(null, { toObject: () => testAnalogValue });
});
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.
The thing you're mocking is the actual pb.response type itself. So you have to manually define what the functions of your mock response type would return. Now that the function readAnalogReader()
is returning response.toObject()
instead of all the other get functions, that's what you have to mock instead.
Let me know if you don't get it! It took me a while to wrap my head around it too.
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.
ohhhh okay i see, that worked thanks so much!
@@ -11,6 +11,7 @@ export interface Tick { | |||
time: number; | |||
} | |||
export type Duration = PBDuration.AsObject; | |||
export type AnalogValue = pb.ReadAnalogReaderResponse.AsObject; |
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'm assuming you defined this because users will need to be able to access this. I forgot to mention, please add this type to the main.ts
file.
(If a user never has to make their own AnalogValue
, then technically you don't need to define this at all, and can use Promise<pb.ReadAnalogReaderResponse.AsObject>
lower on in the file. You'll also have to define a ReadAnalogReaderResponse
instead of AnalogValue
object in the tests. Ignore this if you need/want an explicit AnalogValue
type!)
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.
is tick
supposed to be in that file too? I did that change a couple weeks and didnt add it
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 tick
and AnalogValue
are exported types that users need to have access to, then yes.
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.
okay will add them both, thanks
No description provided.