Return a specific typed Promise<InvokeBindingResponseType> instead of Promise<object> #248
Conversation
Signed-off-by: Amast <snow@amast.dev>
Signed-off-by: Amast <snow@amast.dev>
…ead of Promise<object> Signed-off-by: Amast <snow@amast.dev>
|
Loving some of the ideas in this PR that will benefit a lot of the JS SDK going forward! Thanks a lot for this! Some initial remarks:
We will review this as a team later today and come back to you. |
| constructor(bindingName: string, operation: string) { | ||
| this.bindingName = bindingName; | ||
| this.operation = operation; | ||
|
|
| operation: string, | ||
| data: any, | ||
| metadata: Record<string, string>, | ||
| ): Promise<BindingResponse> { |
There was a problem hiding this comment.
Should it be this?
| ): Promise<BindingResponse> { | |
| ): Promise<InvokeBindingResponse> { |
| import { types } from 'util'; | ||
| import { InvokeBindingResponse } from '../../../types/binding/InvokeBindingResponse.type'; | ||
|
|
||
| export class BindingRequest { |
There was a problem hiding this comment.
Do we need to export BindingRequest and BindingResponse?
Can we use parameter properties instead of members? Class definition will be cleaner.
There was a problem hiding this comment.
Well, do we need these classes at all? send function can directly call makeRequest (instead of going via invoke), and BindingRequest or BindingResponse won't be needed.
There was a problem hiding this comment.
The reason for this is that we do not want to make destructive changes to the send function. The long-term plan is to add invoke(req: BindingRequest): Promise<BindingResponse> and invoke(name: string, operation: string, data: any, metadata: Record<string, string>): Promise<BindingResponse>
There was a problem hiding this comment.
Are you talking about changing the API name from client.binding.send to client.binding.invoke? https://docs.dapr.io/reference/api/bindings_api/#invoking-output-bindings we technically need only one method in our IClientBinding, which we have as send today.
Do we really need the BindingRequest class? Isn't it simpler to use the params name, operation, data, and metadata directly? It makes sense from a consumer point of view and eliminates the need to pack and unpack the data. The class BindingRequest itself provides no value (it's more like a struct).
Similar argument for BindingResponse. Since we already have InvokeBindingResponse, do we need to introduce yet another class here?
/cc @XavierGeerinck
There was a problem hiding this comment.
Agreed, deprecating send in favor of invoke is not something we want to do at this moment.
Regarding classes, I personally also would refrain from adding extra abstraction classes. The gRPC Classes build from dapr/dapr already provide a DTO, this would just add another DTO on top.
I can however see where this is coming from as the Java SDK does it through this way.
|
I tried to add tests for metadata, but I didn't find a way to test metadata. I need help |
|
@xAmast You can try http bindings to test metadata. |
This one you can leave out for now seeing we are working on structuring this part better |
|
ping @xAmast |
|
We decided to split up this PR:
|
Description
Issue reference
Optimize the binding call without affecting the API, and there are no associated issues
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: