RSDK-7153 ReadAnalogReaderResponse update #298
Conversation
oliviamiller
changed the title
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.
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
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.
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.
I dont think so, just tried and got Unsafe return of an any typed value. error
There was a problem hiding this comment.
Did you also try return response.toObject()?
There was a problem hiding this comment.
it says thats not a function
There was a problem hiding this comment.
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.
I don't know if this helps, but these are my changes. I ran `make format lint` without a problem.
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
If tick and AnalogValue are exported types that users need to have access to, then yes.
There was a problem hiding this comment.
okay will add them both, thanks
No description provided.